From: Amos Jeffries Date: Sat, 9 Feb 2013 00:44:07 +0000 (-0700) Subject: Bug 3686: cache_dir max-size default fails X-Git-Tag: SQUID_3_4_0_1~299 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b51ec8c8a64493bc7408e8c3f764fa0a621e36b9;p=thirdparty%2Fsquid.git 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. --- diff --git a/src/SwapDir.cc b/src/SwapDir.cc index 6a89039d99..efa3e0665a 100644 --- a/src/SwapDir.cc +++ b/src/SwapDir.cc @@ -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 @@ SwapDir::minSize() const 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 &) {} diff --git a/src/SwapDir.h b/src/SwapDir.h index 16e24ab870..b87b624e17 100644 --- a/src/SwapDir.h +++ b/src/SwapDir.h @@ -148,7 +148,13 @@ public: 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. + 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 @@ private: 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; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 8972a4537c..9d7d1afc00 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -277,16 +277,23 @@ self_destruct(void) 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 < Config.Store.maxInMemObjSize) + ms = Config.Store.maxInMemObjSize; + store_maxobjsize = ms; } diff --git a/src/cf.data.pre b/src/cf.data.pre index 25fb379f29..91752216cb 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -3195,7 +3195,7 @@ DOC_START 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 @@ -3394,14 +3394,18 @@ DOC_END 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 diff --git a/src/fs/rock/RockIoState.cc b/src/fs/rock/RockIoState.cc index 0b036b7230..a682fb5128 100644 --- a/src/fs/rock/RockIoState.cc +++ b/src/fs/rock/RockIoState.cc @@ -24,7 +24,7 @@ Rock::IoState::IoState(SwapDir *dir, { 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); diff --git a/src/store.cc b/src/store.cc index 659a470c9a..781170f449 100644 --- a/src/store.cc +++ b/src/store.cc @@ -1013,9 +1013,8 @@ StoreEntry::checkCachable() ++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()) { diff --git a/src/store_dir.cc b/src/store_dir.cc index 0d3a30103d..2d6f0fd09f 100644 --- a/src/store_dir.cc +++ b/src/store_dir.cc @@ -277,10 +277,10 @@ storeDirSelectSwapDirLeastLoad(const StoreEntry * e) /* 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 @@ storeDirSelectSwapDirLeastLoad(const StoreEntry * e) } least_load = load; - least_objsize = SD->max_objsize; + least_objsize = SD->maxObjectSize(); most_free = cur_free; dirn = i; } diff --git a/src/store_swapout.cc b/src/store_swapout.cc index 08f8878bb2..9d1ad7266e 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -441,10 +441,11 @@ StoreEntry::mayStartSwapOut() 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? */ diff --git a/src/tests/testRock.cc b/src/tests/testRock.cc index 84481fe823..fbe1115809 100644 --- a/src/tests/testRock.cc +++ b/src/tests/testRock.cc @@ -71,6 +71,7 @@ testRock::setUp() strtok(config_line, w_space); store->parse(0, path); + store_maxobjsize = 1024*1024*2; safe_free(path); @@ -179,8 +180,7 @@ testRock::createEntry(const int i) StoreEntry *const pe = storeCreateEntry(url, "dummy log url", flags, Http::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(); diff --git a/src/tests/testUfs.cc b/src/tests/testUfs.cc index 22c8a50d6b..8205999e9c 100644 --- a/src/tests/testUfs.cc +++ b/src/tests/testUfs.cc @@ -111,6 +111,7 @@ testUfs::testUfsSearch() strtok(config_line, w_space); aStore->parse(0, path); + store_maxobjsize = 1024*1024*2; safe_free(path); @@ -145,7 +146,7 @@ testUfs::testUfsSearch() flags.cachable = true; StoreEntry *pe = storeCreateEntry("dummy url", "dummy log url", flags, Http::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();