git » libjio » commit a2cc3f4

This fixes a very subtle (and not so hard to hit) race in the transaction id handling. Bonnie makes it appear pretty easily.

This fixes a very subtle (and not so hard to hit) race in the transaction id handling. Bonnie makes it appear pretty easily.

This fixes a very subtle (and not so hard to hit) race in the transaction id
handling. Bonnie makes it appear pretty easily.

Before we free a TID, the representing file is unlinked. Then, we call
free_tid() which updates the lock accordingly.

The following series of events can lead to an erroneous (and time consuming)
loop in free_tid():

thread1			thread2			thread3
new trans, tid = 1	new trans, tid = 2	new trans, tid = 3
...			...
unlink(file1)		unlink(file2)		unlink(file3)
lock lockfile (OK)	lock lockfile (block)	lock lockfile (block
update lockfile
   maxtid remains 3
unlock lockfile		(keeps blocked)		lock success
						update lockfile
						  we're the max tid, so we
						  scan the journal directory
						  to find out the next max
						  tid; it's emtpy, so we set
						  it to 0
						unlock lockfile
			lock success
			update lockfile
			  we're max tid, so we
			  scan the journal
			  directory to find out
			  the next max tid. BUG!

At that point we stop and look the code in free_tid():

	for (i = curid - 1; i > 0; i--) {

but hey, curid is 0 (thread3 set it earlier) so we overflow and have to scan
all the entire range of numbers with access() before we exit the loop. It can
take a few hours, and trash all your nice caches (dentry cache will grow a
lot, but this depends of course on the OS).

There are other related or similar races that I won't explain here, but are
pretty similar in spirit.

The fix is to modify the check earlier, and scan only if we are the max tid.

I'll add a document on the transaction ID handling process later.

diff --git a/trans.c b/trans.c
index f1ba9c7..3af6287 100644
--- a/trans.c
+++ b/trans.c
@@ -63,10 +63,9 @@ static void free_tid(struct jfs *fs, unsigned int tid)
 	/* read the current max. curid */
 	curid = *(fs->jmap);
 
-	if (tid < curid) {
-		/* we're not freeing the max. curid, so we just return */
-		goto exit;
-	} else {
+	/* if we're the max tid, scan the directory looking up for the new
+	 * max; the detailed description can be found in the "doc/" dir */
+	if (tid == curid) {
 		/* look up the new max. */
 		for (i = curid - 1; i > 0; i--) {
 			/* this can fail if we're low on mem, but we don't
@@ -83,7 +82,6 @@ static void free_tid(struct jfs *fs, unsigned int tid)
 		*(fs->jmap) = i;
 	}
 
-exit:
 	plockf(fs->jfd, F_UNLOCK, 0, 0);
 	return;
 }