Re: [PATCH] Btrfs: don't delete ordered roots from list during cleanup

[Josef Bacik <jbacik@fusionio.com>]

On Fri, Sep 27, 2013 at 04:18:14PM -0700, Zach Brown wrote:
> On Fri, Sep 27, 2013 at 04:37:46PM -0400, Josef Bacik wrote:
> > During transaction cleanup after an abort we are just removing roots from the
> > ordered roots list which is incorrect.  We have a BUG_ON() to make sure that the
> > root is still part of the ordered roots list when we put our ordered extent
> > which we were tripping in this case.  So do like we do everywhere else and just
> > move it to the tail of the ordered roots list and allow the normal cleanup to
> > take care of stuff.  Thanks,
> > 
> > Signed-off-by: Josef Bacik <jbacik@fusionio.com>
> > ---
> >  fs/btrfs/disk-io.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index f38211f..872b4ce 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3835,7 +3835,8 @@ static void btrfs_destroy_all_ordered_extents(struct btrfs_fs_info *fs_info)
> >  	while (!list_empty(&splice)) {
> >  		root = list_first_entry(&splice, struct btrfs_root,
> >  					ordered_root);
> > -		list_del_init(&root->ordered_root);
> > +		list_move_tail(&root->ordered_root,
> > +			       &fs_info->ordered_roots);
> 
> This function basically only does:
> 
> 	lock
> 	list_for_each
> 		lock
> 		list_for_each
> 			set_bit
> 
> Could we instead add a bit to the root or trans or fs_info or anything
> else that could be trivialy set in _destroy_all_ordered_extents and
> tested in _finish_ordered_io()?  It'd remove a bunch of tedious locking
> and iteration here.
> 
> The similar metaphor in the core page cache is (address_space->flags |
> AS_EIO).
> 
> Would that be too coarse or racey?

So I _think_ we may need to truncate the ordered range in the inode as well, but
I haven't had a consistent reproducer for this case.  I want to leave it like
this for now until I'm sure we don't need the truncate and then we could
probably just replace this with a test for FS_ERROR in finish_ordered_io.
Thanks,

Josef