From 23b7963096624f0b49f979e553e74bc040532fcb Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 1 Sep 2022 02:34:26 +0000 Subject: [PATCH] Fewer "busy shutdown" crashes; simpler shutdown cleanup (#1128) Preparing for shutdown after 186724 requests Waiting 1 seconds for active connections to finish Closing HTTP(S) port [::]:3128 FATAL: assertion failed: Transients.cc:192: "oldE->mem_obj && oldE->mem_obj->xitTable.index == index" This change removes two steps at the end of the manual shutdown sequence (FreeAllFs() and Store::FreeMemory()) and associated deadly Store hacks, adjusting a few Store checks as needed. Also, StoreFileSystem hierarchy declarations were slightly updated when half of its virtual methods were removed. Key changes are detailed below. ### Do not disable Store [by dropping Store::Root()] during shutdown Disabling Store (i.e. calling Store::FreeMemory()) resulted in "do not use Root() during shutdown" assertion-avoiding hacks that bypassed code maintaining key invariants. Those hacks were probably accommodating Store-using destruction sequences triggered by SquidShutdown() or later code, _but_ the hacks also ran during the graceful shutdown period! Violating those invariants in MemObject destructor led to the quoted assertions when CollapsedForwarding::HandleNotification() ran during graceful shutdown. We could add more state to narrow down hacks scope, or we could disable IPC notifications that triggered the known deadly shutdown sequence, but * We should not disable assertions during shutdown because many are about essential (e.g., preventing data corruption) invariants and because it is not practical to disable all assertions that should be disabled (and keep disabling the new ones correctly). The idea leads to unsafe code that is also more difficult to write and maintain. * We should not disable kid-to-kid notifications because some are required for finishing transactions (that can still be successfully finished) during graceful shutdown. Increasing successful completion chances is the whole idea behind that graceful shutdown feature! Instead, Store should provide (essential) services during shutdown. TODO: Update those unit test cases that use Store::FreeMemory() but should not. That requires careful analysis because the next test cases may rely on the previous case removing its Store object/statistics/etc. The same dedicated project might (help) address commit 27685ef TODO about unwanted Store unit test order dependencies. ### Do not disable StoreFileSystem service during shutdown Similar to the Store service, manually killing FS service is a bad idea. The underlying code was also essentially unused and buggy: * Nobody called RegisterAllFsWithCacheManager(). A caller would assert. * Nobody called Fs::Clean(). A call could crash Squid because deleted objects could still be in use by others (they are not refcounted). * SetupAllFs() was called but did essentially nothing. * FreeAllFs() was called but had only harmful effects: It leaked memory and removed usable FS registrations, increasing shutdown dangers. See also: Commit 3a85184 documents why we should avoid explicit "delete essentialService" calls (and why some crashes may still occur if we do). --- src/MemObject.cc | 8 +++---- src/MemStore.cc | 3 +-- src/StoreFileSystem.cc | 34 ------------------------------ src/StoreFileSystem.h | 19 ++++------------- src/fs/Module.cc | 20 ------------------ src/fs/Module.h | 1 - src/fs/rock/RockStoreFileSystem.cc | 23 -------------------- src/fs/rock/RockStoreFileSystem.h | 8 +++---- src/fs/ufs/StoreFSufs.h | 25 +++------------------- src/main.cc | 6 ------ src/store.cc | 17 +++++++-------- src/store_client.cc | 7 +++++- src/store_swapout.cc | 10 +++++---- src/tests/testRock.cc | 2 -- 14 files changed, 34 insertions(+), 149 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 93ecf3f7cc..a8e5aef1ae 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -110,11 +110,9 @@ MemObject::~MemObject() checkUrlChecksum(); #endif - if (!shutting_down) { // Store::Root() is FATALly missing during shutdown - assert(xitTable.index < 0); - assert(memCache.index < 0); - assert(swapout.sio == nullptr); - } + assert(xitTable.index < 0); + assert(memCache.index < 0); + assert(swapout.sio == nullptr); data_hdr.freeContent(); } diff --git a/src/MemStore.cc b/src/MemStore.cc index 5f2ae2904c..9dbd328988 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -587,9 +587,8 @@ MemStore::shouldCache(StoreEntry &e) const return false; } - // Store::Root() in the next check below is FATALly missing during shutdown if (shutting_down) { - debugs(20, 5, "yield to shutdown: " << e); + debugs(20, 5, "avoid heavy optional work during shutdown: " << e); return false; } diff --git a/src/StoreFileSystem.cc b/src/StoreFileSystem.cc index fb58e96e7d..066cdcf62c 100644 --- a/src/StoreFileSystem.cc +++ b/src/StoreFileSystem.cc @@ -13,21 +13,6 @@ std::vector *StoreFileSystem::_FileSystems = nullptr; -void -StoreFileSystem::RegisterAllFsWithCacheManager(void) -{ - for (iterator i = GetFileSystems().begin(); i != GetFileSystems().end(); ++i) - (*i)->registerWithCacheManager(); -} - -void -StoreFileSystem::SetupAllFs() -{ - for (iterator i = GetFileSystems().begin(); i != GetFileSystems().end(); ++i) - /* Call the FS to set up capabilities and initialize the FS driver */ - (*i)->setup(); -} - void StoreFileSystem::FsAdd(StoreFileSystem &instance) { @@ -56,20 +41,6 @@ StoreFileSystem::GetFileSystems() return *_FileSystems; } -/* - * called when a graceful shutdown is to occur - * of each fs module. - */ -void -StoreFileSystem::FreeAllFs() -{ - while (!GetFileSystems().empty()) { - StoreFileSystem *fs = GetFileSystems().back(); - GetFileSystems().pop_back(); - fs->done(); - } -} - StoreFileSystem * StoreFileSystem::FindByType(const char *type) { @@ -80,8 +51,3 @@ StoreFileSystem::FindByType(const char *type) return nullptr; } -/* no filesystem is required to export statistics */ -void -StoreFileSystem::registerWithCacheManager(void) -{} - diff --git a/src/StoreFileSystem.h b/src/StoreFileSystem.h index 92738ce431..b558a18070 100644 --- a/src/StoreFileSystem.h +++ b/src/StoreFileSystem.h @@ -9,6 +9,7 @@ #ifndef SQUID_STOREFILESYSTEM_H #define SQUID_STOREFILESYSTEM_H +#include "base/TypeTraits.h" #include "store/forward.h" #include @@ -86,37 +87,25 @@ * The core API for storage modules this class provides all the hooks * squid uses to interact with a filesystem IO module. */ -class StoreFileSystem +class StoreFileSystem: public Interface { public: - static void SetupAllFs(); static void FsAdd(StoreFileSystem &); - static void FreeAllFs(); static StoreFileSystem *FindByType(const char *type); static std::vector const &FileSystems(); typedef std::vector::iterator iterator; typedef std::vector::const_iterator const_iterator; - StoreFileSystem() : initialised(false) {} - virtual ~StoreFileSystem() {} + StoreFileSystem() = default; + StoreFileSystem(StoreFileSystem &&) = delete; // no copying/moving of any kind virtual char const *type () const = 0; virtual SwapDir *createSwapDir() = 0; - virtual void done() = 0; - virtual void setup() = 0; - // Not implemented - StoreFileSystem(StoreFileSystem const &); - StoreFileSystem &operator=(StoreFileSystem const&); - -protected: - bool initialised; - virtual void registerWithCacheManager(void); private: static std::vector &GetFileSystems(); static std::vector *_FileSystems; - static void RegisterAllFsWithCacheManager(void); }; // TODO: Kill this typedef! diff --git a/src/fs/Module.cc b/src/fs/Module.cc index 1c0ec9a0fe..b8319e473b 100644 --- a/src/fs/Module.cc +++ b/src/fs/Module.cc @@ -51,23 +51,3 @@ void Fs::Init() } -void Fs::Clean() -{ -#if HAVE_FS_UFS - delete UfsInstance; -#endif - -#if HAVE_FS_AUFS - delete AufsInstance; -#endif - -#if HAVE_FS_DISKD - delete DiskdInstance; -#endif - -#if HAVE_FS_ROCK - delete RockInstance; -#endif - -} - diff --git a/src/fs/Module.h b/src/fs/Module.h index dcb51dd179..473e12cd27 100644 --- a/src/fs/Module.h +++ b/src/fs/Module.h @@ -13,7 +13,6 @@ namespace Fs { void Init(); -void Clean(); } // namespace Fs diff --git a/src/fs/rock/RockStoreFileSystem.cc b/src/fs/rock/RockStoreFileSystem.cc index e3b4bd5762..d8c67c0e76 100644 --- a/src/fs/rock/RockStoreFileSystem.cc +++ b/src/fs/rock/RockStoreFileSystem.cc @@ -33,26 +33,3 @@ Rock::StoreFileSystem::createSwapDir() return new SwapDir(); } -void -Rock::StoreFileSystem::done() -{ -} - -void -Rock::StoreFileSystem::registerWithCacheManager() -{ - assert(false); // XXX: implement -} - -void -Rock::StoreFileSystem::setup() -{ - debugs(92,2, "Will use Rock FS"); -} - -void -Rock::StoreFileSystem::Stats(StoreEntry *) -{ - assert(false); // XXX: implement -} - diff --git a/src/fs/rock/RockStoreFileSystem.h b/src/fs/rock/RockStoreFileSystem.h index 61492891bf..a7cbae6ca3 100644 --- a/src/fs/rock/RockStoreFileSystem.h +++ b/src/fs/rock/RockStoreFileSystem.h @@ -25,11 +25,9 @@ public: StoreFileSystem(); virtual ~StoreFileSystem(); - virtual char const *type() const; - virtual SwapDir *createSwapDir(); - virtual void done(); - virtual void registerWithCacheManager(); - virtual void setup(); + /* StoreFileSystem API */ + virtual char const *type() const override; + virtual SwapDir *createSwapDir() override; private: //static Stats Stats_; diff --git a/src/fs/ufs/StoreFSufs.h b/src/fs/ufs/StoreFSufs.h index 5434ac0eb4..7c453da674 100644 --- a/src/fs/ufs/StoreFSufs.h +++ b/src/fs/ufs/StoreFSufs.h @@ -38,13 +38,9 @@ public: StoreFSufs(char const *DefaultModuleType, char const *label); virtual ~StoreFSufs() {} - virtual char const *type() const; - virtual SwapDir *createSwapDir(); - virtual void done(); - virtual void setup(); - /** Not implemented */ - StoreFSufs (StoreFSufs const &); - StoreFSufs &operator=(StoreFSufs const &); + /* StoreFileSystem API */ + virtual const char *type() const override; + virtual SwapDir *createSwapDir() override; protected: DiskIOModule *IO; @@ -73,21 +69,6 @@ StoreFSufs::createSwapDir() return result; } -template -void -StoreFSufs::done() -{ - initialised = false; -} - -template -void -StoreFSufs::setup() -{ - assert(!initialised); - initialised = true; -} - } /* namespace Ufs */ } /* namespace Fs */ diff --git a/src/main.cc b/src/main.cc index 64184e6547..74fe2a0896 100644 --- a/src/main.cc +++ b/src/main.cc @@ -1559,15 +1559,11 @@ SquidMain(int argc, char **argv) storeFsInit(); /* required for config parsing */ - /* TODO: call the FS::Clean() in shutdown to do Fs cleanups */ Fs::Init(); /* May not be needed for parsing, have not audited for such */ DiskIOModule::SetupAllModules(); - /* Shouldn't be needed for config parsing, but have not audited for such */ - StoreFileSystem::SetupAllFs(); - /* we may want the parsing process to set this up in the future */ Store::Init(); Acl::Init(); @@ -2112,9 +2108,7 @@ SquidShutdown() storeLogClose(); accessLogClose(); Store::Root().sync(); /* Flush log close */ - StoreFileSystem::FreeAllFs(); DiskIOModule::FreeAllModules(); - Store::FreeMemory(); fdDumpOpen(); diff --git a/src/store.cc b/src/store.cc index debb92655a..b6e808279f 100644 --- a/src/store.cc +++ b/src/store.cc @@ -360,10 +360,9 @@ StoreEntry::destroyMemObject() { debugs(20, 3, mem_obj << " in " << *this); - // Store::Root() is FATALly missing during shutdown - if (hasTransients() && !shutting_down) + if (hasTransients()) Store::Root().transientsDisconnect(*this); - if (hasMemStore() && !shutting_down) + if (hasMemStore()) Store::Root().memoryDisconnect(*this); if (auto memObj = mem_obj) { @@ -380,8 +379,7 @@ destroyStoreEntry(void *data) StoreEntry *e = static_cast(static_cast(data)); assert(e != nullptr); - // Store::Root() is FATALly missing during shutdown - if (e->hasDisk() && !shutting_down) + if (e->hasDisk()) e->disk().disconnect(*e); e->destroyMemObject(); @@ -1095,8 +1093,7 @@ StoreEntry::abort() void storeGetMemSpace(int size) { - if (!shutting_down) // Store::Root() is FATALly missing during shutdown - Store::Root().freeMemorySpace(size); + Store::Root().freeMemorySpace(size); } /* thunk through to Store::Root().maintain(). Note that this would be better still @@ -1254,6 +1251,9 @@ StoreEntry::memoryCachable() if (!checkCachable()) return 0; + if (shutting_down) + return 0; // avoid heavy optional work during shutdown + if (mem_obj == nullptr) return 0; @@ -1753,8 +1753,7 @@ StoreEntry::storeWritingCheckpoint() } debugs(20, 7, "done with writing " << *this); - if (!shutting_down) // Store::Root() is FATALly missing during shutdown - Store::Root().noteStoppedSharedWriting(*this); + Store::Root().noteStoppedSharedWriting(*this); } void diff --git a/src/store_client.cc b/src/store_client.cc index 725da0b911..6428492d82 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -813,7 +813,7 @@ CheckQuickAbortIsReasonable(StoreEntry * entry) return false; } - if (!shutting_down && Store::Root().transientReaders(*entry)) { + if (Store::Root().transientReaders(*entry)) { debugs(90, 3, "quick-abort? NO still have one or more transient readers"); return false; } @@ -828,6 +828,11 @@ CheckQuickAbortIsReasonable(StoreEntry * entry) return false; } + if (shutting_down) { + debugs(90, 3, "quick-abort? YES avoid heavy optional work during shutdown"); + return true; + } + MemObject * const mem = entry->mem_obj; assert(mem); debugs(90, 3, "mem=" << mem); diff --git a/src/store_swapout.cc b/src/store_swapout.cc index 9d4de299b9..84c3ea6f9e 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -160,10 +160,6 @@ doPages(StoreEntry *anEntry) void StoreEntry::swapOut() { - // Store::Root() in many swapout checks is FATALly missing during shutdown - if (shutting_down) - return; - if (!mem_obj) return; @@ -363,6 +359,12 @@ StoreEntry::mayStartSwapOut() return false; } + if (shutting_down) { + debugs(20, 3, "avoid heavy optional work during shutdown"); + swapOutDecision(MemObject::SwapOut::swImpossible); + return false; + } + // if there is a usable disk entry already, do not start over if (hasDisk() || Store::Root().hasReadableDiskEntry(*this)) { debugs(20, 3, "already did"); // we or somebody else created that entry diff --git a/src/tests/testRock.cc b/src/tests/testRock.cc index 3fff1e930d..5780fa6011 100644 --- a/src/tests/testRock.cc +++ b/src/tests/testRock.cc @@ -124,8 +124,6 @@ testRock::commonInit() if (inited) return; - StoreFileSystem::SetupAllFs(); - Config.Store.avgObjectSize = 1024; Config.Store.objectsPerBucket = 20; Config.Store.maxObjectSize = 2048; -- 2.39.5