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

[David Sterba <dsterba@suse.cz>]

On Thu, Jun 13, 2019 at 10:24:05AM +0100, Filipe Manana wrote:
> On Thu, Jun 6, 2019 at 2:24 PM Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Mon, May 13, 2019 at 6:59 PM David Sterba <dsterba@suse.cz> wrote:
> > >
> > > 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.
> >
> > So, several weeks passed, and this prevents a quite serious bug from happening.
> > Any progress on that or was I supposed to do something about it?
> >
> > Thanks.
> 
> Ping.

Sorry, missed that. I'll apply the patch without changes, the open coding
of the check is not a big deal.