From: NeilBrown <neilb@suse.de>
Date: Mon, 28 Jun 2010 02:19:39 +0000 (+1000)
Subject: Fix races between truncate and cleaner.
X-Git-Url: http://git.neil.brown.name/?a=commitdiff_plain;h=9669d8485b9bc65c3bba68ed29ed211c73e1510a;p=LaFS.git

Fix races between truncate and cleaner.

Not only do we need to recheck the size after putting
the block on the clean list, we also need to check
for inodes that have been cleared. (type == 0).

Signed-off-by: NeilBrown <neilb@suse.de>
---

diff --git a/README b/README
index 96bcf9d..13078e4 100644
--- a/README
+++ b/README
@@ -4816,17 +4816,17 @@ DONE 5d/ At unmount, 16/1 is still pinned.
  
   Would we be spinning on -EAGAIN ?? 4 empty segment are present.
 
- 7/ block.c:624 in lafs_dirty_iblock - no pin, no credits
+DONE 7/ block.c:624 in lafs_dirty_iblock - no pin, no credits
    truncate -> lafs_invalidate_page -> lafs_erase_dblock -> lafs_allocated_block / lafs_dirty_iblock
 Allocated [ce44f240]327/144(1499)r2E:Writeback,PhysValid clean2(1) cleaning(1) -> 0
 SEGMOVE 1499 0
 Oh dear: [ce44f240]327/144(0)r2E:Writeback,PhysValid clean2(1) cleaning(1)
 .......: [cfb69180]327/0(349)r2F:Index(1),Pinned,Phase0,Valid,PhysValid{0,0}[0] child(1) leaf(1)
 Why have I no credits?
-/home/neilb/work/nfsbrick/fs/module/block.c:624: [cfb69180]327/0(349)r2F:Index(1),Pinned,Phase0,Valid,Dirty,PhysValid{0,0}[0] child(1
-) leaf(1)
+/home/neilb/work/nfsbrick/fs/module/block.c:624: [cfb69180]327/0(349)r2F:Index(1),Pinned,Phase0,Valid,Dirty,PhysValid{0,0}[0] child(1) leaf(1)
       
    Cleaning is racing with truncate, and that cannot happen!!
+   Actually it could - if i_size changed at the wrong time.
 
 DONE 7a/ block.c:507 in lafs_dirty_dblock - no credits for 0/2
    block.c:507: [cfa63c58]0/2(4348)r2F:Valid,Dirty,Writeback,PhysValid cluster(1) iblock(1)
@@ -4859,7 +4859,7 @@ DONE 7a/ block.c:507 in lafs_dirty_dblock - no credits for 0/2
       ->uninc blocks.  This is during truncate.  Need some
       interlock with cleaner maybe?
 
- 7h/ truncate finds children - Realloc on clean-leafs
+DONE 7h/ inode.c:845 truncate finds children - Realloc on clean-leafs
 
  7j/ resolve space allocation issues.
     Understand why CleanSpace can be tried and failed 1000
diff --git a/clean.c b/clean.c
index 0609e32..2e3f45a 100644
--- a/clean.c
+++ b/clean.c
@@ -412,19 +412,32 @@ static int try_clean(struct fs *fs, struct toclean *tc)
 			 * properly.  We don't want to even 'get' a block beyond
 			 * EOF, certainly not after the truncate_inode_pages.
 			 */
-			if (LAFSI(ino)->type >= TypeBase &&
-			    ((loff_t)bnum << ino->i_blkbits) >= i_size_read(ino))
+			if (LAFSI(ino)->type == 0 ||
+			    (LAFSI(ino)->type >= TypeBase &&
+			     ((loff_t)bnum << ino->i_blkbits) >= i_size_read(ino)))
 				goto skip;
 			b = lafs_get_block(ino, bnum, NULL, GFP_NOFS,
 					   MKREF(cleaning));
 			if (b == NULL)
 				break;
-			/* FIXME repeat size test?  Is any locking appropriate? */
+
 			if (list_empty(&b->cleaning)) {
 				list_add(&b->cleaning, &tc->cleaning);
+				getdref(b, MKREF(cleaning));
 				igrab(ino);
-			} else
+			}
+			/* FIXME do I need a memory barrier. to ensure truncate
+			 * sees the not-list_empty, and we see i_size? */
+
+			if (LAFSI(ino)->type == 0 ||
+			    (LAFSI(ino)->type >= TypeBase &&
+			     ((loff_t)bnum << ino->i_blkbits)
+			     >= i_size_read(ino))) {
+				iput(ino);
+				list_del_init(&b->cleaning);
 				putdref(b, MKREF(cleaning));
+			}
+			putdref(b, MKREF(cleaning));
 		}
 	skip:
 		/* We modify the descriptor in-place to track where