From: Marc Dionne <marc.c.dionne@gmail.com>
Date: Fri, 28 Jan 2011 00:07:32 +0000 (-0500)
Subject: ubik: always prefer a dirty cache page for write transactions
X-Git-Tag: openafs-devel-1_7_1~953
X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=296cf7d1630c57874b870e161bac5db9fe4dab48

ubik: always prefer a dirty cache page for write transactions

If a write transaction is running concurrently with a read transaction,
a DRead in the write transaction may return a clean cache page brought
in by the read transaction, instead of a previously written dirty page
from the same transaction.  This can result in loss of the written data.

Fix by making sure there is not a dirty version of the requested page
before returning a clean one.

Bug spotted by Jeffrey Hutzelman.

Change-Id: I20543693c98218d8ec1f791508d9404043819376
Reviewed-on: http://gerrit.openafs.org/3764
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
---

diff --git a/src/ubik/disk.c b/src/ubik/disk.c
index 58fa827..b250042 100644
--- a/src/ubik/disk.c
+++ b/src/ubik/disk.c
@@ -346,14 +346,16 @@ static char *
 DRead(struct ubik_trans *atrans, afs_int32 fid, int page)
 {
     /* Read a page from the disk. */
-    struct buffer *tb, *lastbuffer;
+    struct buffer *tb, *lastbuffer, *found_tb = NULL;
     afs_int32 code;
     struct ubik_dbase *dbase = atrans->dbase;
 
     calls++;
     lastbuffer = LruBuffer->lru_prev;
 
-    if (MatchBuffer(lastbuffer, page, fid, atrans)) {
+    /* Skip for write transactions for a clean page - this may not be the right page to use */
+    if (MatchBuffer(lastbuffer, page, fid, atrans)
+		&& (atrans->type == UBIK_READTRANS || lastbuffer->dirty)) {
 	tb = lastbuffer;
 	tb->lockers++;
 	lastb++;
@@ -361,11 +363,21 @@ DRead(struct ubik_trans *atrans, afs_int32 fid, int page)
     }
     for (tb = phTable[pHash(page)]; tb; tb = tb->hashNext) {
 	if (MatchBuffer(tb, page, fid, atrans)) {
-	    Dmru(tb);
-	    tb->lockers++;
-	    return tb->data;
+	    if (tb->dirty || atrans->type == UBIK_READTRANS) {
+		found_tb = tb;
+		break;
+	    }
+	    /* Remember this clean page - we might use it */
+	    found_tb = tb;
 	}
     }
+    /* For a write transaction, use a matching clean page if no dirty one was found */
+    if (found_tb) {
+	Dmru(found_tb);
+	found_tb->lockers++;
+	return found_tb->data;
+    }
+
     /* can't find it */
     tb = newslot(dbase, fid, page);
     if (!tb)