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 d53834c71b5f4592f16c5af556a626dd20063eaf..9f87af2114b47d44fa7ee702ba2ed47d2ac1503e 100644 (file)
--- 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 513e73668bb8420474e5246177aea0f5bbd111aa..fc2f0b270824bfa223a1ceb929bff4382f0fa7ca 100644 (file)
--- 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 d62dd727b03ad971d9764e9f55ccf5d59b79ecb2..18f661dfdf7360eac4f3d4d153ec8c96c7fe793a 100644 (file)
--- 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 e473a326806d7c17d407f5b82b7c270b4704c760..a2d613bf9eaa20757a7b5022c013411b0effee2d 100644 (file)
--- 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);