------------------------------------------------------------ revno: 12493 revision-id: squid3@treenet.co.nz-20130214073442-b5tggdqss1ws6v12 parent: squid3@treenet.co.nz-20130209124822-dio2ah4sqbndp80c fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3686 committer: Amos Jeffries branch nick: 3.3 timestamp: Thu 2013-02-14 00:34:42 -0700 message: Bug 3686: cache_dir max-size default fails If some cache_dir are configured with max-size and some not the default maximum_object_size limit fails. This refactors the max-size management code such that each SwapDir always has a value maxObjectSize(). This value is calculated from the SwapDir local setting or global limit as appropriate. The global maximum_object_size directive is migrated to simply be a default for cache_dir max-size= option. The global store_maxobjsize variable is altered to be the overall global limit on how big an object may be cache by this proxy. It now takes into account the max-size for all cache_dir and cache_mem limitation. NP: The slow accumulation of these and earlier changes means Squid no longer immediately caches unknown-length objects. The unit-tests are therefore changed to test using explicit 0-length objects to ensure the test is on a cached object not bypassing the apparently ested logic. They are also provided with a large global store_maxobjsize limit in order to do a weak test of the SwapDir types max-size in the presence of other larger cache_dir or maximum_object_size settings. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20130214073442-b5tggdqss1ws6v12 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # testament_sha1: f3ed70b00512bd2de48ff4614f7edfe5e3151d08 # timestamp: 2013-02-14 07:38:29 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # base_revision_id: squid3@treenet.co.nz-20130209124822-\ # dio2ah4sqbndp80c # # Begin patch === modified file 'src/SwapDir.cc' --- src/SwapDir.cc 2013-02-09 06:56:06 +0000 +++ src/SwapDir.cc 2013-02-14 07:34:42 +0000 @@ -42,8 +42,8 @@ #include "tools.h" SwapDir::SwapDir(char const *aType): theType(aType), - max_size(0), - path(NULL), index(-1), disker(-1), min_objsize(0), max_objsize (-1), + max_size(0), min_objsize(0), max_objsize (-1), + path(NULL), index(-1), disker(-1), repl(NULL), removals(0), scanned(0), cleanLog(NULL) { @@ -114,6 +114,39 @@ return ((maxSize() * Config.Swap.lowWaterMark) / 100); } +int64_t +SwapDir::maxObjectSize() const +{ + // per-store max-size=N value is authoritative + if (max_objsize > -1) + return max_objsize; + + // store with no individual max limit is limited by configured maximum_object_size + // or the total store size, whichever is smaller + return min(static_cast(maxSize()), Config.Store.maxObjectSize); +} + +void +SwapDir::maxObjectSize(int64_t newMax) +{ + // negative values mean no limit (-1) + if (newMax < 0) { + max_objsize = -1; // set explicitly in case it had a non-default value previously + return; + } + + // prohibit values greater than total storage area size + // but set max_objsize to the maximum allowed to override maximum_object_size global config + if (static_cast(newMax) > maxSize()) { + debugs(47, DBG_PARSE_NOTE(2), "WARNING: Ignoring 'max-size' option for " << path << + " which is larger than total cache_dir size of " << maxSize() << " bytes."); + max_objsize = maxSize(); + return; + } + + max_objsize = newMax; +} + void SwapDir::reference(StoreEntry &) {} === modified file 'src/SwapDir.h' --- src/SwapDir.h 2012-10-16 23:43:54 +0000 +++ src/SwapDir.h 2013-02-14 07:34:42 +0000 @@ -148,7 +148,13 @@ virtual uint64_t minSize() const; - virtual int64_t maxObjectSize() const { return max_objsize; } + /// The maximum size of object which may be stored here. + /// Larger objects will not be added and may be purged. + virtual int64_t maxObjectSize() const; + + /// configure the maximum object size for this storage area. + /// May be any size up to the total storage area. + void maxObjectSize(int64_t newMax); virtual void getStats(StoreInfoStats &stats) const; virtual void stat (StoreEntry &anEntry) const; @@ -180,13 +186,13 @@ protected: uint64_t max_size; ///< maximum allocatable size of the storage area + int64_t min_objsize; ///< minimum size of any object stored here (-1 for no limit) + int64_t max_objsize; ///< maximum size of any object stored here (-1 for no limit) public: char *path; int index; /* This entry's index into the swapDirs array */ int disker; ///< disker kid id dedicated to this SwapDir or -1 - int64_t min_objsize; - int64_t max_objsize; RemovalPolicy *repl; int removals; int scanned; === modified file 'src/cache_cf.cc' --- src/cache_cf.cc 2013-01-28 04:15:03 +0000 +++ src/cache_cf.cc 2013-02-14 07:34:42 +0000 @@ -277,16 +277,23 @@ static void update_maxobjsize(void) { - int i; int64_t ms = -1; - for (i = 0; i < Config.cacheSwap.n_configured; ++i) { + // determine the maximum size object that can be stored to disk + for (int i = 0; i < Config.cacheSwap.n_configured; ++i) { assert (Config.cacheSwap.swapDirs[i].getRaw()); - if (dynamic_cast(Config.cacheSwap.swapDirs[i].getRaw())-> - max_objsize > ms) - ms = dynamic_cast(Config.cacheSwap.swapDirs[i].getRaw())->max_objsize; + const int64_t storeMax = dynamic_cast(Config.cacheSwap.swapDirs[i].getRaw())->maxObjectSize(); + if (ms < storeMax) + ms = storeMax; } + + // Ensure that we do not discard objects which could be stored only in memory. + // It is governed by maximum_object_size_in_memory (for now) + // TODO: update this to check each in-memory location (SMP and local memory limits differ) + if (ms < static_cast(Config.Store.maxInMemObjSize)) + ms = Config.Store.maxInMemObjSize; + store_maxobjsize = ms; } === modified file 'src/cf.data.pre' --- src/cf.data.pre 2013-02-09 07:14:42 +0000 +++ src/cf.data.pre 2013-02-14 07:34:42 +0000 @@ -3061,7 +3061,7 @@ replacement policies. NOTE: if using the LFUDA replacement policy you should increase - the value of maximum_object_size above its default of 4096 KB to + the value of maximum_object_size above its default of 4 MB to to maximize the potential byte hit rate improvement of LFUDA. For more information about the GDSF and LFUDA cache replacement @@ -3260,14 +3260,18 @@ NAME: maximum_object_size COMMENT: (bytes) TYPE: b_int64_t -DEFAULT: 4096 KB +DEFAULT: 4 MB LOC: Config.Store.maxObjectSize DOC_START - Objects larger than this size will NOT be saved on disk. The - value is specified in kilobytes, and the default is 4MB. If - you wish to get a high BYTES hit ratio, you should probably + The default limit on size of objects stored to disk. + This size is used for cache_dir where max-size is not set. + The value is specified in bytes, and the default is 4 MB. + + If you wish to get a high BYTES hit ratio, you should probably increase this (one 32 MB object hit counts for 3200 10KB - hits). If you wish to increase speed more than your want to + hits). + + If you wish to increase hit ratio more than you want to save bandwidth you should leave this low. NOTE: if using the LFUDA replacement policy you should increase === modified file 'src/fs/rock/RockIoState.cc' --- src/fs/rock/RockIoState.cc 2012-09-01 14:38:36 +0000 +++ src/fs/rock/RockIoState.cc 2013-02-14 07:34:42 +0000 @@ -24,7 +24,7 @@ { e = anEntry; // swap_filen, swap_dirn, diskOffset, and payloadEnd are set by the caller - slotSize = dir->max_objsize; + slotSize = dir->maxObjectSize(); file_callback = cbFile; callback = cbIo; callback_data = cbdataReference(data); === modified file 'src/store.cc' --- src/store.cc 2012-10-05 23:36:22 +0000 +++ src/store.cc 2013-02-14 07:34:42 +0000 @@ -998,9 +998,8 @@ ++store_check_cachable_hist.no.negative_cached; return 0; /* avoid release call below */ } else if ((getReply()->content_length > 0 && - getReply()->content_length - > Config.Store.maxObjectSize) || - mem_obj->endOffset() > Config.Store.maxObjectSize) { + getReply()->content_length > store_maxobjsize) || + mem_obj->endOffset() > store_maxobjsize) { debugs(20, 2, "StoreEntry::checkCachable: NO: too big"); ++store_check_cachable_hist.no.too_big; } else if (checkTooSmall()) { === modified file 'src/store_dir.cc' --- src/store_dir.cc 2013-01-22 04:42:19 +0000 +++ src/store_dir.cc 2013-02-14 07:34:42 +0000 @@ -277,10 +277,10 @@ /* If the load is equal, then look in more details */ if (load == least_load) { - /* closest max_objsize fit */ + /* closest max-size fit */ if (least_objsize != -1) - if (SD->max_objsize > least_objsize || SD->max_objsize == -1) + if (SD->maxObjectSize() > least_objsize) continue; /* most free */ @@ -289,7 +289,7 @@ } least_load = load; - least_objsize = SD->max_objsize; + least_objsize = SD->maxObjectSize(); most_free = cur_free; dirn = i; } === modified file 'src/store_swapout.cc' --- src/store_swapout.cc 2013-02-09 12:48:22 +0000 +++ src/store_swapout.cc 2013-02-14 07:34:42 +0000 @@ -441,10 +441,11 @@ const int64_t maxKnownSize = mem_obj->availableForSwapOut(); debugs(20, 7, HERE << "maxKnownSize= " << maxKnownSize); /* - * 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 + * NOTE: the store_maxobjsize here is the global maximum + * size of object cacheable in any of Squid cache stores + * both disk and memory stores. + * + * 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? */ === modified file 'src/tests/testRock.cc' --- src/tests/testRock.cc 2012-09-24 19:34:52 +0000 +++ src/tests/testRock.cc 2013-02-14 07:34:42 +0000 @@ -71,6 +71,7 @@ strtok(config_line, w_space); store->parse(0, path); + store_maxobjsize = 1024*1024*2; safe_free(path); @@ -179,8 +180,7 @@ StoreEntry *const pe = storeCreateEntry(url, "dummy log url", flags, METHOD_GET); HttpReply *const rep = const_cast(pe->getReply()); - rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", - -1, -1, squid_curtime + 100000); + rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); pe->setPublicKey(); === modified file 'src/tests/testUfs.cc' --- src/tests/testUfs.cc 2012-09-18 21:05:32 +0000 +++ src/tests/testUfs.cc 2013-02-14 07:34:42 +0000 @@ -111,6 +111,7 @@ strtok(config_line, w_space); aStore->parse(0, path); + store_maxobjsize = 1024*1024*2; safe_free(path); @@ -145,7 +146,7 @@ flags.cachable = 1; StoreEntry *pe = storeCreateEntry("dummy url", "dummy log url", flags, METHOD_GET); HttpReply *rep = (HttpReply *) pe->getReply(); // bypass const - rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", -1, -1, squid_curtime + 100000); + rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); pe->setPublicKey();