[bug] label cli hangs until balance is completed

Commit Message

Stefan Behrens July 17, 2013, 9:09 a.m. UTC

On Wed, 17 Jul 2013 16:29:05 +0800, Anand Jain wrote:
>  'btrfs fi label /btrfs' will hang until balance is completed.
>  (and probably even set label would hang). This is because we
>  are trying to hold volume_mutex lock which balance will hold
>  during its tenure.
> 
> -------
> static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
> ::
>         mutex_lock(&root->fs_info->volume_mutex);
>         ret = copy_to_user(arg, label, len);
>         mutex_unlock(&root->fs_info->volume_mutex);
> --------
> 
>  I doubt if get label would need such a heavy weight lock?
>  Do we have any other lock which could fit better here ?
>  Any comments ?

Just use the uuid_mutex instead of the volume_mutex.

In addition to the btrfs_ioctl_get_fslabel() and
btrfs_ioctl_set_fslabel() functions, only btrfs_scan_one_device()
accesses the label. And btrfs_scan_one_device() holds the uuid_mutex,
not the volume_mutex. You can do the same.

And since cwillu's commit 1332a002b, the uuid_mutex is not hold for a
long time anymore.

And while you change it, please move the mutex_lock() so that it
includes also the strnlen() access to the label, like this:

        mutex_unlock(&root->fs_info->volume_mutex);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Zach Brown July 17, 2013, 5:49 p.m. UTC | #1

On Wed, Jul 17, 2013 at 11:09:46AM +0200, Stefan Behrens wrote:
> On Wed, 17 Jul 2013 16:29:05 +0800, Anand Jain wrote:
> >  'btrfs fi label /btrfs' will hang until balance is completed.
> >  (and probably even set label would hang). This is because we
> >  are trying to hold volume_mutex lock which balance will hold
> >  during its tenure.
> > 
> > -------
> > static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
> > ::
> >         mutex_lock(&root->fs_info->volume_mutex);
> >         ret = copy_to_user(arg, label, len);
> >         mutex_unlock(&root->fs_info->volume_mutex);
> > --------
> > 
> >  I doubt if get label would need such a heavy weight lock?
> >  Do we have any other lock which could fit better here ?
> >  Any comments ?
> 
> Just use the uuid_mutex instead of the volume_mutex.

I wouldn't copy_to_user() with the lock held.  That nests all of readpage
and mkwrite under the mutex creating more possibilities for deadlocks.

Whatever lock is chosen, I'd have get copy the label onto the kernel
stack before sending it to user space.

- z
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2177bea..30a8126 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4043,15 +4043,16 @@  static int btrfs_ioctl_get_fslabel(struct file
*file, vo
 {
        struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
        const char *label = root->fs_info->super_copy->label;
-       size_t len = strnlen(label, BTRFS_LABEL_SIZE);
+       size_t len;
        int ret;

+       mutex_lock(&root->fs_info->volume_mutex);
+       len = strnlen(label, BTRFS_LABEL_SIZE);
        if (len == BTRFS_LABEL_SIZE) {
                pr_warn("btrfs: label is too long, return the first %zu
bytes\n"
                        --len);
        }

-       mutex_lock(&root->fs_info->volume_mutex);
        ret = copy_to_user(arg, label, len);