From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 30 Jul 2013 17:10:57 +0000 (-0600)
Subject: Prevented STORE_DISK_CLIENT assertions for aborted entries. Polished code.
X-Git-Tag: SQUID_3_5_0_1~444^2~18
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f25d697fa8eba099303cbb0e63df7fc5c99fa771;p=thirdparty%2Fsquid.git

Prevented STORE_DISK_CLIENT assertions for aborted entries. Polished code.

To prevent store_client.cc:445: "STORE_DISK_CLIENT == getType()" assertions,
rewrote the storeClientNoMoreToSend() function so that it does not send a
STORE_MEM_CLIENT to read from disk. Used this opportunity to polish this
negative function code and convert it into a positive method.

Documented known (and mostly old) StoreEntry::storeClientType() problems.
---

diff --git a/src/StoreClient.h b/src/StoreClient.h
index 9264c6fd74..f7e2c3b06e 100644
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -98,6 +98,8 @@ public:
     StoreIOBuffer copyInto;
 
 private:
+    bool moreToSend() const;
+
     void fileRead();
     void scheduleDiskRead();
     void scheduleMemRead();
diff --git a/src/store.cc b/src/store.cc
index 20d9582468..9aabf49d68 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -309,6 +309,11 @@ StoreEntry::setNoDelay (bool const newValue)
         mem_obj->setNoDelay(newValue);
 }
 
+// XXX: Type names mislead. STORE_DISK_CLIENT actually means that we should
+//      open swapin file, aggressively trim memory, and ignore read-ahead gap.
+//      It does not mean we will read from disk exclusively (or at all!).
+// XXX: May create STORE_DISK_CLIENT with no disk caching configured.
+// XXX: Collapsed clients cannot predict their type.
 store_client_t
 StoreEntry::storeClientType() const
 {
@@ -336,7 +341,7 @@ StoreEntry::storeClientType() const
             if (swap_status == SWAPOUT_DONE) {
                 debugs(20,7, HERE << mem_obj << " lo: " << mem_obj->inmem_lo << " hi: " << mem_obj->endOffset() << " size: " << mem_obj->object_sz);
                 if (mem_obj->endOffset() == mem_obj->object_sz) {
-                    /* hot object fully swapped in */
+                    /* hot object fully swapped in (XXX: or swapped out?) */
                     return STORE_MEM_CLIENT;
                 }
             } else {
diff --git a/src/store_client.cc b/src/store_client.cc
index 519c0d0e4b..58c6dc63dc 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -278,31 +278,35 @@ store_client::copy(StoreEntry * anEntry,
     anEntry->unlock("store_client::copy");
 }
 
-/*
- * This function is used below to decide if we have any more data to
- * send to the client.  If the store_status is STORE_PENDING, then we
- * do have more data to send.  If its STORE_OK, then
- * we continue checking.  If the object length is negative, then we
- * don't know the real length and must open the swap file to find out.
- * However, if there is no swap file, then there is no more to send.
- * If the length is >= 0, then we compare it to the requested copy
- * offset.
- */
-static int
-storeClientNoMoreToSend(StoreEntry * e, store_client * sc)
+/// Whether there is (or will be) more entry data for us.
+bool
+store_client::moreToSend() const
 {
-    int64_t len;
+    if (entry->store_status == STORE_PENDING)
+        return true; // there may be more coming
 
-    if (e->store_status == STORE_PENDING)
-        return 0;
+    /* STORE_OK, including aborted entries: no more data is coming */
 
-    if ((len = e->objectLen()) < 0)
-        return e->swap_filen < 0;
+    const int64_t len = entry->objectLen();
 
-    if (sc->copyInto.offset < len)
-        return 0;
+    // If we do not know the entry length, then we have to open the swap file,
+    // which is only possible if there is one AND if we are allowed to use it.
+    const bool canSwapIn = entry->swap_filen >= 0 &&
+                           getType() == STORE_DISK_CLIENT;
+    if (len < 0)
+        return canSwapIn;
 
-    return 1;
+    if (copyInto.offset >= len)
+        return false; // sent everything there is
+
+    if (canSwapIn)
+        return true; // if we lack prefix, we can swap it in
+
+    // If we cannot swap in, make sure we have what we want in RAM. Otherwise,
+    // scheduleRead calls scheduleDiskRead which asserts on STORE_MEM_CLIENTs.
+    const MemObject *mem = entry->mem_obj;
+    return mem &&
+        mem->inmem_lo <= copyInto.offset && copyInto.offset < mem->endOffset();
 }
 
 static void
@@ -359,7 +363,7 @@ store_client::doCopy(StoreEntry *anEntry)
            copyInto.offset << ", hi: " <<
            mem->endOffset());
 
-    if (storeClientNoMoreToSend(entry, this)) {
+    if (!moreToSend()) {
         /* There is no more to send! */
         debugs(33, 3, HERE << "There is no more to send!");
         callback(0);