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

From: Filipe Manana <fdmanana@kernel.org>
To: dsterba@suse.cz, Filipe Manana <fdmanana@kernel.org>,
	linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH] Btrfs: prevent send failures and crashes due to concurrent relocation
Date: Mon, 13 May 2019 17:43:55 +0100	[thread overview]
Message-ID: <CAL3q7H4LD1F=D7ERBNTeSTBWUOTnTS-oyBoN3KVBV-uZ0t+QLg@mail.gmail.com> (raw)
In-Reply-To: <20190513163216.GF3138@twin.jikos.cz>

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.
Same for the other similar comments below.

>
> eg.
>
>         if (!btrfs_send_can_start(fs_info)
>                 return -EAGAIN;
>
> > +
> >       current->journal_info = BTRFS_SEND_TRANS_STUB;
> >       ret = send_subvol(sctx);
> >       current->journal_info = NULL;
> > +     mutex_lock(&fs_info->balance_mutex);
> > +     fs_info->send_in_progress--;
> > +     mutex_unlock(&fs_info->balance_mutex);
>
>         btrfs_send_end();
>
> >       if (ret < 0)
> >               goto out;
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index db934ceae9c1..8145b62e3912 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -4203,6 +4203,14 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
> >                          get_raid_name(meta_index), get_raid_name(data_index));
> >       }
> >
> > +     if (fs_info->send_in_progress) {
> > +             btrfs_warn_rl(fs_info,
> > +"Can not run balance while send operations are in progress (%d in progress)",
> > +                           fs_info->send_in_progress);
> > +             ret = -EAGAIN;
> > +             goto out;
> > +     }
>
> Similar here.
>
> As the operation compatibility is done on the filesystem level, it would
> be better to hide all the logic in helpers, now that there's more than
> the per-subvolume send_in_progress.