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 96bcf9dffe5d449cebd6d2818144195b63c0f8d9..13078e40b4907e8af8c9d49422a5619e88aef60d 100644 (file)
--- 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 0609e32848d97de2492fcae5e88dafcc6fe870c3..2e3f45a5a445cff6e8c740122ffbbb72ac6db14b 100644 (file)
--- 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