From: Christos Tsantilas Date: Fri, 13 Jan 2012 13:49:26 +0000 (+0200) Subject: trimMemory for unswappable objects X-Git-Tag: BumpSslServerFirst.take05~12^2~84 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5b55f1f1555190af35a2e814ab2ff010a5846f3f;p=thirdparty%2Fsquid.git trimMemory for unswappable objects 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 --- diff --git a/src/MemObject.cc b/src/MemObject.cc index 08e8ead3cb..373a3db0c1 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -492,3 +492,9 @@ MemObject::mostBytesAllowed() const } #endif + +int64_t +MemObject::availableForSwapOut() const +{ + return endOffset() - swapout.queue_offset; +} diff --git a/src/MemObject.h b/src/MemObject.h index f02ace5521..fab4f0d517 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -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; diff --git a/src/Store.h b/src/Store.h index 78481647e5..21959fbae3 100644 --- a/src/Store.h +++ b/src/Store.h @@ -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() {} diff --git a/src/store.cc b/src/store.cc index 9e9cb3b3bb..7198db54f0 100644 --- a/src/store.cc +++ b/src/store.cc @@ -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; diff --git a/src/store_client.cc b/src/store_client.cc index f8bd81bfdf..fd8003b3e5 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -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 diff --git a/src/store_swapout.cc b/src/store_swapout.cc index a11638bd6f..7a78acf86d 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -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; } diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index 5b2d7dff30..5a99d8c2d0 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -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) diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index f3eca34750..b8732c73e7 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -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)