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 6a89039d990e5c5d9b85b3d5662686edf3f34246..efa3e0665a740d3120c6a119535a0ec1d5d11935 100644 (file)
--- 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<int64_t>(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<uint64_t>(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 16e24ab87069b506de8bb6ae863bf1bb5d686ffe..b87b624e1752d5ee6079c87ca354b86eafe92d02 100644 (file)
--- 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 8972a4537ce2a6da82cd6b0ef6d3d980d6cc119d..9d7d1afc00422cf72546915858a7b40566c922f3 100644 (file)
--- 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<SwapDir *>(Config.cacheSwap.swapDirs[i].getRaw())->
-                max_objsize > ms)
-            ms = dynamic_cast<SwapDir *>(Config.cacheSwap.swapDirs[i].getRaw())->max_objsize;
+        const int64_t storeMax = dynamic_cast<SwapDir *>(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 25fb379f2995a174ba7b4a700dba749c93be94a2..91752216cb617d2c6404cdcb96a435ed056ddf16 100644 (file)
--- 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 0b036b7230c932fc204c7eee818b05926a338c34..a682fb5128573aa6a5258cfb5c97490edd41cb1d 100644 (file)
--- 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 659a470c9a62b16d416d6f55826f326db2710f04..781170f4495596bcbfe4a248f84b626e002ecbc2 100644 (file)
--- 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 0d3a30103dbd5e16ba631be758edf20a95970237..2d6f0fd09faab3ac18e87981cf2a634070d794b3 100644 (file)
--- 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 08f8878bb27131ac21f7f65d472807695da73c27..9d1ad7266e64c37ea8f4d1d3b1976156ff103b0a 100644 (file)
--- 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 84481fe823704c81642801a1400eaa4db2ad9e6c..fbe1115809e28c4fa6176184ee19d5b69a54303d 100644 (file)
--- 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<HttpReply *>(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 22c8a50d6b10544dada7fa609ead30fbeb8141c7..8205999e9cc9d2d507d501bbba39a0f9960a45f7 100644 (file)
--- 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();