From: Alex Rousskov Date: Wed, 13 Apr 2011 05:19:00 +0000 (-0600) Subject: Fixed how storeSwapOutStart() prevents repeated calls on failures. X-Git-Tag: take06~36 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ddc9b32c1c23a52a73956e1ae7c5b8ebd8798628;p=thirdparty%2Fsquid.git Fixed how storeSwapOutStart() prevents repeated calls on failures. We used to release the entry to signal that swapout is not possible. That hack worked for disk caching, but it prevents nearly all memory caching because released entries cannot be cached in memory. A polished solution is to explicitly remember whether we made the decision to allow or reject a swapout. The decision is now stored in MemObject::SwapOut. Call StoreEntry::checkCachable() from StoreEntry::swapoutPossible(). This allows us to make the decision sooner in some cases. Needs more work because some checks in the two functions overlap and "too many files" checks in checkCachable() should not be there at all. Added an XXX for the checkCachable() call at the end of swapout. Out of this project scope. --- diff --git a/src/MemObject.cc b/src/MemObject.cc index d53834c71b..9f87af2114 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -99,6 +99,8 @@ MemObject::MemObject(char const *aUrl, char const *aLog_url) object_sz = -1; /* XXX account log_url */ + + swapout.decision = SwapOut::swNeedsCheck; } MemObject::~MemObject() diff --git a/src/MemObject.h b/src/MemObject.h index 513e73668b..fc2f0b2708 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -114,6 +114,10 @@ public: public: int64_t queue_offset; ///< number of bytes sent to SwapDir for writing StoreIOState::Pointer sio; + + /// Decision states for StoreEntry::swapoutPossible() and related code. + typedef enum { swNeedsCheck = 0, swImpossible = -1, swPossible = +1 } Decision; + Decision decision; ///< current decision state }; SwapOut swapout; diff --git a/src/store.cc b/src/store.cc index d62dd727b0..18f661dfdf 100644 --- a/src/store.cc +++ b/src/store.cc @@ -963,6 +963,8 @@ StoreEntry::checkTooSmall() return 0; } +// TODO: remove checks already performed by swapoutPossible() +// TODO: move "too many open..." checks outside -- we are called too early/late int StoreEntry::checkCachable() { @@ -1866,32 +1868,57 @@ StoreEntry::getSerialisedMetaData() bool StoreEntry::swapoutPossible() { + if (!Config.cacheSwap.n_configured) + return false; + /* should we swap something out to disk? */ debugs(20, 7, "storeSwapOut: " << url()); debugs(20, 7, "storeSwapOut: store_status = " << storeStatusStr[store_status]); + assert(mem_obj); + MemObject::SwapOut::Decision &decision = mem_obj->swapout.decision; + + // if we decided that swapout is not possible, do not repeat same checks + if (decision == MemObject::SwapOut::swImpossible) { + debugs(20, 3, "storeSwapOut: already rejected"); + return false; + } + + // this flag may change so we must check it even if we already said "yes" if (EBIT_TEST(flags, ENTRY_ABORTED)) { assert(EBIT_TEST(flags, RELEASE_REQUEST)); // StoreEntry::abort() already closed the swap out file, if any + decision = MemObject::SwapOut::swImpossible; return false; } // if we decided that swapout is possible, do not repeat same checks - // TODO: do not repeat any checks if we decided that swapout is impossible + if (decision == MemObject::SwapOut::swPossible) { + debugs(20, 3, "storeSwapOut: already allowed"); + return true; + } + + // if we are swapping out already, do not repeat same checks if (swap_status != SWAPOUT_NONE) { debugs(20, 3, "storeSwapOut: already started"); + decision = MemObject::SwapOut::swPossible; return true; } + if (!checkCachable()) { + debugs(20, 3, "storeSwapOut: not cachable"); + decision = MemObject::SwapOut::swImpossible; + return false; + } + if (EBIT_TEST(flags, ENTRY_SPECIAL)) { debugs(20, 3, "storeSwapOut: " << url() << " SPECIAL"); + decision = MemObject::SwapOut::swImpossible; return false; } // check cache_dir max-size limit if all cache_dirs have it if (store_maxobjsize >= 0) { - assert(mem_obj); - // TODO: add estimated store metadata size to be conservative // use guaranteed maximum if it is known @@ -1900,6 +1927,7 @@ StoreEntry::swapoutPossible() if (expectedEnd > store_maxobjsize) { debugs(20, 3, "storeSwapOut: will not fit: " << expectedEnd << " > " << store_maxobjsize); + decision = MemObject::SwapOut::swImpossible; return false; // known to outgrow the limit eventually } if (expectedEnd < 0) { @@ -1913,10 +1941,12 @@ StoreEntry::swapoutPossible() if (currentEnd > store_maxobjsize) { debugs(20, 3, "storeSwapOut: does not fit: " << currentEnd << " > " << store_maxobjsize); + decision = MemObject::SwapOut::swImpossible; return false; // already does not fit and may only get bigger } } + decision = MemObject::SwapOut::swPossible; return true; } diff --git a/src/store_swapout.cc b/src/store_swapout.cc index e473a32680..a2d613bf9e 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -85,9 +85,7 @@ storeSwapOutStart(StoreEntry * e) if (sio == NULL) { e->swap_status = SWAPOUT_NONE; - // our caller thinks SWAPOUT_NONE means swapping out has not started - // yet so we better release here to avoid being called again and again - e->releaseRequest(); + mem->swapout.decision = MemObject::SwapOut::swImpossible; delete c; xfree((char*)buf); storeLog(STORE_LOG_SWAPOUTFAIL, e); @@ -279,13 +277,7 @@ StoreEntry::swapOut() if (swap_status == SWAPOUT_NONE) { assert(mem_obj->swapout.sio == NULL); assert(mem_obj->inmem_lo == 0); - - if (checkCachable()) - storeSwapOutStart(this); - else - return; - - /* ENTRY_CACHABLE will be cleared and we'll never get here again */ + storeSwapOutStart(this); // sets SwapOut::swImpossible on failures } if (mem_obj->swapout.sio == NULL) @@ -364,6 +356,10 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self) e->swap_status = SWAPOUT_DONE; e->store()->updateSize(e->swap_file_sz, 1); + // XXX: For some Stores, it is pointless to re-check cachability here + // and it leads to double counts in store_check_cachable_hist. We need + // another way to signal a completed but failed swapout. Or, better, + // each Store should handle its own logging and LOG state setting. if (e->checkCachable()) { storeLog(STORE_LOG_SWAPOUT, e); storeDirSwapLog(e, SWAP_LOG_ADD);