------------------------------------------------------------ revno: 11782 revision-id: squid3@treenet.co.nz-20130215093603-itfelqgmcc1h2kvk parent: squid3@treenet.co.nz-20130209071555-coyfdnc1vlomz7u0 fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3752 author: Alex Rousskov committer: Amos Jeffries branch nick: 3.2 timestamp: Fri 2013-02-15 02:36:03 -0700 message: Bug 3752: objects that cannot be cached in memory are not cached on disk if cache_dir max-size is used. 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20130215093603-itfelqgmcc1h2kvk # target_branch: http://bzr.squid-cache.org/bzr/squid3/branches\ # /SQUID_3_2 # testament_sha1: 00d87dceb2f014562d2d1589e9c48af888d01060 # timestamp: 2013-02-15 09:38:13 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/branches\ # /SQUID_3_2 # base_revision_id: squid3@treenet.co.nz-20130209071555-\ # coyfdnc1vlomz7u0 # # Begin patch === modified file 'src/store_swapout.cc' --- src/store_swapout.cc 2012-07-17 11:41:40 +0000 +++ src/store_swapout.cc 2013-02-15 09:36:03 +0000 @@ -200,7 +200,7 @@ 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 @@ bool StoreEntry::mayStartSwapOut() { - dlink_node *node; - // must be checked in the caller assert(!EBIT_TEST(flags, ENTRY_ABORTED)); assert(!swappingOut()); @@ -403,6 +401,18 @@ 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,67 +436,22 @@ 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; - } - } - - if (!mem_obj->isContiguous()) { - debugs(20, 3, "storeSwapOut: not Contiguous"); - decision = MemObject::SwapOut::swImpossible; - return false; + /* + * 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 + } } decision = MemObject::SwapOut::swPossible;