On Mon, May 27, 2013 at 02:07:00PM +0200, David Sterba wrote: > On Sun, May 26, 2013 at 09:50:27PM +0800, Liu Bo wrote: > > Commit be283b2e674a09457d4563729015adb637ce7cc1 > > ( Btrfs: use helper to cleanup tree roots) introduced the following bug, > > Well, it did not introduce the bug, but made it visible. > > > We've free'ed commit_root before actually getting to free block groups where > > caching thread needs valid extent_root->commit_root. > > > > Signed-off-by: Liu Bo <bo.li....@oracle.com> > > --- > > fs/btrfs/disk-io.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index e7b3cb5..a5c8f28 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -3512,10 +3512,10 @@ int close_ctree(struct btrfs_root *root) > > percpu_counter_sum(&fs_info->delalloc_bytes)); > > } > > > > - free_root_pointers(fs_info, 1); > > - > > btrfs_free_block_groups(fs_info); > > > > + free_root_pointers(fs_info, 1); > > + > > del_fs_roots(fs_info); > > > > iput(fs_info->btree_inode); > > This makes it just harder to hit, but the worker threads that get > stopped after iput may still access the freed roots, like mentioned here > > http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg24239.html > > david
Hi David, The original code just free_extent_buffer(commit_root), so the bug proves that there can still be a reference on commit_root during the period between freeing tree roots and stopping workers. I think we should add a parameter for free_root_pointers() to tell it not set root->commit_root NULL. If you agree on this, I can make a patch :) (I did spend a lot of time to reproduce this with xfstests as you showed, but I failed somehow, the magic is that I reproduced it while testing the dedup patch set, lol) thanks, liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
