On Mon, May 13, 2019 at 5:31 PM David Sterba <dsterba@xxxxxxx> wrote:
>
> On Mon, Apr 22, 2019 at 04:44:09PM +0100, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> > --- 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.
