]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
trimMemory for unswappable objects
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Fri, 13 Jan 2012 13:49:26 +0000 (15:49 +0200)
committerChristos Tsantilas <chtsanti@users.sourceforge.net>
Fri, 13 Jan 2012 13:49:26 +0000 (15:49 +0200)
Inside StoreEntry::swapOut() the StoreEntry::trimMemory() method called to
release unused MemObjects memory. The trimMemory method must called for all
store entries, but current code blocks that call for not-swappable objects,
at least.

This patch trying to fix this bug implementing the following simple logic:
   {
     bool weAreOrMayBeSwappingOut =
           swappingOut() || mayStartSwapout();

     trimMemory(weAreOrMayBeSwappingOut);

     if (!weAreOrMayBeSwappingOut)
         return; // nothing else to do
   }

This is a Measurement Factory project

src/MemObject.cc
src/MemObject.h
src/Store.h
src/store.cc
src/store_client.cc
src/store_swapout.cc
src/tests/stub_MemObject.cc
src/tests/stub_store.cc

index 08e8ead3cbf42e7961a9fa7a2bbcb176f81d3cb6..373a3db0c1d17eb1947303636b38d1d941314ec5 100644 (file)
@@ -492,3 +492,9 @@ MemObject::mostBytesAllowed() const
 }
 
 #endif
+
+int64_t
+MemObject::availableForSwapOut() const
+{
+    return endOffset() - swapout.queue_offset;
+}
index f02ace552123d702abcd70e52e953e6857b9f680..fab4f0d517a5cad67d115bb2bd83726d61619c11 100644 (file)
@@ -81,6 +81,7 @@ public:
      */
     int64_t objectBytesOnDisk() const;
     int64_t policyLowestOffsetToKeep(bool swap) const;
+    int64_t availableForSwapOut() const; ///< buffered bytes we have not swapped out yet
     void trimSwappable();
     void trimUnSwappable();
     bool isContiguous() const;
index 78481647e5b1d1f0cf6857018532b4c356698b02..21959fbae399f4dcd74737279cab20058b45c639 100644 (file)
@@ -92,8 +92,9 @@ public:
     virtual char const *getSerialisedMetaData();
     void replaceHttpReply(HttpReply *, bool andStartWriting = true);
     void startWriting(); ///< pack and write reply headers and, maybe, body
-    virtual bool swapoutPossible();
-    virtual void trimMemory();
+    /// whether we may start writing to disk (now or in the future)
+    virtual bool mayStartSwapOut();
+    virtual void trimMemory(const bool preserveSwappable);
     void abort();
     void unlink();
     void makePublic();
@@ -108,7 +109,8 @@ public:
     void purgeMem();
     void cacheInMemory(); ///< start or continue storing in memory cache
     void swapOut();
-    bool swapOutAble() const;
+    /// whether we are in the process of writing this entry to disk
+    bool swappingOut() const { return swap_status == SWAPOUT_WRITING; }
     void swapOutFileClose(int how);
     const char *url() const;
     int checkCachable();
@@ -247,7 +249,7 @@ private:
     store_client_t storeClientType() const {return STORE_MEM_CLIENT;}
 
     char const *getSerialisedMetaData();
-    bool swapoutPossible() {return false;}
+    bool mayStartSwapout() {return false;}
 
     void trimMemory() {}
 
index 9e9cb3b3bb3e267b1975b00a5173e6aa46c872b7..7198db54f038f9db95ae5a47cfef84a783311383 100644 (file)
@@ -1891,95 +1891,8 @@ StoreEntry::getSerialisedMetaData()
     return result;
 }
 
-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
-    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) {
-        // TODO: add estimated store metadata size to be conservative
-
-        // use guaranteed maximum if it is known
-        const int64_t expectedEnd = mem_obj->expectedReplySize();
-        debugs(20, 7, "storeSwapOut: expectedEnd = " << expectedEnd);
-        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
-        }
-
-        // use current minimum (always known)
-        const int64_t currentEnd = mem_obj->endOffset();
-        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
-        }
-
-        // prevent default swPossible answer for yet unknown length
-        if (expectedEnd < 0) {
-            debugs(20, 3, "storeSwapOut: wait for more info: " <<
-                   store_maxobjsize);
-            return false; // may fit later, but will be rejected now
-        }
-    }
-
-    decision = MemObject::SwapOut::swPossible;
-    return true;
-}
-
 void
