From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Fri, 19 Jul 2019 09:23:55 +0000 (+0000)
Subject: Disabled partial rock cache_dir writes (#440)
X-Git-Tag: SQUID_5_0_1~66
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=424f369933b372eb6a4093b13593311778ba9152;p=thirdparty%2Fsquid.git

Disabled partial rock cache_dir writes (#440)

Partial disk writes may be useful for CF disk slaves and SMP disk hit
readers, but their correct implementation requires a lot of additional
work, the current implementation is insufficient/buggy, and partially
written entries were probably never read because Rock writers do not
enable shared/appending locks.

Here is a brief (but complicated) history of the issue, for the record:

    1. 807feb1 adds partial disk writes "in order to propagate data from
       the hit writer to the collapsed hit readers". The readers
       probably could not read any data though because the disk entry
       was still exclusively locked for writing. The developers either
       did not realize that or intended to address it later, but did not
       document their intentions -- all this development was happening
       on a fast-moving CF development branch.

    2. 0b8be48 makes those partial disk writes conditional on CF being
       enabled. It is not clear whether the developers wanted to reduce
       the scope of a risky feature or did not realize that non-CF use
       cases also benefit from partial writes (when fully supported).

    3. ce49546 adds low-level appending lock support but does not
       actually use append locks for any caches.

    4. 4475555 adds appending support to the shared memory cache.

    5. 4976925 explicitly disables partial disk writes, acknowledging
       that they were probably never used (by readers) anyway due to the
       lack of a Ipc::StoreMap::startAppending() call. The same commit
       documents that partial writes caused problems (for writers) in
       performance tests.

    6. 5296bbd re-enables partial disk writes (for writers) after fixing
       problems detected earlier in performance tests. This commit does
       not add the critical (for readers) startAppending() call. It
       looks like the lack of that call was overlooked, again!
---

diff --git a/src/fs/rock/RockIoState.cc b/src/fs/rock/RockIoState.cc
index 131f5d4215..ec80e2403c 100644
--- a/src/fs/rock/RockIoState.cc
+++ b/src/fs/rock/RockIoState.cc
@@ -210,9 +210,15 @@ Rock::IoState::tryWrite(char const *buf, size_t size, off_t coreOff)
             const auto sidNext = dir->reserveSlotForWriting(); // throws
             assert(sidNext >= 0);
             writeToDisk(sidNext);
-        } else if (Store::Root().transientReaders(*e)) {
-            // write partial buffer for all remote hit readers to see
-            writeBufToDisk(-1, false, false);
+            // } else if (Store::Root().transientReaders(*e)) {
+            // XXX: Partial writes cannot be read -- no map->startAppending()!
+            // XXX: Partial writes confuse SwapDir::droppedEarlierRequest(),
+            // resulting in released entries and, hence, misses and CF retries.
+            // XXX: The effective benefit of partial writes is reduced by
+            // doPages() buffering SM_PAGE_SIZE*n leftovers.
+
+            // // write partial buffer for all remote hit readers to see
+            // writeBufToDisk(-1, false, false);
         }
     }