From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Wed, 23 Jan 2019 04:30:40 +0000 (+0000)
Subject: Fail Rock swapout if the disk dropped some of the write requests (#352)
X-Git-Tag: M-staged-PR356~4
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a12f1a4c6ca6e058d1bc10113a59be7fa6ee5c2d;p=thirdparty%2Fsquid.git

Fail Rock swapout if the disk dropped some of the write requests (#352)

Detecting dropped writes earlier is more than a TODO: If the last entry
write was successful, the whole entry becomes available for hits
immediately.  IpcIoFile::checkTimeouts() that runs every 7 seconds
(IpcIoFile::Timeout) would eventually notify Rock about the timeout,
allowing Rock to release the failed entry, but that notification may
be too late.

The precise outcome of hitting an entry with a missing on-disk slice is
unknown (because the bug was detected by temporary hit validation code
that turned such hits into misses), but SWAPFAIL is the best we could
hope for.
---

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index 894c582c90..b738a5a06a 100644
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -869,42 +869,59 @@ Rock::SwapDir::writeCompleted(int errflag, size_t, RefCount< ::WriteRequest> r)
 
     debugs(79, 7, "errflag=" << errflag << " rlen=" << request->len << " eof=" << request->eof);
 
-    // TODO: Fail if disk dropped one of the previous write requests.
-
-    if (errflag == DISK_OK) {
-        // do not increment sio.offset_ because we do it in sio->write()
-
-        // finalize the shared slice info after writing slice contents to disk
-        Ipc::StoreMap::Slice &slice =
-            map->writeableSlice(sio.swap_filen, request->sidCurrent);
-        slice.size = request->len - sizeof(DbCellHeader);
-        slice.next = request->sidNext;
-
-        if (request->eof) {
-            assert(sio.e);
-            assert(sio.writeableAnchor_);
-            if (sio.touchingStoreEntry()) {
-                sio.e->swap_file_sz = sio.writeableAnchor_->basics.swap_file_sz =
-                                          sio.offset_;
-                map->switchWritingToReading(sio.swap_filen);
-                // sio.e keeps the (now read) lock on the anchor
-            }
-            sio.writeableAnchor_ = NULL;
-            sio.splicingPoint = request->sidCurrent;
-            sio.finishedWriting(errflag);
-        }
-    } else {
-        noteFreeMapSlice(request->sidNext);
-
-        writeError(sio);
-        sio.finishedWriting(errflag);
-        // and wait for the finalizeSwapoutFailure() call to close the map entry
-    }
+    if (errflag != DISK_OK)
+        handleWriteCompletionProblem(errflag, *request);
+    else if (droppedEarlierRequest(*request))
+        handleWriteCompletionProblem(DISK_ERROR, *request);
+    else
+        handleWriteCompletionSuccess(*request);
 
     if (sio.touchingStoreEntry())
         CollapsedForwarding::Broadcast(*sio.e);
 }
 
+/// code shared by writeCompleted() success handling cases
+void
+Rock::SwapDir::handleWriteCompletionSuccess(const WriteRequest &request)
+{
+    auto &sio = *(request.sio);
+    sio.splicingPoint = request.sidCurrent;
+    // do not increment sio.offset_ because we do it in sio->write()
+
+    // finalize the shared slice info after writing slice contents to disk
+    Ipc::StoreMap::Slice &slice =
+        map->writeableSlice(sio.swap_filen, request.sidCurrent);
+    slice.size = request.len - sizeof(DbCellHeader);
+    slice.next = request.sidNext;
+
+    if (request.eof) {
+        assert(sio.e);
+        assert(sio.writeableAnchor_);
+        if (sio.touchingStoreEntry()) {
+            sio.e->swap_file_sz = sio.writeableAnchor_->basics.swap_file_sz =
+                                      sio.offset_;
+
+            map->switchWritingToReading(sio.swap_filen);
+            // sio.e keeps the (now read) lock on the anchor
+        }
+        sio.writeableAnchor_ = NULL;
+        sio.finishedWriting(DISK_OK);
+    }
+}
+
+/// code shared by writeCompleted() error handling cases
+void
+Rock::SwapDir::handleWriteCompletionProblem(const int errflag, const WriteRequest &request)
+{
+    auto &sio = *request.sio;
+
+    noteFreeMapSlice(request.sidNext);
+
+    writeError(sio);
+    sio.finishedWriting(errflag);
+    // and hope that Core will call disconnect() to close the map entry
+}
+
 void
 Rock::SwapDir::writeError(StoreIOState &sio)
 {
@@ -919,6 +936,23 @@ Rock::SwapDir::writeError(StoreIOState &sio)
     // All callers must also call IoState callback, to propagate the error.
 }
 
+/// whether the disk has dropped at least one of the previous write requests
+bool
+Rock::SwapDir::droppedEarlierRequest(const WriteRequest &request) const
+{
+    const auto &sio = *request.sio;
+    assert(sio.writeableAnchor_);
+    const Ipc::StoreMapSliceId expectedSliceId = sio.splicingPoint < 0 ?
+            sio.writeableAnchor_->start :
+            map->writeableSlice(sio.swap_filen, sio.splicingPoint).next;
+    if (expectedSliceId != request.sidCurrent) {
+        debugs(79, 3, "yes; expected " << expectedSliceId << ", but got " << request.sidCurrent);
+        return true;
+    }
+
+    return false;
+}
+
 void
 Rock::SwapDir::updateHeaders(StoreEntry *updatedE)
 {
diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h
index aa88283758..0748337b1e 100644
--- a/src/fs/rock/RockSwapDir.h
+++ b/src/fs/rock/RockSwapDir.h
@@ -138,6 +138,9 @@ protected:
 
 private:
     void createError(const char *const msg);
+    void handleWriteCompletionSuccess(const WriteRequest &request);
+    void handleWriteCompletionProblem(const int errflag, const WriteRequest &request);
+    bool droppedEarlierRequest(const WriteRequest &request) const;
 
     DiskIOStrategy *io;
     RefCount<DiskFile> theFile; ///< cache storage for this cache_dir
diff --git a/src/fs/rock/forward.h b/src/fs/rock/forward.h
index ba64d03c22..4e68a2792c 100644
--- a/src/fs/rock/forward.h
+++ b/src/fs/rock/forward.h
@@ -40,6 +40,8 @@ class HeaderUpdater;
 
 class DbCellHeader;
 
+class WriteRequest;
+
 }
 
 #endif /* SQUID_FS_ROCK_FORWARD_H */