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 cd5e54c48cfb55e5cb06b8f956e04e7346da2e3c..e38b04b411feb7b8808eae235184ab522565b0b0 100644 (file)
--- a/src/SwapDir.cc
+++ b/src/SwapDir.cc
@@ -39,8 +39,8 @@
 #include "ConfigOption.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)
 {
@@ -111,6 +111,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 0a445f2b5333e6be1b8badea7001fc3f375a952d..3531de802f8ff9bb4cff7ed966fc0f3a873000bf 100644 (file)
--- a/src/SwapDir.h
+++ b/src/SwapDir.h
@@ -149,7 +149,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.
+    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;
@@ -181,13 +187,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 ed493154dd234899f07d5246295e862486ddb22f..bf2bd20db8675f1b305d508e9792903c91cd0610 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -223,16 +223,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 < static_cast<int64_t>(Config.Store.maxInMemObjSize))
+        ms = Config.Store.maxInMemObjSize;
+
     store_maxobjsize = ms;
 }
 

diff --git a/src/cf.data.pre b/src/cf.data.pre
index bcf9e0c81554239c0fd0720706d474b868395cfa..cb591c4adf823a0f7ed03f6505f9f6ffe1971689 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -2808,7 +2808,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
@@ -3007,14 +3007,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 d755efe90c2e0e2a3969c07d256257a7755d0a5a..781863b2d604f36fea6d8d16d3633d22884f2ff8 100644 (file)
--- a/src/fs/rock/RockIoState.cc
+++ b/src/fs/rock/RockIoState.cc
@@ -25,7 +25,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 91fc190f0c35e20b00d8136e21e2e9a36b46785b..032543aa5c348ba539265b59b991a966a05a2cf4 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -988,9 +988,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 7468f4610f571569f1e1e32d7672169211e6f2a8..d1d4723f7fd42ccacaca24570282d1347cc1b5ad 100644 (file)
--- a/src/store_dir.cc
+++ b/src/store_dir.cc
@@ -270,10 +270,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 */
@@ -282,7 +282,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 2f62e1b5f7fdfd932a67ef9c443b43c2bdcf91a1..d03d18329d88bfb1d0cb66d3d34e49a5b2839f2b 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 34b1ff112dd6222f1d6a8ec3636412127471a0d2..8a29ea9b2123bd84a948a85402b4c9edccd8ff04 100644 (file)
--- a/src/tests/testRock.cc
+++ b/src/tests/testRock.cc
@@ -65,6 +65,7 @@ testRock::setUp()
     strtok(config_line, w_space);
 
     store->parse(0, path);
+    store_maxobjsize = 1024*1024*2;
 
     safe_free(path);
 
@@ -173,8 +174,7 @@ testRock::createEntry(const int i)
     StoreEntry *const pe =
         storeCreateEntry(url, "dummy log url", flags, 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 34e129135a392d86b39a01c591341db56cfb4dc0..a76f4a68cf78116403aaa38e250aecdace118f7f 100644 (file)
--- a/src/tests/testUfs.cc
+++ b/src/tests/testUfs.cc
@@ -108,6 +108,7 @@ testUfs::testUfsSearch()
     strtok(config_line, w_space);
 
     aStore->parse(0, path);
+    store_maxobjsize = 1024*1024*2;
 
     safe_free(path);
 
@@ -142,7 +143,7 @@ testUfs::testUfsSearch()
         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();