Re: [PATCH v3] btrfs: clean snapshots one by one

[Chris Mason <clmason@fusionio.com>]

Quoting David Sterba (2013-05-07 07:54:49)
> On Mon, May 06, 2013 at 08:41:06PM -0400, Chris Mason wrote:
> > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > > index 988b860..4de2351 100644
> > > --- a/fs/btrfs/disk-io.c
> > > +++ b/fs/btrfs/disk-io.c
> > > @@ -1690,15 +1690,19 @@ static int cleaner_kthread(void *arg)
> > >         struct btrfs_root *root = arg;
> > >  
> > >         do {
> > > +               int again = 0;
> > > +
> > >                 if (!(root->fs_info->sb->s_flags & MS_RDONLY) &&
> > > +                   down_read_trylock(&root->fs_info->sb->s_umount) &&
> > >                     mutex_trylock(&root->fs_info->cleaner_mutex)) {
> > >                         btrfs_run_delayed_iputs(root);
> > > -                       btrfs_clean_old_snapshots(root);
> > > +                       again = btrfs_clean_one_deleted_snapshot(root);
> > >                         mutex_unlock(&root->fs_info->cleaner_mutex);
> > >                         btrfs_run_defrag_inodes(root->fs_info);
> > > +                       up_read(&root->fs_info->sb->s_umount);
> > 
> > Can we use just the cleaner mutex for this?  We're deadlocking during
> > 068 with autodefrag on because the cleaner is holding s_umount while
> > autodefrag is trying to bump the writer count.
> 
> I have now reproduced the deadlock and see where it's stuck.  It did not
> happen with running 068 in a loop, but after interrupting the test.

Hmmm, interrupting the test may just mean you've left it frozen?  It
happens every time for me with autodefrag on.

> 
> > If unmount takes the cleaner mutex once it should wait long enough for
> > the cleaner to stop.
> 
> You mean removing s_umount from here completely? I'm not sure about
> other mis-interaction, eg with remount + autodefrag. Miao sent a patch
> for that case http://www.spinics.net/lists/linux-btrfs/msg16634.html
> (but it would not fix this deadlock).

Mostly we need to pull the run_defrag_inodes out of the s_umount.  It
may be much smarter to put that into a dedicated worker pool.

> 
> I'm for keeping the clean-by-one patch for 3.10, we can fix other
> regressions during rc cycle.

I do agree, and left it in the pull that Linus took.  Thanks for working
on this one.

-chris