diff --git a/src/Store.h b/src/Store.h index 552695d..84b384a 100644 --- 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 6931def..daf626d 100644 --- 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 3efefe1..9651252 100644 --- 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 0a4748c..699dbf8 100644 --- 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 c372b63..49f9d32 100644 --- 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 4f3d7f6..0cff0d0 100644 --- 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 ad71993..5761198 100644 --- 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(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 " <<