[bug] label cli hangs until balance is completed

[bug] label cli hangs until balance is completed

[Anand Jain]

  '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 ?

Thanks,  Anand

Re: [bug] label cli hangs until balance is completed

[Stefan Behrens]

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:

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);
        mutex_unlock(&root->fs_info->volume_mutex);

Re: [bug] label cli hangs until balance is completed

[Zach Brown]

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

[PATCH] btrfs: fix get set label blocking against balance

[Anand Jain]

btrfs_ioctl_get_fslabel() and btrfs_ioctl_set_fslabel()
used root->fs_info->volume_mutex mutex which caused operations
like balance to block set/get label operation until its
completion and generally balance operation takes a long
time to complete, so it will be annoying to the user when
cli appears hung

This patch will use mutex uuid_mutex. which is defined
in volume.c, and so it is moved to volume.h as well.

also this patch will add a bit of optimization within
the btrfs_ioctl_get_falabel() function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c   | 16 ++++++++++------
 fs/btrfs/volumes.c |  1 -
 fs/btrfs/volumes.h |  1 +
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2177bea..d67d7d3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4043,17 +4043,21 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
 {
 	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);
+	char label_copy[BTRFS_LABEL_SIZE];
+	size_t	len;
 	int ret;
 
+	mutex_lock(&uuid_mutex);
+	memcpy(label_copy, label, BTRFS_LABEL_SIZE);
+	mutex_unlock(&uuid_mutex);
+
+	len = strnlen(label_copy, 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);
-	mutex_unlock(&root->fs_info->volume_mutex);
+	ret = copy_to_user(arg, label_copy, len);
 
 	return ret ? -EFAULT : 0;
 }
@@ -4082,7 +4086,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	mutex_lock(&root->fs_info->volume_mutex);
+	mutex_lock(&uuid_mutex);
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
@@ -4093,7 +4097,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	ret = btrfs_end_transaction(trans, root);
 
 out_unlock:
-	mutex_unlock(&root->fs_info->volume_mutex);
+	mutex_unlock(&uuid_mutex);
 	mnt_drop_write_file(file);
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 557a743..a5b3eb3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -49,7 +49,6 @@ static void __btrfs_reset_dev_stats(struct btrfs_device *dev);
 static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev);
 static void btrfs_dev_stat_print_on_load(struct btrfs_device *device);
 
-static DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);
 
 static void lock_chunks(struct btrfs_root *root)
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 8670558..7855ef9 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -26,6 +26,7 @@
 
 #define BTRFS_STRIPE_LEN	(64 * 1024)
 
+static DEFINE_MUTEX(uuid_mutex);
 struct buffer_head;
 struct btrfs_pending_bios {
 	struct bio *head;
-- 
1.8.1.227.g44fe835

Re: [PATCH] btrfs: fix get set label blocking against balance

[Stefan Behrens]

On Thu, 18 Jul 2013 19:18:18 +0800, Anand Jain wrote:
> btrfs_ioctl_get_fslabel() and btrfs_ioctl_set_fslabel()
> used root->fs_info->volume_mutex mutex which caused operations
> like balance to block set/get label operation until its
> completion and generally balance operation takes a long
> time to complete, so it will be annoying to the user when
> cli appears hung
> 
> This patch will use mutex uuid_mutex. which is defined
> in volume.c, and so it is moved to volume.h as well.
> 
> also this patch will add a bit of optimization within
> the btrfs_ioctl_get_falabel() function.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c   | 16 ++++++++++------
>  fs/btrfs/volumes.c |  1 -
>  fs/btrfs/volumes.h |  1 +
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 2177bea..d67d7d3 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4043,17 +4043,21 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
>  {
>  	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);
> +	char label_copy[BTRFS_LABEL_SIZE];
> +	size_t	len;
>  	int ret;
>  
> +	mutex_lock(&uuid_mutex);
> +	memcpy(label_copy, label, BTRFS_LABEL_SIZE);
> +	mutex_unlock(&uuid_mutex);
> +
> +	len = strnlen(label_copy, 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);
> -	mutex_unlock(&root->fs_info->volume_mutex);
> +	ret = copy_to_user(arg, label_copy, len);
>  
>  	return ret ? -EFAULT : 0;
>  }
> @@ -4082,7 +4086,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
>  	if (ret)
>  		return ret;
>  
> -	mutex_lock(&root->fs_info->volume_mutex);
> +	mutex_lock(&uuid_mutex);
>  	trans = btrfs_start_transaction(root, 0);
>  	if (IS_ERR(trans)) {
>  		ret = PTR_ERR(trans);
> @@ -4093,7 +4097,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
>  	ret = btrfs_end_transaction(trans, root);
>  
>  out_unlock:
> -	mutex_unlock(&root->fs_info->volume_mutex);
> +	mutex_unlock(&uuid_mutex);
>  	mnt_drop_write_file(file);
>  	return ret;
>  }
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 557a743..a5b3eb3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -49,7 +49,6 @@ static void __btrfs_reset_dev_stats(struct btrfs_device *dev);
>  static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev);
>  static void btrfs_dev_stat_print_on_load(struct btrfs_device *device);
>  
> -static DEFINE_MUTEX(uuid_mutex);
>  static LIST_HEAD(fs_uuids);
>  
>  static void lock_chunks(struct btrfs_root *root)
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 8670558..7855ef9 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -26,6 +26,7 @@
>  
>  #define BTRFS_STRIPE_LEN	(64 * 1024)
>  
> +static DEFINE_MUTEX(uuid_mutex);

