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 9851bbe836b36c84c2d726e68a49567ff12a9ada..6244b06715f30a05000b8a24d1e5a86a53c1931a 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 488cafca918279a9b82177561874b9fadba12e31..d585c99c0f889fa9be5c8618cab3f579ef8543a3 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.
+    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 @@ 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 fbcaa91459024e26013967013a58420f95d069d1..d480969a32241333e9bdd503af1c34b2c9743ec7 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 < 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 77563983613aee90d913750d5b83caac9c41e78a..b04c7cc6e89a6b4866c32cc82d54b98985fdeb74 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -3061,7 +3061,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
@@ -3260,14 +3260,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 f61827151c94334c3410db75edc28c8af17613ac..ab1199beb203d4d08ffd7002fcb36e0063ec8371 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -998,9 +998,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 8b21afe7bfaf840c2901d3d0a6eb00c6bdd5945f..400f29dfdb1b2227031f8676c96b4d583e736f08 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 c1d3ae5969ccc66115610f070e47fa0c849874d9..5fdad0eb443af30e86bcae081676718640ba4c06 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, 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 1213414642fa884af162115a93f985a45daa2e73..8d962b44a6c21b1231e084d9a69c8b77da5e2622 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 = 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();