| Message ID | 1374146298-30789-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)