Bug 3752: objects that cannot be cached in memory are not cached on disk if cache_dir...

This fix contains four related changes:

1) When fixing "trimMemory for unswappable objects" (trunk r11969), we
replaced swapoutPossible() with swappingOut()||mayStartSwapOut() but missed
the fact that swapoutPossible() had "possible now" semantics while
mayStartSwapOut() has "may start now or in the future" semantics. When all
cache_dirs had max-size set, mayStartSwapOut() returned false for objects of
unknown size and even for smaller-than-maximum but not-yet-received objects,
despite the fact that those objects could be swapped out later.

That false mayStartSwapOut() result allowed maybeTrimMemory() to trim those
objects memory and mark the objects for release, preventing their subsequent
disk caching.

2) To fix (1) above, mayStartSwapOut() had to return true for not-yet-received
objects of unknown size. However, returning true is correct only if no
subsequent check can return false. Thus, we had to move all lower/later checks
that could return false up, placing them before the maximum-of-all-max-sizes
check.

3) Once (2) was done, the end of mayStartSwapOut() had (a) a loop that could
return true while setting decision to MemObject::SwapOut::swPossible and (b)
an unconditional code that did ... the same thing. Thus, the loop could no
longer change the method outcome. The loop also had a lot of doubts and XXXs
attached to it. We removed it. If that loop is needed, it is needed and must
be resurrected elsewhere.

4) Since mayStartSwapOut() returns true if swapout is possible in the future
(but not necessarily now), we cannot rely on its return value to initiate
swapout code. We need to test whether swapout.decision is swPossible instead.

diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index a8d7bcdbb0e6ef08c996b45ea1a8583b5d6799b5..2f62e1b5f7fdfd932a67ef9c443b43c2bdcf91a1 100644 (file)
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -200,7 +200,7 @@ StoreEntry::swapOut()
 
     Store::Root().maybeTrimMemory(*this, weAreOrMayBeSwappingOut);
 
-    if (!weAreOrMayBeSwappingOut)
+    if (mem_obj->swapout.decision != MemObject::SwapOut::swPossible)
         return; // nothing else to do
 
     // Aborted entries have STORE_OK, but swapoutPossible rejects them. Thus,
@@ -360,8 +360,6 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
 bool
 StoreEntry::mayStartSwapOut()
 {
-    dlink_node *node;
-
     // must be checked in the caller
     assert(!EBIT_TEST(flags, ENTRY_ABORTED));
     assert(!swappingOut());
@@ -403,6 +401,18 @@ StoreEntry::mayStartSwapOut()
         return false;
     }
 
+    if (mem_obj->inmem_lo > 0) {
+        debugs(20, 3, "storeSwapOut: (inmem_lo > 0)  imem_lo:" <<  mem_obj->inmem_lo);
+        decision = MemObject::SwapOut::swImpossible;
+        return false;
+    }
+
+    if (!mem_obj->isContiguous()) {
+        debugs(20, 3, "storeSwapOut: not Contiguous");
+        decision = MemObject::SwapOut::swImpossible;
+        return false;
+    }
+
     // check cache_dir max-size limit if all cache_dirs have it
     if (store_maxobjsize >= 0) {
         // TODO: add estimated store metadata size to be conservative
@@ -426,69 +436,24 @@ StoreEntry::mayStartSwapOut()
             return false; // already does not fit and may only get bigger
         }
 
-        // prevent default swPossible answer for yet unknown length
-        if (expectedEnd < 0) {
-            debugs(20, 3,  HERE << "wait for more info: " <<
-                   store_maxobjsize);
-            return false; // may fit later, but will be rejected now
-        }
-
-        if (store_status != STORE_OK) {
-            const int64_t maxKnownSize = expectedEnd < 0 ?
-                                         mem_obj->availableForSwapOut() : expectedEnd;
+        // prevent final default swPossible answer for yet unknown length
+        if (expectedEnd < 0 && store_status != STORE_OK) {
+            const int64_t maxKnownSize = mem_obj->availableForSwapOut();
             debugs(20, 7, HERE << "maxKnownSize= " << maxKnownSize);
-            if (maxKnownSize < store_maxobjsize) {
-                /*
-                 * NOTE: the store_maxobjsize here is the max of optional
-                 * max-size values from 'cache_dir' lines.  It is not the
-                 * same as 'maximum_object_size'.  By default, store_maxobjsize
-                 * will be set to -1.  However, I am worried that this
-                 * deferance may consume a lot of memory in some cases.
-                 * Should we add an option to limit this memory consumption?
-                 */
-                debugs(20, 5,  HERE << "Deferring swapout start for " <<
-                       (store_maxobjsize - maxKnownSize) << " bytes");
-                return false;
-            }
-        }
-    }
-
-    if (mem_obj->inmem_lo > 0) {
-        debugs(20, 3, "storeSwapOut: (inmem_lo > 0)  imem_lo:" <<  mem_obj->inmem_lo);
-        decision = MemObject::SwapOut::swImpossible;
-        return false;
-    }
-
-    /*
-     * If there are DISK clients, we must write to disk
-     * even if its not cachable
-     * RBC: Surely we should not create disk client on non cacheable objects?
-     * therefore this should be an assert?
-     * RBC 20030708: We can use disk to avoid mem races, so this shouldn't be
-     * an assert.
-     *
-     * XXX: Not clear what "mem races" the above refers to, especially when
-     * dealing with non-cachable objects that cannot have multiple clients.
-     *
-     * XXX: If STORE_DISK_CLIENT needs SwapOut::swPossible, we have to check
-     * for that flag earlier, but forcing swapping may contradict max-size or
-     * other swapability restrictions. Change storeClientType() and/or its
-     * callers to take swap-in availability into account.
-     */
-    for (node = mem_obj->clients.head; node; node = node->next) {
-        if (((store_client *) node->data)->getType() == STORE_DISK_CLIENT) {
-            debugs(20, 3, HERE << "DISK client found");
-            decision = MemObject::SwapOut::swPossible;
-            return true;
+            /*
+             * NOTE: the store_maxobjsize here is the max of optional
+             * max-size values from 'cache_dir' lines.  It is not the
+             * same as 'maximum_object_size'.  By default, store_maxobjsize
+             * will be set to -1.  However, I am worried that this
+             * deferance may consume a lot of memory in some cases.
+             * Should we add an option to limit this memory consumption?
+             */
+            debugs(20, 5,  HERE << "Deferring swapout start for " <<
+                   (store_maxobjsize - maxKnownSize) << " bytes");
+            return true; // may still fit, but no final decision yet
         }
     }
 
-    if (!mem_obj->isContiguous()) {
-        debugs(20, 3, "storeSwapOut: not Contiguous");
-        decision = MemObject::SwapOut::swImpossible;
-        return false;
-    }
-
     decision = MemObject::SwapOut::swPossible;
     return true;
 }