Skip to contentSkip to conversation

label cli hangs until balance is completed

bug Superseded Anand Jain 3 participants 7 messages Jul 17, 2013 – Jul 19, 2013 Btrfs file system

lore
Share & tools
lore

Anand Jain

author
#0 lore raw

'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

#1 lore raw
Quoted 8 lines · 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) ::

Quoted 8 lines
        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:

fs/btrfs/ioctl.c +0 -0
index 2177bea..30a8126 100644
@@ -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);
#2 lore raw
Quoted 14 lines · Stefan Behrens
Quoted 13 lines · 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);

--------

Quoted 5 lines
Quoted 3 lines

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

author
#3 lore raw

PATCH 1/1 btrfs: fix get set label blocking against balance

In reply to 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 <[email protected]>
---
 fs/btrfs/ioctl.c   | 16 ++++++++++------
 fs/btrfs/volumes.c |  1 -
 fs/btrfs/volumes.h |  1 +
 3 files changed, 11 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +10 -6
index 2177bea..d67d7d3 100644
@@ -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;
 }
fs/btrfs/volumes.c +0 -1
index 557a743..a5b3eb3 100644
@@ -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)
fs/btrfs/volumes.h +1 -0
index 8670558..7855ef9 100644
@@ -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;
Signature · 1 line
1.8.1.227.g44fe835
#4 lore raw

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

Quoted 85 lines · 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 <[email protected]>
---
 fs/btrfs/ioctl.c   | 16 ++++++++++------
 fs/btrfs/volumes.c |  1 -
 fs/btrfs/volumes.h |  1 +
 3 files changed, 11 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +10 -6
index 2177bea..d67d7d3 100644
@@ -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;
 }
fs/btrfs/volumes.c +0 -1
index 557a743..a5b3eb3 100644
@@ -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)
fs/btrfs/volumes.h +0 -0
index 8670558..7855ef9 100644

fs/btrfs/volumes.h line 29

Quoted 4 lines
fs/btrfs/volumes.h +1 -0
@@ -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.

Quoted 4 lines
fs/btrfs/volumes.h +0 -0
 struct buffer_head;
 struct btrfs_pending_bios {
 	struct bio *head;
author
#5 lore raw

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

Quoted 6 lines

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

author
#6 lore raw

PATCH v2 1/1 btrfs: fix get set label blocking against balance

In reply to 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 <[email protected]>
---
 fs/btrfs/ioctl.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
fs/btrfs/ioctl.c +10 -6
index 2177bea..9a864f1 100644
@@ -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;
 }
Signature · 1 line
1.8.1.227.g44fe835