diff options
| -rw-r--r-- | NEWS | 7 | ||||
| -rw-r--r-- | src/event/Loop.cxx | 3 | ||||
| -rw-r--r-- | src/lib/nfs/Blocking.cxx | 7 | ||||
| -rw-r--r-- | src/lib/nfs/Blocking.hxx | 9 | ||||
| -rw-r--r-- | src/lib/nfs/Cancellable.hxx | 6 | ||||
| -rw-r--r-- | src/lib/nfs/Connection.cxx | 149 | ||||
| -rw-r--r-- | src/lib/nfs/Connection.hxx | 33 | ||||
| -rw-r--r-- | src/storage/plugins/NfsStorage.cxx | 1 |
8 files changed, 190 insertions, 25 deletions
@@ -19,6 +19,13 @@ ver 0.20 (not yet released) * remove dependency on GLib ver 0.19.7 (not yet released) +* input + - nfs: fix crash while canceling a failing file open operation + - nfs: fix memory leak on connection failure + - nfs: fix reconnect after mount failure + - nfs: implement mount timeout (60 seconds) +* storage + - nfs: implement I/O timeout (60 seconds) * playlist - don't skip non-existent songs in "listplaylist" * fix memory allocator bug on Windows diff --git a/src/event/Loop.cxx b/src/event/Loop.cxx index 4ded68ff4..1bac7c551 100644 --- a/src/event/Loop.cxx +++ b/src/event/Loop.cxx @@ -179,9 +179,10 @@ EventLoop::Run() mutex.lock(); HandleDeferred(); busy = false; + const bool _again = again; mutex.unlock(); - if (again) + if (_again) /* re-evaluate timers because one of the IdleMonitors may have added a new timeout */ diff --git a/src/lib/nfs/Blocking.cxx b/src/lib/nfs/Blocking.cxx index 5f769c408..58eaf6af2 100644 --- a/src/lib/nfs/Blocking.cxx +++ b/src/lib/nfs/Blocking.cxx @@ -20,7 +20,9 @@ #include "config.h" #include "Blocking.hxx" #include "Connection.hxx" +#include "Domain.hxx" #include "event/Call.hxx" +#include "util/Error.hxx" bool BlockingNfsOperation::Run(Error &_error) @@ -31,7 +33,10 @@ BlockingNfsOperation::Run(Error &_error) [this](){ connection.AddLease(*this); }); /* wait for completion */ - LockWaitFinished(); + if (!LockWaitFinished()) { + _error.Set(nfs_domain, 0, "Timeout"); + return false; + } /* check for error */ if (error.IsDefined()) { diff --git a/src/lib/nfs/Blocking.hxx b/src/lib/nfs/Blocking.hxx index f8354822d..eb16dfb8c 100644 --- a/src/lib/nfs/Blocking.hxx +++ b/src/lib/nfs/Blocking.hxx @@ -35,6 +35,8 @@ class NfsConnection; * thread, and method Run() waits for completion. */ class BlockingNfsOperation : protected NfsCallback, NfsLease { + static constexpr unsigned timeout_ms = 60000; + Mutex mutex; Cond cond; @@ -52,10 +54,13 @@ public: bool Run(Error &error); private: - void LockWaitFinished() { + bool LockWaitFinished() { const ScopeLock protect(mutex); while (!finished) - cond.wait(mutex); + if (!cond.timed_wait(mutex, timeout_ms)) + return false; + + return true; } /** diff --git a/src/lib/nfs/Cancellable.hxx b/src/lib/nfs/Cancellable.hxx index be4527ac3..151be0528 100644 --- a/src/lib/nfs/Cancellable.hxx +++ b/src/lib/nfs/Cancellable.hxx @@ -157,6 +157,12 @@ public: return *i; } + + template<typename F> + void ForEach(F &&f) { + for (CT &i : list) + f(i); + } }; #endif diff --git a/src/lib/nfs/Connection.cxx b/src/lib/nfs/Connection.cxx index 06d2a4d2a..6e9f77345 100644 --- a/src/lib/nfs/Connection.cxx +++ b/src/lib/nfs/Connection.cxx @@ -34,11 +34,15 @@ extern "C" { #include <poll.h> /* for POLLIN, POLLOUT */ +static constexpr unsigned NFS_MOUNT_TIMEOUT = 60; + inline bool NfsConnection::CancellableCallback::Stat(nfs_context *ctx, const char *path, Error &error) { + assert(connection.GetEventLoop().IsInside()); + int result = nfs_stat_async(ctx, path, Callback, this); if (result < 0) { error.Format(nfs_domain, "nfs_stat_async() failed: %s", @@ -54,6 +58,8 @@ NfsConnection::CancellableCallback::OpenDirectory(nfs_context *ctx, const char *path, Error &error) { + assert(connection.GetEventLoop().IsInside()); + int result = nfs_opendir_async(ctx, path, Callback, this); if (result < 0) { error.Format(nfs_domain, "nfs_opendir_async() failed: %s", @@ -69,6 +75,8 @@ NfsConnection::CancellableCallback::Open(nfs_context *ctx, const char *path, int flags, Error &error) { + assert(connection.GetEventLoop().IsInside()); + int result = nfs_open_async(ctx, path, flags, Callback, this); if (result < 0) { @@ -85,6 +93,8 @@ NfsConnection::CancellableCallback::Stat(nfs_context *ctx, struct nfsfh *fh, Error &error) { + assert(connection.GetEventLoop().IsInside()); + int result = nfs_fstat_async(ctx, fh, Callback, this); if (result < 0) { error.Format(nfs_domain, "nfs_fstat_async() failed: %s", @@ -100,6 +110,8 @@ NfsConnection::CancellableCallback::Read(nfs_context *ctx, struct nfsfh *fh, uint64_t offset, size_t size, Error &error) { + assert(connection.GetEventLoop().IsInside()); + int result = nfs_pread_async(ctx, fh, offset, size, Callback, this); if (result < 0) { error.Format(nfs_domain, "nfs_pread_async() failed: %s", @@ -113,6 +125,7 @@ NfsConnection::CancellableCallback::Read(nfs_context *ctx, struct nfsfh *fh, inline void NfsConnection::CancellableCallback::CancelAndScheduleClose(struct nfsfh *fh) { + assert(connection.GetEventLoop().IsInside()); assert(!open); assert(close_fh == nullptr); assert(fh != nullptr); @@ -122,8 +135,21 @@ NfsConnection::CancellableCallback::CancelAndScheduleClose(struct nfsfh *fh) } inline void +NfsConnection::CancellableCallback::PrepareDestroyContext() +{ + assert(IsCancelled()); + + if (close_fh != nullptr) { + connection.InternalClose(close_fh); + close_fh = nullptr; + } +} + +inline void NfsConnection::CancellableCallback::Callback(int err, void *data) { + assert(connection.GetEventLoop().IsInside()); + if (!IsCancelled()) { assert(close_fh == nullptr); @@ -143,8 +169,10 @@ NfsConnection::CancellableCallback::Callback(int err, void *data) allocated file handle immediately */ assert(close_fh == nullptr); - struct nfsfh *fh = (struct nfsfh *)data; - connection.Close(fh); + if (err >= 0) { + struct nfsfh *fh = (struct nfsfh *)data; + connection.Close(fh); + } } else if (close_fh != nullptr) connection.DeferClose(close_fh); @@ -209,6 +237,7 @@ NfsConnection::RemoveLease(NfsLease &lease) bool NfsConnection::Stat(const char *path, NfsCallback &callback, Error &error) { + assert(GetEventLoop().IsInside()); assert(!callbacks.Contains(callback)); auto &c = callbacks.Add(callback, *this, false); @@ -225,6 +254,7 @@ bool NfsConnection::OpenDirectory(const char *path, NfsCallback &callback, Error &error) { + assert(GetEventLoop().IsInside()); assert(!callbacks.Contains(callback)); auto &c = callbacks.Add(callback, *this, true); @@ -240,12 +270,16 @@ NfsConnection::OpenDirectory(const char *path, NfsCallback &callback, const struct nfsdirent * NfsConnection::ReadDirectory(struct nfsdir *dir) { + assert(GetEventLoop().IsInside()); + return nfs_readdir(context, dir); } void NfsConnection::CloseDirectory(struct nfsdir *dir) { + assert(GetEventLoop().IsInside()); + return nfs_closedir(context, dir); } @@ -253,6 +287,7 @@ bool NfsConnection::Open(const char *path, int flags, NfsCallback &callback, Error &error) { + assert(GetEventLoop().IsInside()); assert(!callbacks.Contains(callback)); auto &c = callbacks.Add(callback, *this, true); @@ -268,6 +303,7 @@ NfsConnection::Open(const char *path, int flags, NfsCallback &callback, bool NfsConnection::Stat(struct nfsfh *fh, NfsCallback &callback, Error &error) { + assert(GetEventLoop().IsInside()); assert(!callbacks.Contains(callback)); auto &c = callbacks.Add(callback, *this, false); @@ -284,6 +320,7 @@ bool NfsConnection::Read(struct nfsfh *fh, uint64_t offset, size_t size, NfsCallback &callback, Error &error) { + assert(GetEventLoop().IsInside()); assert(!callbacks.Contains(callback)); auto &c = callbacks.Add(callback, *this, false); @@ -307,10 +344,22 @@ DummyCallback(int, struct nfs_context *, void *, void *) { } +inline void +NfsConnection::InternalClose(struct nfsfh *fh) +{ + assert(GetEventLoop().IsInside()); + assert(context != nullptr); + assert(fh != nullptr); + + nfs_close_async(context, fh, DummyCallback, nullptr); +} + void NfsConnection::Close(struct nfsfh *fh) { - nfs_close_async(context, fh, DummyCallback, nullptr); + assert(GetEventLoop().IsInside()); + + InternalClose(fh); ScheduleSocket(); } @@ -327,12 +376,26 @@ NfsConnection::DestroyContext() assert(GetEventLoop().IsInside()); assert(context != nullptr); +#ifndef NDEBUG + assert(!in_destroy); + in_destroy = true; +#endif + + if (!mount_finished) { + assert(TimeoutMonitor::IsActive()); + TimeoutMonitor::Cancel(); + } + /* cancel pending DeferredMonitor that was scheduled to notify new leases */ DeferredMonitor::Cancel(); if (SocketMonitor::IsDefined()) - SocketMonitor::Cancel(); + SocketMonitor::Steal(); + + callbacks.ForEach([](CancellableCallback &c){ + c.PrepareDestroyContext(); + }); nfs_destroy_context(context); context = nullptr; @@ -341,8 +404,11 @@ NfsConnection::DestroyContext() inline void NfsConnection::DeferClose(struct nfsfh *fh) { + assert(GetEventLoop().IsInside()); assert(in_event); assert(in_service); + assert(context != nullptr); + assert(fh != nullptr); deferred_close.push_front(fh); } @@ -350,6 +416,7 @@ NfsConnection::DeferClose(struct nfsfh *fh) void NfsConnection::ScheduleSocket() { + assert(GetEventLoop().IsInside()); assert(context != nullptr); if (!SocketMonitor::IsDefined()) { @@ -364,9 +431,35 @@ NfsConnection::ScheduleSocket() SocketMonitor::Schedule(libnfs_to_events(nfs_which_events(context))); } +inline int +NfsConnection::Service(unsigned flags) +{ + assert(GetEventLoop().IsInside()); + assert(context != nullptr); + +#ifndef NDEBUG + assert(!in_event); + in_event = true; + + assert(!in_service); + in_service = true; +#endif + + int result = nfs_service(context, events_to_libnfs(flags)); + +#ifndef NDEBUG + assert(context != nullptr); + assert(in_service); + in_service = false; +#endif + + return result; +} + bool NfsConnection::OnSocketReady(unsigned flags) { + assert(GetEventLoop().IsInside()); assert(deferred_close.empty()); bool closed = false; @@ -378,21 +471,10 @@ NfsConnection::OnSocketReady(unsigned flags) re-register it each time */ SocketMonitor::Steal(); - assert(!in_event); - in_event = true; - - assert(!in_service); - in_service = true; - - int result = nfs_service(context, events_to_libnfs(flags)); - - assert(context != nullptr); - assert(in_service); - in_service = false; + const int result = Service(flags); while (!deferred_close.empty()) { - nfs_close_async(context, deferred_close.front(), - DummyCallback, nullptr); + InternalClose(deferred_close.front()); deferred_close.pop_front(); } @@ -413,9 +495,9 @@ NfsConnection::OnSocketReady(unsigned flags) DestroyContext(); closed = true; - } else if (SocketMonitor::IsDefined() && nfs_get_fd(context) < 0) { + } else if (nfs_get_fd(context) < 0) { /* this happens when rpc_reconnect_requeue() is called - after the connection broke, but autoreconnet was + after the connection broke, but autoreconnect was disabled - nfs_service() returns 0 */ Error error; const char *msg = nfs_get_error(context); @@ -431,8 +513,12 @@ NfsConnection::OnSocketReady(unsigned flags) closed = true; } + assert(context == nullptr || nfs_get_fd(context) >= 0); + +#ifndef NDEBUG assert(in_event); in_event = false; +#endif if (context != nullptr) ScheduleSocket(); @@ -444,10 +530,14 @@ inline void NfsConnection::MountCallback(int status, gcc_unused nfs_context *nfs, gcc_unused void *data) { + assert(GetEventLoop().IsInside()); assert(context == nfs); mount_finished = true; + assert(TimeoutMonitor::IsActive() || in_destroy); + TimeoutMonitor::Cancel(); + if (status < 0) { postponed_mount_error.Format(nfs_domain, status, "nfs_mount_async() failed: %s", @@ -468,6 +558,7 @@ NfsConnection::MountCallback(int status, nfs_context *nfs, void *data, inline bool NfsConnection::MountInternal(Error &error) { + assert(GetEventLoop().IsInside()); assert(context == nullptr); context = nfs_init_context(); @@ -478,8 +569,14 @@ NfsConnection::MountInternal(Error &error) postponed_mount_error.Clear(); mount_finished = false; + + TimeoutMonitor::ScheduleSeconds(NFS_MOUNT_TIMEOUT); + +#ifndef NDEBUG in_service = false; in_event = false; + in_destroy = false; +#endif if (nfs_mount_async(context, server.c_str(), export_name.c_str(), MountCallback, this) != 0) { @@ -536,8 +633,22 @@ NfsConnection::BroadcastError(Error &&error) } void +NfsConnection::OnTimeout() +{ + assert(GetEventLoop().IsInside()); + assert(!mount_finished); + + mount_finished = true; + DestroyContext(); + + BroadcastMountError(Error(nfs_domain, "Mount timeout")); +} + +void NfsConnection::RunDeferred() { + assert(GetEventLoop().IsInside()); + if (context == nullptr) { Error error; if (!MountInternal(error)) { diff --git a/src/lib/nfs/Connection.hxx b/src/lib/nfs/Connection.hxx index aebafd4d3..3969a7e8f 100644 --- a/src/lib/nfs/Connection.hxx +++ b/src/lib/nfs/Connection.hxx @@ -23,6 +23,7 @@ #include "Lease.hxx" #include "Cancellable.hxx" #include "event/SocketMonitor.hxx" +#include "event/TimeoutMonitor.hxx" #include "event/DeferredMonitor.hxx" #include "util/Error.hxx" @@ -40,7 +41,7 @@ class NfsCallback; /** * An asynchronous connection to a NFS server. */ -class NfsConnection : SocketMonitor, DeferredMonitor { +class NfsConnection : SocketMonitor, TimeoutMonitor, DeferredMonitor { class CancellableCallback : public CancellablePointer<NfsCallback> { NfsConnection &connection; @@ -84,6 +85,13 @@ class NfsConnection : SocketMonitor, DeferredMonitor { */ void CancelAndScheduleClose(struct nfsfh *fh); + /** + * Called by NfsConnection::DestroyContext() right + * before nfs_destroy_context(). This object is given + * a chance to prepare for the latter. + */ + void PrepareDestroyContext(); + private: static void Callback(int err, struct nfs_context *nfs, void *data, void *private_data); @@ -111,6 +119,7 @@ class NfsConnection : SocketMonitor, DeferredMonitor { Error postponed_mount_error; +#ifndef NDEBUG /** * True when nfs_service() is being called. */ @@ -122,13 +131,20 @@ class NfsConnection : SocketMonitor, DeferredMonitor { */ bool in_event; + /** + * True when DestroyContext() is being called. + */ + bool in_destroy; +#endif + bool mount_finished; public: gcc_nonnull_all NfsConnection(EventLoop &_loop, const char *_server, const char *_export_name) - :SocketMonitor(_loop), DeferredMonitor(_loop), + :SocketMonitor(_loop), TimeoutMonitor(_loop), + DeferredMonitor(_loop), server(_server), export_name(_export_name), context(nullptr) {} @@ -185,6 +201,11 @@ private: void DestroyContext(); /** + * Wrapper for nfs_close_async(). + */ + void InternalClose(struct nfsfh *fh); + + /** * Invoke nfs_close_async() after nfs_service() returns. */ void DeferClose(struct nfsfh *fh); @@ -200,9 +221,17 @@ private: void ScheduleSocket(); + /** + * Wrapper for nfs_service(). + */ + int Service(unsigned flags); + /* virtual methods from SocketMonitor */ virtual bool OnSocketReady(unsigned flags) override; + /* virtual methods from TimeoutMonitor */ + void OnTimeout() final; + /* virtual methods from DeferredMonitor */ virtual void RunDeferred() override; }; diff --git a/src/storage/plugins/NfsStorage.cxx b/src/storage/plugins/NfsStorage.cxx index 8ddb14250..823d662c5 100644 --- a/src/storage/plugins/NfsStorage.cxx +++ b/src/storage/plugins/NfsStorage.cxx @@ -146,6 +146,7 @@ private: const ScopeLock protect(mutex); state = _state; + last_error.Clear(); last_error.Set(error); cond.broadcast(); } |