Bug 3819: "fd >= 0" assertion in file_write() during reconfiguration

Other possible assertions include "NumberOfUFSDirs" in UFSSwapDir and
"fd >= 0" in file_close(). And Fs::Ufs::UFSStoreState::drainWriteQueue()
may segfault while dereferencing nil theFile, although I am not sure
that bug is caused specifically by reconfiguration.

Since trunk r9181.3.1, reconfiguration is done in at least two steps:
First, mainReconfigureStart() closes/cleans/disables all modules
supporting hot reconfiguration and then, at the end of the event loop
iteration, mainReconfigureFinish() opens/configures/enables them again.
UFS code hits assertions if it has to log entries or rebuild swap.state
between those two steps. The tiny assertion opportunity window hides
these bugs from most proxies that are not doing enough disk I/O or are
not being reconfigured frequently enough.

Asynchronous UFS cache_dirs such as diskd were the most exposed, but
even blocking UFS caching code could probably hit [rebuild] assertions.

The swap.state rebuilding (always initiated at startup) probably did not
work as intended if reconfiguration happened during the rebuild time
because reconfiguration closed the swap.state file being rebuilt. We now
protect that swap.state file and delay rebuilding progress until
reconfiguration is over.

TODO: To squash more similar bugs, we need to move hot reconfiguration
support into the modules so that each module can reconfigure instantly.