-StoreEntry::trimMemory()
+StoreEntry::trimMemory(const bool preserveSwappable)
 {
     /*
      * DPW 2007-05-09
@@ -1989,7 +1902,7 @@ StoreEntry::trimMemory()
     if (mem_status == IN_MEMORY)
         return;
 
-    if (!swapOutAble()) {
+    if (!preserveSwappable) {
         if (mem_obj->policyLowestOffsetToKeep(0) == 0) {
             /* Nothing to do */
             return;
index f8bd81bfdfc0176ea9144ec63068a077fc452226..fd8003b3e5ada344147e6f8722a4803a8335ba84 100644 (file)
@@ -195,7 +195,7 @@ store_client::store_client(StoreEntry *e) : entry (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->swap_filen > -1 || entry->swapOutAble());
+        assert(entry->swap_filen > -1 || entry->swappingOut());
 
 #if STORE_CLIENT_LIST_DEBUG
 
index a11638bd6f2258737acc1dfb01fbd5e1c6c7c9ab..7a78acf86d357341e55a121ab7e5f97e6adee70c 100644 (file)
@@ -188,8 +188,20 @@ StoreEntry::swapOut()
     if (!mem_obj)
         return;
 
-    if (!swapoutPossible())
+    // this flag may change so we must check even if we are swappingOut
+    if (EBIT_TEST(flags, ENTRY_ABORTED)) {
+        assert(EBIT_TEST(flags, RELEASE_REQUEST));
+        // StoreEntry::abort() already closed the swap out file, if any
+        // no trimming: data producer must stop production if ENTRY_ABORTED
         return;
+    }
+
+    const bool weAreOrMayBeSwappingOut = swappingOut() || mayStartSwapOut();
+    trimMemory(weAreOrMayBeSwappingOut);
+
+    if (!weAreOrMayBeSwappingOut)
+        return; // nothing else to do
 
     // Aborted entries have STORE_OK, but swapoutPossible rejects them. Thus,
     // store_status == STORE_OK below means we got everything we wanted.
@@ -201,39 +213,10 @@ StoreEntry::swapOut()
     if (mem_obj->swapout.sio != NULL)
         debugs(20, 7, "storeSwapOut: storeOffset() = " << mem_obj->swapout.sio->offset()  );
 
-    // buffered bytes we have not swapped out yet
-    int64_t swapout_maxsize = mem_obj->endOffset() - mem_obj->swapout.queue_offset;
-
-    assert(swapout_maxsize >= 0);
-
     int64_t const lowest_offset = mem_obj->lowestMemReaderOffset();
 
     debugs(20, 7, HERE << "storeSwapOut: lowest_offset = " << lowest_offset);
 
-    // Check to see whether we're going to defer the swapout based upon size
-    if (store_status != STORE_OK) {
-        const int64_t expectedSize = mem_obj->expectedReplySize();
-        const int64_t maxKnownSize = expectedSize < 0 ?
-                                     swapout_maxsize : expectedSize;
-        debugs(20, 7, HERE << "storeSwapOut: 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, "storeSwapOut: Deferring swapout start for " <<
-                   (store_maxobjsize - maxKnownSize) << " bytes");
-            return;
-        }
-    }
-
-// TODO: it is better to trim as soon as we swap something out, not before
-    trimMemory();
 #if SIZEOF_OFF_T <= 4
 
     if (mem_obj->endOffset() > 0x7FFF0000) {
@@ -246,9 +229,9 @@ StoreEntry::swapOut()
     if (swap_status == SWAPOUT_WRITING)
         assert(mem_obj->inmem_lo <=  mem_obj->objectBytesOnDisk() );
 
-    if (!swapOutAble())
-        return;
-
+    // buffered bytes we have not swapped out yet
+    const int64_t swapout_maxsize = mem_obj->availableForSwapOut();
+    assert(swapout_maxsize >= 0);
     debugs(20, 7, "storeSwapOut: swapout_size = " << swapout_maxsize);
 
     if (swapout_maxsize == 0) { // swapped everything we got
@@ -374,19 +357,106 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
     e->unlock();
 }
 
-/*
- * Is this entry a candidate for writing to disk?
- */
 bool
-StoreEntry::swapOutAble() const
+StoreEntry::mayStartSwapOut()
 {
     dlink_node *node;
 
-    if (mem_obj->swapout.sio != NULL)
+    // must be checked in the caller
+    assert(!EBIT_TEST(flags, ENTRY_ABORTED));
+
+    if (!Config.cacheSwap.n_configured)
+        return false;
+
+    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, HERE << " already rejected");
+        return false;
+    }
+
+    // if we decided that swapout is possible, do not repeat same checks
+    if (decision == MemObject::SwapOut::swPossible) {
+        debugs(20, 3,  HERE << "already allowed");
         return true;
+    }
+
+    // if we are swapping out already, do not repeat same checks
+    if (swap_status != SWAPOUT_NONE) {
+        debugs(20, 3,  HERE << " already started");
+        decision = MemObject::SwapOut::swPossible;
+        return true;
+    }
+
+    if (!checkCachable()) {
+        debugs(20, 3,  HERE << "not cachable");
+        decision = MemObject::SwapOut::swImpossible;
+        return false;
+    }
+
+    if (EBIT_TEST(flags, ENTRY_SPECIAL)) {
+        debugs(20, 3,  HERE  << url() << " SPECIAL");
+        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
+
+        // use guaranteed maximum if it is known
+        const int64_t expectedEnd = mem_obj->expectedReplySize();
+        debugs(20, 7,  HERE << "expectedEnd = " << expectedEnd);
+        if (expectedEnd > store_maxobjsize) {
+            debugs(20, 3,  HERE << "will not fit: " << expectedEnd <<
+                   " > " << store_maxobjsize);
+            decision = MemObject::SwapOut::swImpossible;
+            return false; // known to outgrow the limit eventually
+        }
+
+        // use current minimum (always known)
+        const int64_t currentEnd = mem_obj->endOffset();
+        if (currentEnd > store_maxobjsize) {
+            debugs(20, 3,  HERE << "does not fit: " << currentEnd <<
+                   " > " << store_maxobjsize);
+            decision = MemObject::SwapOut::swImpossible;
+            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 (mem_obj->inmem_lo > 0)
+        if (store_status != STORE_OK) {
+            const int64_t maxKnownSize = expectedEnd < 0 ?
+                                          mem_obj->availableForSwapOut() : expectedEnd;
+            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
@@ -395,21 +465,29 @@ StoreEntry::swapOutAble() const
      * 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)
+        if (((store_client *) node->data)->getType() == STORE_DISK_CLIENT) {
+            debugs(20, 3, HERE << "DISK client found");
+            decision = MemObject::SwapOut::swPossible;
             return true;
+        }
     }
 
-    /* Don't pollute the disk with icons and other special entries */
-    if (EBIT_TEST(flags, ENTRY_SPECIAL))
-        return false;
-
-    if (!EBIT_TEST(flags, ENTRY_CACHABLE))
-        return false;
-
-    if (!mem_obj->isContiguous())
+    if (!mem_obj->isContiguous()) {
+        debugs(20, 3, "storeSwapOut: not Contiguous");
+        decision = MemObject::SwapOut::swImpossible;
         return false;
+    }
 
+    decision = MemObject::SwapOut::swPossible;
     return true;
 }
