From 029555d1921e651f1ed6929561a52eb598def3fb Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 13:10:16 +0100 Subject: lib/nfs/FileReader: include Compiler.h for "final" fallback --- src/lib/nfs/FileReader.hxx | 1 + 1 file changed, 1 insertion(+) (limited to 'src/lib') diff --git a/src/lib/nfs/FileReader.hxx b/src/lib/nfs/FileReader.hxx index 7f43e0ecf..424ff766d 100644 --- a/src/lib/nfs/FileReader.hxx +++ b/src/lib/nfs/FileReader.hxx @@ -24,6 +24,7 @@ #include "Lease.hxx" #include "Callback.hxx" #include "event/DeferredMonitor.hxx" +#include "Compiler.h" #include -- cgit v1.2.3 From f5f43db2da6bf0e5f39c8fb281ccca6e3dd8e65a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 13:09:19 +0100 Subject: lib/nfs/Connection: cancel DeferredMonitor on disconnect Fixes potential second mount attempt after the old connection to the NFS server was shut down. --- src/lib/nfs/Connection.cxx | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/lib') diff --git a/src/lib/nfs/Connection.cxx b/src/lib/nfs/Connection.cxx index c2c7ceb2b..c4612f613 100644 --- a/src/lib/nfs/Connection.cxx +++ b/src/lib/nfs/Connection.cxx @@ -327,6 +327,10 @@ NfsConnection::DestroyContext() assert(GetEventLoop().IsInside()); assert(context != nullptr); + /* cancel pending DeferredMonitor that was scheduled to notify + new leases */ + DeferredMonitor::Cancel(); + if (SocketMonitor::IsDefined()) SocketMonitor::Cancel(); -- cgit v1.2.3 From b293b160079fd19cfc675fa86c2b99695aac171f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 13:27:06 +0100 Subject: lib/nfs/Connection: broadcast error before closing connection During the NfsLease::OnNfsConnectionFailed() call, the old (defunct) nfs_context may be used to close file handles. Such code does not yet exist, but will be added soon to fix other bugs. --- src/lib/nfs/Connection.cxx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/lib') diff --git a/src/lib/nfs/Connection.cxx b/src/lib/nfs/Connection.cxx index c4612f613..06d2a4d2a 100644 --- a/src/lib/nfs/Connection.cxx +++ b/src/lib/nfs/Connection.cxx @@ -409,10 +409,10 @@ NfsConnection::OnSocketReady(unsigned flags) error.Format(nfs_domain, "NFS connection has failed: %s", nfs_get_error(context)); + BroadcastError(std::move(error)); + DestroyContext(); closed = true; - - BroadcastError(std::move(error)); } else if (SocketMonitor::IsDefined() && nfs_get_fd(context) < 0) { /* this happens when rpc_reconnect_requeue() is called after the connection broke, but autoreconnet was @@ -425,10 +425,10 @@ NfsConnection::OnSocketReady(unsigned flags) error.Format(nfs_domain, "NFS socket disappeared: %s", msg); + BroadcastError(std::move(error)); + DestroyContext(); closed = true; - - BroadcastError(std::move(error)); } assert(in_event); -- cgit v1.2.3 From 3cef348f30ad8a018b1c46f257d3e719be3f96b0 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 10:42:52 +0100 Subject: lib/nfs/Manager: defer NfsConnection destruction Avoids a crash that occurs when NfsConnection::OnSocketReady() dereferences itself before returning. --- src/lib/nfs/Manager.cxx | 30 +++++++++++++++++++++++++----- src/lib/nfs/Manager.hxx | 33 +++++++++++++++++++++++++++++---- 2 files changed, 54 insertions(+), 9 deletions(-) (limited to 'src/lib') diff --git a/src/lib/nfs/Manager.cxx b/src/lib/nfs/Manager.cxx index c5aecf48d..6d50cce18 100644 --- a/src/lib/nfs/Manager.cxx +++ b/src/lib/nfs/Manager.cxx @@ -29,8 +29,10 @@ NfsManager::ManagedConnection::OnNfsConnectionError(Error &&error) { FormatError(error, "NFS error on %s:%s", GetServer(), GetExportName()); - manager.connections.erase(manager.connections.iterator_to(*this)); - delete this; + /* defer deletion so the caller + (i.e. NfsConnection::OnSocketReady()) can still use this + object */ + manager.ScheduleDelete(*this); } inline bool @@ -59,7 +61,9 @@ NfsManager::Compare::operator()(const ManagedConnection &a, NfsManager::~NfsManager() { - assert(loop.IsInside()); + assert(GetEventLoop().IsInside()); + + CollectGarbage(); connections.clear_and_dispose([](ManagedConnection *c){ delete c; @@ -71,13 +75,13 @@ NfsManager::GetConnection(const char *server, const char *export_name) { assert(server != nullptr); assert(export_name != nullptr); - assert(loop.IsInside()); + assert(GetEventLoop().IsInside()); Map::insert_commit_data hint; auto result = connections.insert_check(LookupKey{server, export_name}, Compare(), hint); if (result.second) { - auto c = new ManagedConnection(*this, loop, + auto c = new ManagedConnection(*this, GetEventLoop(), server, export_name); connections.insert_commit(*c, hint); return *c; @@ -85,3 +89,19 @@ NfsManager::GetConnection(const char *server, const char *export_name) return *result.first; } } + +void +NfsManager::CollectGarbage() +{ + assert(GetEventLoop().IsInside()); + + garbage.clear_and_dispose([](ManagedConnection *c){ + delete c; + }); +} + +void +NfsManager::OnIdle() +{ + CollectGarbage(); +} diff --git a/src/lib/nfs/Manager.hxx b/src/lib/nfs/Manager.hxx index 612b01f9c..130c81aca 100644 --- a/src/lib/nfs/Manager.hxx +++ b/src/lib/nfs/Manager.hxx @@ -23,14 +23,16 @@ #include "check.h" #include "Connection.hxx" #include "Compiler.h" +#include "event/IdleMonitor.hxx" #include +#include /** * A manager for NFS connections. Handles multiple connections to * multiple NFS servers. */ -class NfsManager { +class NfsManager final : IdleMonitor { struct LookupKey { const char *server; const char *export_name; @@ -38,6 +40,7 @@ class NfsManager { class ManagedConnection final : public NfsConnection, + public boost::intrusive::slist_base_hook>, public boost::intrusive::set_base_hook> { NfsManager &manager; @@ -63,8 +66,6 @@ class NfsManager { const LookupKey b) const; }; - EventLoop &loop; - /** * Maps server and export_name to #ManagedConnection. */ @@ -74,9 +75,18 @@ class NfsManager { Map connections; + typedef boost::intrusive::slist List; + + /** + * A list of "garbage" connection objects. Their destruction + * is postponed because they were thrown into the garbage list + * when callers on the stack were still using them. + */ + List garbage; + public: NfsManager(EventLoop &_loop) - :loop(_loop) {} + :IdleMonitor(_loop) {} /** * Must be run from EventLoop's thread. @@ -86,6 +96,21 @@ public: gcc_pure NfsConnection &GetConnection(const char *server, const char *export_name); + +private: + void ScheduleDelete(ManagedConnection &c) { + connections.erase(connections.iterator_to(c)); + garbage.push_front(c); + IdleMonitor::Schedule(); + } + + /** + * Delete all connections on the #garbage list. + */ + void CollectGarbage(); + + /* virtual methods from IdleMonitor */ + void OnIdle() override; }; #endif -- cgit v1.2.3 From 40dd968f1302da9fa65c53ba0ae0e6a12c7cda9b Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 13:03:09 +0100 Subject: lib/nfs/FileReader: update "state" in OnNfsError() Clean up the "state" to indicate that there is no longer any asynchronous operation. Fixes another NFS-related crash due to cleanup of a non-existing asynchronous operation. --- src/lib/nfs/FileReader.cxx | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'src/lib') diff --git a/src/lib/nfs/FileReader.cxx b/src/lib/nfs/FileReader.cxx index 4837e1f0e..7f5506d50 100644 --- a/src/lib/nfs/FileReader.cxx +++ b/src/lib/nfs/FileReader.cxx @@ -246,6 +246,30 @@ NfsFileReader::OnNfsCallback(unsigned status, void *data) void NfsFileReader::OnNfsError(Error &&error) { + switch (state) { + case State::INITIAL: + case State::DEFER: + case State::MOUNT: + case State::IDLE: + assert(false); + gcc_unreachable(); + + case State::OPEN: + connection->RemoveLease(*this); + state = State::INITIAL; + break; + + case State::STAT: + connection->RemoveLease(*this); + connection->Close(fh); + state = State::INITIAL; + break; + + case State::READ: + state = State::IDLE; + break; + } + OnNfsFileError(std::move(error)); } -- cgit v1.2.3 From 38f19981b2bcaa6f08f1d1e81be66d217e8da9b8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 13:50:36 +0100 Subject: lib/nfs/FileReader: reset state in OnNfsConnectionFailed() Avoid calling NfsConnection::RemoveLease(), because the lease has been removed already. --- src/lib/nfs/FileReader.cxx | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/lib') diff --git a/src/lib/nfs/FileReader.cxx b/src/lib/nfs/FileReader.cxx index 7f5506d50..79256492b 100644 --- a/src/lib/nfs/FileReader.cxx +++ b/src/lib/nfs/FileReader.cxx @@ -164,6 +164,8 @@ NfsFileReader::OnNfsConnectionFailed(const Error &error) { assert(state == State::MOUNT); + state = State::INITIAL; + Error copy; copy.Set(error); OnNfsFileError(std::move(copy)); -- cgit v1.2.3 From 016063c810281bd05c792dfe8643cc68b4c3cab2 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 14:00:32 +0100 Subject: lib/nfs/FileReader: move code to CancelOrClose() --- src/lib/nfs/FileReader.cxx | 10 ++++++++++ src/lib/nfs/FileReader.hxx | 6 ++++++ 2 files changed, 16 insertions(+) (limited to 'src/lib') diff --git a/src/lib/nfs/FileReader.cxx b/src/lib/nfs/FileReader.cxx index 79256492b..455165bbd 100644 --- a/src/lib/nfs/FileReader.cxx +++ b/src/lib/nfs/FileReader.cxx @@ -56,8 +56,18 @@ NfsFileReader::Close() return; } + /* this cancels State::MOUNT */ connection->RemoveLease(*this); + CancelOrClose(); +} + +void +NfsFileReader::CancelOrClose() +{ + assert(state != State::INITIAL && + state != State::DEFER); + if (state == State::IDLE) /* no async operation in progress: can close immediately */ diff --git a/src/lib/nfs/FileReader.hxx b/src/lib/nfs/FileReader.hxx index 424ff766d..1495a2832 100644 --- a/src/lib/nfs/FileReader.hxx +++ b/src/lib/nfs/FileReader.hxx @@ -76,6 +76,12 @@ protected: virtual void OnNfsFileError(Error &&error) = 0; private: + /** + * Cancel the current operation, if any. The NfsLease must be + * unregistered already. + */ + void CancelOrClose(); + void OpenCallback(nfsfh *_fh); void StatCallback(const struct stat *st); -- cgit v1.2.3 From e72eef421b75516e0447e4a06419d1a7aba4d853 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 25 Nov 2014 12:29:55 +0100 Subject: lib/nfs/FileReader: clean up on disconnect Avoids crash because Close() invokes a call on a destructed NfsConnection. --- src/lib/nfs/FileReader.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/lib') diff --git a/src/lib/nfs/FileReader.cxx b/src/lib/nfs/FileReader.cxx index 455165bbd..1b80f2c86 100644 --- a/src/lib/nfs/FileReader.cxx +++ b/src/lib/nfs/FileReader.cxx @@ -186,7 +186,7 @@ NfsFileReader::OnNfsConnectionDisconnected(const Error &error) { assert(state > State::MOUNT); - state = State::INITIAL; + CancelOrClose(); Error copy; copy.Set(error); -- cgit v1.2.3