diff -ru squid-3.2.0.14/src/MemObject.cc squid-3.2.0.14-mem_node/src/MemObject.cc --- squid-3.2.0.14/src/MemObject.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/MemObject.cc 2012-01-17 20:11:54.756609310 +0100 @@ -492,3 +492,9 @@ } #endif + +int64_t +MemObject::availableForSwapOut() const +{ + return endOffset() - swapout.queue_offset; +} diff -ru squid-3.2.0.14/src/MemObject.h squid-3.2.0.14-mem_node/src/MemObject.h --- squid-3.2.0.14/src/MemObject.h 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/MemObject.h 2012-01-17 20:11:54.757609437 +0100 @@ -81,6 +81,7 @@ */ 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 -ru squid-3.2.0.14/src/store.cc squid-3.2.0.14-mem_node/src/store.cc --- squid-3.2.0.14/src/store.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/store.cc 2012-01-17 20:11:54.761609945 +0100 @@ -1890,95 +1890,8 @@ 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 @@ -1988,7 +1901,7 @@ if (mem_status == IN_MEMORY) return; - if (!swapOutAble()) { + if (!preserveSwappable) { if (mem_obj->policyLowestOffsetToKeep(0) == 0) { /* Nothing to do */ return; Endast i squid-3.2.0.14-mem_node/src: store.cc.orig diff -ru squid-3.2.0.14/src/store_client.cc squid-3.2.0.14-mem_node/src/store_client.cc --- squid-3.2.0.14/src/store_client.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/store_client.cc 2012-01-17 20:11:54.768610834 +0100 @@ -194,7 +194,7 @@ 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 Endast i squid-3.2.0.14-mem_node/src: store_client.cc.orig diff -ru squid-3.2.0.14/src/Store.h squid-3.2.0.14-mem_node/src/Store.h --- squid-3.2.0.14/src/Store.h 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/Store.h 2012-01-17 20:11:56.770865312 +0100 @@ -92,8 +92,9 @@ 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 @@ 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,9 +249,9 @@ store_client_t storeClientType() const {return STORE_MEM_CLIENT;} char const *getSerialisedMetaData(); - bool swapoutPossible() {return false;} + bool mayStartSwapout() {return false;} - void trimMemory() {} + void trimMemory(const bool preserveSwappable) {} static NullStoreEntry _instance; diff -ru squid-3.2.0.14/src/store_swapout.cc squid-3.2.0.14-mem_node/src/store_swapout.cc --- squid-3.2.0.14/src/store_swapout.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/store_swapout.cc 2012-01-17 20:11:54.771611216 +0100 @@ -187,8 +187,20 @@ 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. @@ -200,39 +212,10 @@ 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) { @@ -245,9 +228,9 @@ 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 @@ -373,19 +356,106 @@ 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 (mem_obj->inmem_lo > 0) + 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 (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 @@ -394,21 +464,29 @@ * 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; } Endast i squid-3.2.0.14-mem_node/src: store_swapout.cc.orig diff -ru squid-3.2.0.14/src/tests/stub_MemObject.cc squid-3.2.0.14-mem_node/src/tests/stub_MemObject.cc --- squid-3.2.0.14/src/tests/stub_MemObject.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/tests/stub_MemObject.cc 2012-01-17 20:11:54.772611344 +0100 @@ -47,6 +47,7 @@ { return data_hdr.endOffset(); } +int64_t MemObject::availableForSwapOut() const STUB_RETVAL(0) void MemObject::trimSwappable() Endast i squid-3.2.0.14-mem_node/src/tests: stub_MemObject.cc.orig diff -ru squid-3.2.0.14/src/tests/stub_store.cc squid-3.2.0.14-mem_node/src/tests/stub_store.cc --- squid-3.2.0.14/src/tests/stub_store.cc 2011-12-12 12:08:18.000000000 +0100 +++ squid-3.2.0.14-mem_node/src/tests/stub_store.cc 2012-01-17 20:11:54.773611472 +0100 @@ -26,8 +26,8 @@ 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::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)