diff --git a/src/fs/ufs/RebuildState.cc b/src/fs/ufs/RebuildState.cc
index b69de3740e4922ff1818bcd7a7411957ea789455..918434aafa403ba4fb55da2edb2aa7fc01fa5b58 100644 (file)
--- a/src/fs/ufs/RebuildState.cc
+++ b/src/fs/ufs/RebuildState.cc
@@ -91,9 +91,11 @@ void
 Fs::Ufs::RebuildState::RebuildStep(void *data)
 {
     RebuildState *rb = (RebuildState *)data;
-    rb->rebuildStep();
+    if (!reconfiguring)
+        rb->rebuildStep();
 
-    if (!rb->isDone())
+    // delay storeRebuildComplete() when reconfiguring to protect storeCleanup()
+    if (!rb->isDone() || reconfiguring)
         eventAdd("storeRebuild", RebuildStep, rb, 0.01, 1);
     else {
         -- StoreController::store_dirs_rebuilding;

diff --git a/src/fs/ufs/UFSStoreState.cc b/src/fs/ufs/UFSStoreState.cc
index cf8a7da833552d2476d342e83c71e1330ed55609..1f31de2d89e1cd28eef63152181b0f064d8a9621 100644 (file)
--- a/src/fs/ufs/UFSStoreState.cc
+++ b/src/fs/ufs/UFSStoreState.cc
@@ -405,7 +405,7 @@ Fs::Ufs::UFSStoreState::drainWriteQueue()
     if (flags.write_draining)
         return;
 
-    if (!theFile->canWrite())
+    if (!theFile || !theFile->canWrite())
         return;
 
     flags.write_draining = true;

diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc
index 260c67a4a5772b342c868e36abdacaa2a9dd96df..511a8f3831e3a3d43b7a5ee89e765c07d0cbeb6c 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.cc
+++ b/src/fs/ufs/UFSSwapDir.cc
@@ -317,7 +317,8 @@ Fs::Ufs::UFSSwapDir::UFSSwapDir(char const *aType, const char *anIOType) :
     currentIOOptions(new ConfigOptionVector()),
     ioType(xstrdup(anIOType)),
     cur_size(0),
-    n_disk_objects(0)
+    n_disk_objects(0),
+    rebuilding_(false)
 {
     /* modulename is only set to disk modules that are built, by configure,
      * so the Find call should never return NULL here.
@@ -725,6 +726,15 @@ Fs::Ufs::UFSSwapDir::logFile(char const *ext) const
 void
 Fs::Ufs::UFSSwapDir::openLog()
 {
+    assert(NumberOfUFSDirs || !UFSDirToGlobalDirMapping);
+    ++NumberOfUFSDirs;
+    assert(NumberOfUFSDirs <= Config.cacheSwap.n_configured);
+
+    if (rebuilding_) { // we did not close the temporary log used for rebuilding
+        assert(swaplog_fd >= 0);
+        return;
+    }
+
     char *logPath;
     logPath = logFile();
     swaplog_fd = file_open(logPath, O_WRONLY | O_CREAT | O_BINARY);
@@ -736,13 +746,6 @@ Fs::Ufs::UFSSwapDir::openLog()
     }
 
     debugs(50, 3, HERE << "Cache Dir #" << index << " log opened on FD " << swaplog_fd);
-
-    if (0 == NumberOfUFSDirs)
-        assert(NULL == UFSDirToGlobalDirMapping);
-
-    ++NumberOfUFSDirs;
-
-    assert(NumberOfUFSDirs <= Config.cacheSwap.n_configured);
 }
 
 void
@@ -751,18 +754,19 @@ Fs::Ufs::UFSSwapDir::closeLog()
     if (swaplog_fd < 0) /* not open */
         return;
 
+    --NumberOfUFSDirs;
+    assert(NumberOfUFSDirs >= 0);
+    if (!NumberOfUFSDirs)
+        safe_free(UFSDirToGlobalDirMapping);
+
+    if (rebuilding_) // we cannot close the temporary log used for rebuilding
+        return;
+
     file_close(swaplog_fd);
 
     debugs(47, 3, "Cache Dir #" << index << " log closed on FD " << swaplog_fd);
 
     swaplog_fd = -1;
-
-    --NumberOfUFSDirs;
-
-    assert(NumberOfUFSDirs >= 0);
-
-    if (0 == NumberOfUFSDirs)
-        safe_free(UFSDirToGlobalDirMapping);
 }
 
 bool
@@ -842,6 +846,9 @@ Fs::Ufs::UFSSwapDir::rebuild()
 void
 Fs::Ufs::UFSSwapDir::closeTmpSwapLog()
 {
+    assert(rebuilding_);
+    rebuilding_ = false;
+
     char *swaplog_path = xstrdup(logFile(NULL)); // where the swaplog should be
     char *tmp_path = xstrdup(logFile(".new")); // the temporary file we have generated
     int fd;
@@ -868,6 +875,8 @@ Fs::Ufs::UFSSwapDir::closeTmpSwapLog()
 FILE *
 Fs::Ufs::UFSSwapDir::openTmpSwapLog(int *clean_flag, int *zero_flag)
 {
+    assert(!rebuilding_);
+
     char *swaplog_path = xstrdup(logFile(NULL));
     char *clean_path = xstrdup(logFile(".last-clean"));
     char *new_path = xstrdup(logFile(".new"));
@@ -902,6 +911,7 @@ Fs::Ufs::UFSSwapDir::openTmpSwapLog(int *clean_flag, int *zero_flag)
     }
 
     swaplog_fd = fd;
+    rebuilding_ = true;
 
     {
         const StoreSwapLogHeader header;
@@ -1060,18 +1070,17 @@ Fs::Ufs::UFSSwapDir::writeCleanDone()
     cleanLog = NULL;
 }
 
-void
-Fs::Ufs::UFSSwapDir::CleanEvent(void *)
+/// safely cleans a few unused files if possible
+int
+Fs::Ufs::UFSSwapDir::HandleCleanEvent()
 {
     static int swap_index = 0;
     int i;
     int j = 0;
     int n = 0;
-    /*
-     * Assert that there are UFS cache_dirs configured, otherwise
-     * we should never be called.
-     */
-    assert(NumberOfUFSDirs);
+
+    if (!NumberOfUFSDirs)
+        return 0; // probably in the middle of reconfiguration
 
     if (NULL == UFSDirToGlobalDirMapping) {
         SwapDir *sd;
@@ -1115,6 +1124,13 @@ Fs::Ufs::UFSSwapDir::CleanEvent(void *)
         ++swap_index;
     }
 
+    return n;
+}
+
+void
+Fs::Ufs::UFSSwapDir::CleanEvent(void *)
+{
+    const int n = HandleCleanEvent();
     eventAdd("storeDirClean", CleanEvent, NULL,
              15.0 * exp(-0.25 * n), 1);
 }
@@ -1284,6 +1300,11 @@ Fs::Ufs::UFSSwapDir::swappedOut(const StoreEntry &e)
 void
 Fs::Ufs::UFSSwapDir::logEntry(const StoreEntry & e, int op) const
 {
+    if (swaplog_fd < 0) {
+        debugs(36, 5, "cannot log " << e << " in the middle of reconfiguration");
+        return;
+    }
+
     StoreSwapLogData *s = new StoreSwapLogData;
     s->op = (char) op;
     s->swap_filen = e.swap_filen;

diff --git a/src/fs/ufs/UFSSwapDir.h b/src/fs/ufs/UFSSwapDir.h
index 72b3788b8639eecfdc9efaf6b944199d769f2f61..4cfb48b8d909f54102fad7e442f8890fbd9575fb 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.h
+++ b/src/fs/ufs/UFSSwapDir.h
@@ -126,6 +126,7 @@ private:
     bool pathIsDirectory(const char *path)const;
     int swaplog_fd;
     static EVH CleanEvent;
+    static int HandleCleanEvent();
     /** Verify that the the CacheDir exists
      *
      * If this returns < 0, then Squid exits, complains about swap
@@ -145,6 +146,7 @@ private:
     char const *ioType;
     uint64_t cur_size; ///< currently used space in the storage area
     uint64_t n_disk_objects; ///< total number of objects stored
+    bool rebuilding_; ///< whether RebuildState is writing the new swap.state
 };
 
 } //namespace Ufs

diff --git a/src/store/Disks.cc b/src/store/Disks.cc
index 790c11e00290232faa50e3f1ac108e964608aad8..8bd5cf5768228d0ad42522d4d6072294fbb55e5d 100644 (file)
--- a/src/store/Disks.cc
+++ b/src/store/Disks.cc
@@ -582,7 +582,7 @@ storeDirWriteCleanLogs(int reopen)
 
     // Check for store_dirs_rebuilding because fatal() often calls us in early
     // initialization phases, before store log is initialized and ready. Also,
-    // some stores probably do not support log cleanup during Store rebuilding.
+    // some stores do not support log cleanup during Store rebuilding.
     if (StoreController::store_dirs_rebuilding) {
         debugs(20, DBG_IMPORTANT, "Not currently OK to rewrite swap log.");
         debugs(20, DBG_IMPORTANT, "storeDirWriteCleanLogs: Operation aborted.");