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@xxxxxxxxxx>
> ---
> 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;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
