From 8916cfafc4ce57d4c6038bce7a0c4b5afcce695c Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 20 Jul 2012 19:28:42 -0600
Subject: [PATCH] Avoid bogus "Disk space over limit" warnings when rebuidling
 dirty ufs index.

Subtract sizes of added-then-rejected entries while loading ufs cache index.

Before SMP changes, the ufs code was incorrectly ignoring the size of
loaded-but-not-yet-validated entries, leading to cache disk overflows.
After SMP changes, the ufs code was not subtracting sizes of
loaded-but-then-rejected entries, leading to bogus "Disk space over
limit" warnings. Now we correctly account for both kinds of entries.
---
 src/fs/ufs/store_dir_ufs.cc | 15 +++++++++-
 src/fs/ufs/ufscommon.cc     | 56 ++++++++++++++++---------------------
 src/fs/ufs/ufscommon.h      |  3 ++
 3 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/src/fs/ufs/store_dir_ufs.cc b/src/fs/ufs/store_dir_ufs.cc
index 96a0cfa2c2..c64a17118e 100644
--- a/src/fs/ufs/store_dir_ufs.cc
+++ b/src/fs/ufs/store_dir_ufs.cc
@@ -728,6 +728,19 @@ UFSSwapDir::addDiskRestore(const cache_key * key,
     return e;
 }
 
+void
+UFSSwapDir::undoAddDiskRestore(StoreEntry *e)
+{
+    debugs(47, 5, HERE << *e);
+    replacementRemove(e); // checks swap_dirn so do it before we invalidate it
+    // Do not unlink the file as it might be used by a subsequent entry.
+    mapBitReset(e->swap_filen);
+    e->swap_filen = -1;
+    e->swap_dirn = -1;
+    cur_size -= fs.blksize * sizeInBlocks(e->swap_file_sz);
+    --n_disk_objects;
+}
+
 void
 UFSSwapDir::rebuild()
 {
@@ -1305,7 +1318,7 @@ UFSSwapDir::unlink(StoreEntry & e)
 {
     debugs(79, 3, "storeUfsUnlink: dirno " << index  << ", fileno "<<
            std::setfill('0') << std::hex << std::uppercase << std::setw(8) << e.swap_filen);
-    if (e.swap_status == SWAPOUT_DONE && EBIT_TEST(e.flags, ENTRY_VALIDATED)) {
+    if (e.swap_status == SWAPOUT_DONE) {
         cur_size -= fs.blksize * sizeInBlocks(e.swap_file_sz);
         --n_disk_objects;
     }
diff --git a/src/fs/ufs/ufscommon.cc b/src/fs/ufs/ufscommon.cc
index 266715e7f5..821806fa0a 100644
--- a/src/fs/ufs/ufscommon.cc
+++ b/src/fs/ufs/ufscommon.cc
@@ -567,25 +567,7 @@ RebuildState::rebuildFromSwapLog()
         currentEntry (Store::Root().get(swapData.key));
 
         if (currentEntry() != NULL && swapData.lastref >= e->lastref) {
-            /*
-             * Make sure we don't unlink the file, it might be
-             * in use by a subsequent entry.  Also note that
-             * we don't have to subtract from cur_size because
-             * adding to cur_size happens in the cleanup procedure.
-             */
-            currentEntry()->expireNow();
-            currentEntry()->releaseRequest();
-
-            if (currentEntry()->swap_filen > -1) {
-                UFSSwapDir *sdForThisEntry = dynamic_cast<UFSSwapDir *>(INDEXSD(currentEntry()->swap_dirn));
-                assert (sdForThisEntry);
-                sdForThisEntry->replacementRemove(currentEntry());
-                sdForThisEntry->mapBitReset(currentEntry()->swap_filen);
-                currentEntry()->swap_filen = -1;
-                currentEntry()->swap_dirn = -1;
-            }
-
-            currentEntry()->release();
+            undoAdd();
             counts.objcount--;
             counts.cancelcount++;
         }
@@ -682,19 +664,8 @@ RebuildState::rebuildFromSwapLog()
     } else if (currentEntry()) {
         /* key already exists, this swapfile not being used */
         /* junk old, load new */
-        currentEntry()->expireNow();
-        currentEntry()->releaseRequest();
-
-        if (currentEntry()->swap_filen > -1) {
-            UFSSwapDir *sdForThisEntry = dynamic_cast<UFSSwapDir *>(INDEXSD(currentEntry()->swap_dirn));
-            sdForThisEntry->replacementRemove(currentEntry());
-            /* Make sure we don't actually unlink the file */
-            sdForThisEntry->mapBitReset(currentEntry()->swap_filen);
-            currentEntry()->swap_filen = -1;
-            currentEntry()->swap_dirn = -1;
-        }
-
-        currentEntry()->release();
+        undoAdd();
+        counts.objcount--;
         counts.dupcount++;
     } else {
         /* URL doesnt exist, swapfile not in use */
@@ -718,6 +689,27 @@ RebuildState::rebuildFromSwapLog()
     storeDirSwapLog(currentEntry(), SWAP_LOG_ADD);
 }
 
+/// undo the effects of adding an entry in rebuildFromSwapLog()
+void
+RebuildState::undoAdd()
+{
+    StoreEntry *added = currentEntry();
+    assert(added);
+    currentEntry(NULL);
+
+    // TODO: Why bother with these two if we are going to release?!
+    added->expireNow();
+    added->releaseRequest();
+
+    if (added->swap_filen > -1) {
+        UFSSwapDir *sde = dynamic_cast<UFSSwapDir *>(INDEXSD(added->swap_dirn));
+        assert(sde);
+        sde->undoAddDiskRestore(added);
+    }
+
+    added->release();
+}
+
 int
 RebuildState::getNextFile(sfileno * filn_p, int *size)
 {
diff --git a/src/fs/ufs/ufscommon.h b/src/fs/ufs/ufscommon.h
index a5ea7f15e4..0ddde25967 100644
--- a/src/fs/ufs/ufscommon.h
+++ b/src/fs/ufs/ufscommon.h
@@ -104,6 +104,8 @@ public:
                                uint32_t refcount,
                                uint16_t flags,
                                int clean);
+    /// Undo the effects of UFSSwapDir::addDiskRestore().
+    void undoAddDiskRestore(StoreEntry *e);
     int validFileno(sfileno filn, int flag) const;
     int mapBitAllocate();
     virtual ConfigOption *getOptionTree() const;
@@ -404,6 +406,7 @@ private:
     void rebuildFromDirectory();
     void rebuildFromSwapLog();
     void rebuildStep();
+    void undoAdd();
     int getNextFile(sfileno *, int *size);
     StoreEntry *currentEntry() const;
     void currentEntry(StoreEntry *);
-- 
2.47.2