Bug 4823: assertion failed: "lowestOffset () <= target_offset" (#370)

This assertion could be triggered by various swapout failures for
ufs/aufs/diskd cache_dir entries.

The bug was caused by 4310f8b change related to storeSwapOutFileClosed()
method. Before that change, swapout failures resulted in
StoreEntry::swap_status set to SWAPOUT_NONE, preventing
another/asserting iteration of StoreEntry::swapOut().

This fix adds SWAPOUT_FAILED swap status for marking swapout failures
(instead of reviving and abusing SWAPOUT_NONE), making the code more
reliable.

Also removed storeSwapOutFileNotify() implementation.  We should not
waste time on maintaining an unused method that now contains conflicting
assertions: swappingOut() and !hasDisk().

diff --git a/src/Store.h b/src/Store.h
index 2c28bc7d78f1a31e0ec327d62b00fb7798718599..a78c9790bc0f94d67dbb793e1c6f439e37502fe7 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -119,6 +119,8 @@ public:
     bool swappingOut() const { return swap_status == SWAPOUT_WRITING; }
     /// whether the entire entry is now on disk (possibly marked for deletion)
     bool swappedOut() const { return swap_status == SWAPOUT_DONE; }
+    /// whether we failed to write this entry to disk
+    bool swapoutFailed() const { return swap_status == SWAPOUT_FAILED; }
     void swapOutFileClose(int how);
     const char *url() const;
     /// Satisfies cachability requirements shared among disk and RAM caches.

diff --git a/src/enums.h b/src/enums.h
index 9018c99d601d5b2d79047738a42c52fac3813f46..a7d3db0663de959ba6d61f1451c6b08520ad5c08 100644 (file)
--- a/src/enums.h
+++ b/src/enums.h
@@ -57,7 +57,11 @@ typedef enum {
     SWAPOUT_WRITING,
     /// StoreEntry is associated with a complete (i.e., fully swapped out) disk store entry.
     /// Guarantees the disk store entry existence.
-    SWAPOUT_DONE
+    SWAPOUT_DONE,
+    /// StoreEntry is associated with an unusable disk store entry.
+    /// Swapout attempt has failed. The entry should be marked for eventual deletion.
+    /// Guarantees the disk store entry existence.
+    SWAPOUT_FAILED
 } swap_status_t;
 
 typedef enum {

diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc
index efc84aa436f3bf8f6506ab4f61b11c277574e534..f42b421c4fe248c19ac83ba0155880b6214b1471 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.cc
+++ b/src/fs/ufs/UFSSwapDir.cc
@@ -1185,6 +1185,8 @@ Fs::Ufs::UFSSwapDir::evictCached(StoreEntry & e)
     if (!e.hasDisk())
         return; // see evictIfFound()
 
+    // Since these fields grow only after swap out ends successfully,
+    // do not decrement them for e.swappingOut() and e.swapoutFailed().
     if (e.swappedOut()) {
         cur_size -= fs.blksize * sizeInBlocks(e.swap_file_sz);
         --n_disk_objects;
@@ -1274,7 +1276,7 @@ void
 Fs::Ufs::UFSSwapDir::finalizeSwapoutFailure(StoreEntry &entry)
 {
     debugs(47, 5, entry);
-    // rely on the expected subsequent StoreEntry::release(), evictCached(), or
+    // rely on the expected eventual StoreEntry::release(), evictCached(), or
     // a similar call to call unlink(), detachFromDisk(), etc. for the entry.
 }
 

diff --git a/src/store.cc b/src/store.cc
index 2189c68a58ddb1afff169399ee2376afcb46e756..9324abf29bed342cb52a1c1d8bc3c8e6f388d525 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -84,7 +84,8 @@ const char *storeStatusStr[] = {
 const char *swapStatusStr[] = {
     "SWAPOUT_NONE",
     "SWAPOUT_WRITING",
-    "SWAPOUT_DONE"
+    "SWAPOUT_DONE",
+    "SWAPOUT_FAILED"
 };
 
 /*
@@ -258,6 +259,8 @@ StoreEntry::setNoDelay(bool const newValue)
 // XXX: Type names mislead. STORE_DISK_CLIENT actually means that we should
 //      open swapin file, aggressively trim memory, and ignore read-ahead gap.
 //      It does not mean we will read from disk exclusively (or at all!).
+//      STORE_MEM_CLIENT covers all other cases, including in-memory entries,
+//      newly created entries, and entries not backed by disk or memory cache.
 // XXX: May create STORE_DISK_CLIENT with no disk caching configured.
 // XXX: Collapsed clients cannot predict their type.
 store_client_t
@@ -280,6 +283,9 @@ StoreEntry::storeClientType() const
         return STORE_MEM_CLIENT;
     }
 
+    if (swapoutFailed())
+        return STORE_MEM_CLIENT;
+
     if (store_status == STORE_OK) {
         /* the object has completed. */
 
@@ -2029,13 +2035,23 @@ StoreEntry::detachFromDisk()
 void
 StoreEntry::checkDisk() const
 {
-    const bool ok = (swap_dirn < 0) == (swap_filen < 0) &&
-                    (swap_dirn < 0) == (swap_status == SWAPOUT_NONE) &&
-                    (swap_dirn < 0 || swap_dirn < Config.cacheSwap.n_configured);
-
-    if (!ok) {
-        debugs(88, DBG_IMPORTANT, "ERROR: inconsistent disk entry state " << *this);
-        throw std::runtime_error("inconsistent disk entry state ");
+    try {
+        if (swap_dirn < 0) {
+            Must(swap_filen < 0);
+            Must(swap_status == SWAPOUT_NONE);
+        } else {
+            Must(swap_filen >= 0);
+            Must(swap_dirn < Config.cacheSwap.n_configured);
+            if (swapoutFailed()) {
+                Must(EBIT_TEST(flags, RELEASE_REQUEST));
+            } else {
+                Must(swappingOut() || swappedOut());
+            }
+        }
+    } catch (...) {
+        debugs(88, DBG_IMPORTANT, "ERROR: inconsistent disk entry state " <<
+               *this << "; problem: " << CurrentException);
+        throw;
     }
 }
 

diff --git a/src/store_client.cc b/src/store_client.cc
index 07f7f480c92019494f12122ee6ca6710af3e9bca..ae0e8a0509ef869beac8b3030cd7a03657664221 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -210,7 +210,7 @@ store_client::store_client(StoreEntry *e) :
     if (getType() == STORE_DISK_CLIENT) {
         /* assert we'll be able to get the data we want */
         /* maybe we should open swapin_sio here */
-        assert(entry->hasDisk() || entry->swappingOut());
+        assert(entry->hasDisk() && !entry->swapoutFailed());
     }
 }
 
@@ -710,7 +710,8 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
     dlinkDelete(&sc->node, &mem->clients);
     -- mem->nclients;
 
-    if (e->store_status == STORE_OK && !e->swappedOut())
+    const auto swapoutFinished = e->swappedOut() || e->swapoutFailed();
+    if (e->store_status == STORE_OK && !swapoutFinished)
         e->swapOut();
 
     if (sc->swapin_sio != NULL) {

diff --git a/src/store_swapin.cc b/src/store_swapin.cc
index 4f3d7f6d1a9bef65bbced047181bc72d2af4d13d..0cff0d0a6aaaee0ec70d447be50eaa0e8643e0f3 100644 (file)
--- a/src/store_swapin.cc
+++ b/src/store_swapin.cc
@@ -38,6 +38,11 @@ storeSwapInStart(store_client * sc)
         return;
     }
 
+    if (e->swapoutFailed()) {
+        debugs(20, DBG_IMPORTANT, "BUG: Attempt to swap in a failed-to-store entry " << *e << ". Salvaged.");
+        return;
+    }
+
     assert(e->mem_obj != NULL);
     sc->swapin_sio = storeOpen(e, storeSwapInFileNotify, storeSwapInFileClosed, sc);
 }

diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index ad7199366e1625f90460a807bcde57b00afbd59d..57611984ac1f47f889526d6cc88930771fdefc52 100644 (file)
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -88,19 +88,9 @@ storeSwapOutStart(StoreEntry * e)
 
 /// XXX: unused, see a related StoreIOState::file_callback
 static void
-storeSwapOutFileNotify(void *data, int errflag, StoreIOState::Pointer self)
+storeSwapOutFileNotify(void *, int, StoreIOState::Pointer)
 {
-    StoreEntry *e;
-    static_cast<generic_cbdata *>(data)->unwrap(&e);
-
-    MemObject *mem = e->mem_obj;
-    assert(e->swappingOut());
-    assert(mem);
-    assert(mem->swapout.sio == self);
-    assert(errflag == 0);
-    assert(!e->hasDisk()); // if this fails, call SwapDir::disconnect(e)
-    e->swap_filen = mem->swapout.sio->swap_filen;
-    e->swap_dirn = mem->swapout.sio->swap_dirn;
+    assert(false);
 }
 
 static bool
@@ -304,8 +294,11 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
             storeConfigure();
         }
 
+        // mark the locked entry for deletion
+        // TODO: Keep the memory entry (if any)
+        e->releaseRequest();
+        e->swap_status = SWAPOUT_FAILED;
         e->disk().finalizeSwapoutFailure(*e);
-        e->releaseRequest(); // TODO: Keep the memory entry (if any)
     } else {
         /* swapping complete */
         debugs(20, 3, "storeSwapOutFileClosed: SwapOut complete: '" << e->url() << "' to " <<