On Mon, May 13, 2019 at 05:43:55PM +0100, Filipe Manana wrote: > On Mon, May 13, 2019 at 5:31 PM David Sterba <[email protected]> wrote: > > > > On Mon, Apr 22, 2019 at 04:44:09PM +0100, [email protected] wrote: > > > From: Filipe Manana <[email protected]> > > > --- 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.
