Re: [PATCH] Btrfs: prevent send failures and crashes due to concurrent relocation

[David Sterba <dsterba@suse.cz>]

On Mon, May 13, 2019 at 05:43:55PM +0100, Filipe Manana wrote:
> On Mon, May 13, 2019 at 5:31 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Mon, Apr 22, 2019 at 04:44:09PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > > --- a/fs/btrfs/send.c
> > > +++ b/fs/btrfs/send.c
> > > @@ -6869,9 +6869,23 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
> > >       if (ret)
> > >               goto out;
> > >
> > > +     mutex_lock(&fs_info->balance_mutex);
> > > +     if (test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
> > > +             mutex_unlock(&fs_info->balance_mutex);
> > > +             btrfs_warn_rl(fs_info,
> > > +           "Can not run send because a balance operation is in progress");
> > > +             ret = -EAGAIN;
> > > +             goto out;
> > > +     }
> > > +     fs_info->send_in_progress++;
> > > +     mutex_unlock(&fs_info->balance_mutex);
> >
> > This would be better in a helper that hides that the balance mutex from
> > send.
> 
> Given the large number of cleanup patches that open code helpers that
> had only one caller, this somewhat surprises me.

Fair point, though I'd object that there are cases where the function
name says in short what happens without the implementation details and
this helps code readability. I struck me when I saw 'send_in_progress
protected by balance_mutex'. You can find functions that are called just
once, that's not an anti-pattern in general.

I'll take a fresh look later, the setup phase of btrfs_ioctl_send is not
exactly short so the added check does not stand out.