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

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 552695dd5cb58f248d870b4912cd8d022ac51863..84b384ae1a23911048b71333570f1d7fff016134 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 6931def8ca8dc8f65b4da1a900a0858f6a8699e8..daf626d9825b995eef41b2bd73473ea47b97f6ac 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 3efefe1bc349bfc622ecbd5a9ff32babff4c12d4..9651252b4150cd5b0454fee6638224e0db5fe925 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.cc
+++ b/src/fs/ufs/UFSSwapDir.cc
@@ -1181,6 +1181,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;
@@ -1270,7 +1272,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 0a4748c6d1226d0f4f58cb9fb4b6e0bb6d18d658..699dbf817a9e9cb44662bf2aa08ba18c4cb96930 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -83,7 +83,8 @@ const char *storeStatusStr[] = {
 const char *swapStatusStr[] = {
     "SWAPOUT_NONE",
     "SWAPOUT_WRITING",
-    "SWAPOUT_DONE"
+    "SWAPOUT_DONE",
+    "SWAPOUT_FAILED"
 };
 
 /*
@@ -257,6 +258,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
@@ -279,6 +282,9 @@ StoreEntry::storeClientType() const
         return STORE_MEM_CLIENT;
     }
 
+    if (swapoutFailed())
+        return STORE_MEM_CLIENT;
+
     if (store_status == STORE_OK) {
         /* the object has completed. */
 
@@ -2044,13 +2050,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 c372b63fab0c83ab03a0f792ecdcc2857c532e10..49f9d329a5737f493e031bda9466a94cf07a4299 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -162,7 +162,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());
     }
 }
 
@@ -662,7 +662,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 " <<