index 5b2d7dff3028bb9ae8d3c3e4574a0ada8f041193..5a99d8c2d0d8cb703998dcc7f8f8acdb27efaa93 100644 (file)
@@ -47,3 +47,4 @@ int64_t MemObject::expectedReplySize() const STUB_RETVAL(0)
 void MemObject::resetUrls(char const*, char const*) STUB
 void MemObject::markEndOfReplyHeaders() STUB
 size_t MemObject::inUseCount() STUB_RETVAL(0)
+int64_t MemObject::availableForSwapOut() const STUB_RETVAL(0)
index f3eca34750e3e275e20030f241c63f0694104d4e..b8732c73e76461d9cca24cb473b5f27f54d87675 100644 (file)
@@ -26,8 +26,8 @@ void StoreEntry::complete() STUB
 store_client_t StoreEntry::storeClientType() const STUB_RETVAL(STORE_NON_CLIENT)
 char const *StoreEntry::getSerialisedMetaData() STUB_RETVAL(NULL)
 void StoreEntry::replaceHttpReply(HttpReply *, bool andStartWriting) STUB
-bool StoreEntry::swapoutPossible() STUB_RETVAL(false)
-void StoreEntry::trimMemory() STUB
+bool StoreEntry::mayStartSwapOut() STUB_RETVAL(false)
+void StoreEntry::trimMemory(const bool preserveSwappable) STUB
 void StoreEntry::abort() STUB
 void StoreEntry::unlink() STUB
 void StoreEntry::makePublic() STUB
@@ -41,7 +41,6 @@ void StoreEntry::cacheNegatively() STUB
 void StoreEntry::invokeHandlers() STUB
 void StoreEntry::purgeMem() STUB
 void StoreEntry::swapOut() STUB
-bool StoreEntry::swapOutAble() const STUB_RETVAL(false)
 void StoreEntry::swapOutFileClose(int how) STUB
 const char *StoreEntry::url() const STUB_RETVAL(NULL)
 int StoreEntry::checkCachable() STUB_RETVAL(0)