^^^ Not like this. Everybody who includes volumes.h creates his own
instance of the mutex because of the "static" keyword. A shared mutex
cannot work like this. Functions in volumes.c and functions in ioctl.c
would use different mutexs, thus would not protect concurrent read/write
access.

And I'm sorry to say that I just realized that what I wrote yesterday
was nuisance, the label that btrfs_scan_one_device() accesses while
holding the uuid_mutex is not the one from the copy of the super block
that the fs_info structure contains, it's the one that is temporarily
read from disk. The whole idea to use the uuid_mutex was valueless and
nuisance. Instead the spinlock super_lock looks appropriate for
protecting concurrent access to the label field of fs_info->super_copy.
In btrfs_ioctl_set_fslabel() make sure to only hold the spinlock for the
copy operation, not while calling the transaction functions.


>  struct buffer_head;
>  struct btrfs_pending_bios {
>  	struct bio *head;
> 

Re: [PATCH] btrfs: fix get set label blocking against balance

[Anand Jain]

> Instead the spinlock super_lock looks appropriate for
> protecting concurrent access to the label field of fs_info->super_copy.
> In btrfs_ioctl_set_fslabel() make sure to only hold the spinlock for the
> copy operation, not while calling the transaction functions.

  Oh yeah ! fs_info->super_lock is most suitable.
  its right there. Thanks.

  New patch has been sent out.

Thanks.
Anand

[PATCH v2] btrfs: fix get set label blocking against balance

[Anand Jain]

btrfs_ioctl_get_fslabel() and btrfs_ioctl_set_fslabel()
used root->fs_info->volume_mutex mutex which caused operations
like balance to block set/get label operation until its
completion and generally balance operation takes a long
time to complete, so it will be annoying to the user when
cli appears hung

also this patch will add a bit of optimization within
the btrfs_ioctl_get_falabel() function.

v1->v2:
   use fs_info->super_lock instead of uuid_mutex

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2177bea..9a864f1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4042,18 +4042,22 @@ out:
 static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
 {
 	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;
+	char label[BTRFS_LABEL_SIZE];
+
+	spin_lock(&root->fs_info->super_lock);
+	memcpy(label, root->fs_info->super_copy->label, BTRFS_LABEL_SIZE);
+	spin_unlock(&root->fs_info->super_lock);
+
+	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);
-	mutex_unlock(&root->fs_info->volume_mutex);
 
 	return ret ? -EFAULT : 0;
 }
@@ -4082,18 +4086,18 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	mutex_lock(&root->fs_info->volume_mutex);
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out_unlock;
 	}
 
+	spin_lock(&root->fs_info->super_lock);
 	strcpy(super_block->label, label);
+	spin_unlock(&root->fs_info->super_lock);
 	ret = btrfs_end_transaction(trans, root);
 
 out_unlock:
-	mutex_unlock(&root->fs_info->volume_mutex);
 	mnt_drop_write_file(file);
 	return ret;
 }
-- 
1.8.1.227.g44fe835