Wang Shilong
2013-Mar-28 10:53 UTC
[PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> This patch introduces mutex lock ''quota_lock'', and makes all the user change for quota protected by quota_lock. Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> --- fs/btrfs/ctree.h | 3 +++ fs/btrfs/disk-io.c | 1 + fs/btrfs/ioctl.c | 16 ++++++++++++---- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 6e81860..a11a8ed 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1584,6 +1584,9 @@ struct btrfs_fs_info { struct rb_root qgroup_tree; spinlock_t qgroup_lock; + /* protect user change operations for quota */ + struct mutex quota_lock; + /* list of dirty qgroups to be written at next commit */ struct list_head dirty_qgroups; diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index fe82d08..4552f14 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2250,6 +2250,7 @@ int open_ctree(struct super_block *sb, mutex_init(&fs_info->dev_replace.lock); spin_lock_init(&fs_info->qgroup_lock); + mutex_init(&fs_info->quota_lock); fs_info->qgroup_tree = RB_ROOT; INIT_LIST_HEAD(&fs_info->dirty_qgroups); fs_info->qgroup_seq = 1; diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 222ce84..e2950f1 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent, if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0) goto out_up_read; - + mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock); if (snap_src) { error = create_snapshot(snap_src, dir, dentry, name, namelen, async_transid, readonly, inherit); @@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent, } if (!error) fsnotify_mkdir(dir, dentry); + mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock); out_up_read: up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem); out_dput: @@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) goto drop_write; } + mutex_lock(&root->fs_info->quota_lock); down_read(&root->fs_info->subvol_sem); if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) { trans = btrfs_start_transaction(root, 2); @@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) out: kfree(sa); up_read(&root->fs_info->subvol_sem); + mutex_unlock(&root->fs_info->quota_lock); drop_write: mnt_drop_write_file(file); return ret; @@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) goto drop_write; } + mutex_lock(&root->fs_info->quota_lock); trans = btrfs_join_transaction(root); if (IS_ERR(trans)) { ret = PTR_ERR(trans); @@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) out: kfree(sa); + mutex_unlock(&root->fs_info->quota_lock); drop_write: mnt_drop_write_file(file); return ret; @@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) ret = -EINVAL; goto out; } - + mutex_lock(&root->fs_info->quota_lock); trans = btrfs_join_transaction(root); if (IS_ERR(trans)) { ret = PTR_ERR(trans); - goto out; + goto out_unlock; } /* FIXME: check if the IDs really exist */ @@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) if (err && !ret) ret = err; +out_unlock: + mutex_unlock(&root->fs_info->quota_lock); out: kfree(sa); drop_write: @@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) ret = PTR_ERR(sa); goto drop_write; } - + mutex_lock(&root->fs_info->quota_lock); trans = btrfs_join_transaction(root); if (IS_ERR(trans)) { ret = PTR_ERR(trans); @@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) out: kfree(sa); + mutex_unlock(&root->fs_info->quota_lock); drop_write: mnt_drop_write_file(file); return ret; -- 1.7.7.6 -- 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
Arne Jansen
2013-Mar-30 23:41 UTC
Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
On 03/28/13 11:53, Wang Shilong wrote:> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > This patch introduces mutex lock ''quota_lock'', and makes > all the user change for quota protected by quota_lock. > > Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> > --- > fs/btrfs/ctree.h | 3 +++ > fs/btrfs/disk-io.c | 1 + > fs/btrfs/ioctl.c | 16 ++++++++++++---- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 6e81860..a11a8ed 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1584,6 +1584,9 @@ struct btrfs_fs_info { > struct rb_root qgroup_tree; > spinlock_t qgroup_lock; > > + /* protect user change operations for quota */ > + struct mutex quota_lock; > + > /* list of dirty qgroups to be written at next commit */ > struct list_head dirty_qgroups; > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index fe82d08..4552f14 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2250,6 +2250,7 @@ int open_ctree(struct super_block *sb, > mutex_init(&fs_info->dev_replace.lock); > > spin_lock_init(&fs_info->qgroup_lock); > + mutex_init(&fs_info->quota_lock); > fs_info->qgroup_tree = RB_ROOT; > INIT_LIST_HEAD(&fs_info->dirty_qgroups); > fs_info->qgroup_seq = 1; > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 222ce84..e2950f1 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent, > > if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0) > goto out_up_read; > - > + mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock); > if (snap_src) { > error = create_snapshot(snap_src, dir, dentry, name, namelen, > async_transid, readonly, inherit); > @@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent, > } > if (!error) > fsnotify_mkdir(dir, dentry); > + mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock);You are completely serializing subvolume operations here. I''d prefer if you''d move the lock to a lower level to only protect the quota operations. Can''t you move the lock completely to qgroup.c?> out_up_read: > up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem); > out_dput: > @@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) > goto drop_write; > } > > + mutex_lock(&root->fs_info->quota_lock); > down_read(&root->fs_info->subvol_sem); > if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) { > trans = btrfs_start_transaction(root, 2); > @@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) > out: > kfree(sa); > up_read(&root->fs_info->subvol_sem); > + mutex_unlock(&root->fs_info->quota_lock); > drop_write: > mnt_drop_write_file(file); > return ret; > @@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) > goto drop_write; > } > > + mutex_lock(&root->fs_info->quota_lock); > trans = btrfs_join_transaction(root); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > @@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) > > out: > kfree(sa); > + mutex_unlock(&root->fs_info->quota_lock); > drop_write: > mnt_drop_write_file(file); > return ret; > @@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) > ret = -EINVAL; > goto out; > } > - > + mutex_lock(&root->fs_info->quota_lock); > trans = btrfs_join_transaction(root); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > - goto out; > + goto out_unlock; > } > > /* FIXME: check if the IDs really exist */ > @@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) > if (err && !ret) > ret = err; > > +out_unlock: > + mutex_unlock(&root->fs_info->quota_lock); > out: > kfree(sa); > drop_write: > @@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) > ret = PTR_ERR(sa); > goto drop_write; > } > - > + mutex_lock(&root->fs_info->quota_lock); > trans = btrfs_join_transaction(root); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > @@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) > > out: > kfree(sa); > + mutex_unlock(&root->fs_info->quota_lock); > drop_write: > mnt_drop_write_file(file); > return ret; >-- 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
Wang Shilong
2013-Mar-31 01:12 UTC
Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
Hello Arne,> On 03/28/13 11:53, Wang Shilong wrote: >> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> >> >> This patch introduces mutex lock ''quota_lock'', and makes >> all the user change for quota protected by quota_lock. >> >> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> >> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> >> --- >> fs/btrfs/ctree.h | 3 +++ >> fs/btrfs/disk-io.c | 1 + >> fs/btrfs/ioctl.c | 16 ++++++++++++---- >> 3 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >> index 6e81860..a11a8ed 100644 >> --- a/fs/btrfs/ctree.h >> +++ b/fs/btrfs/ctree.h >> @@ -1584,6 +1584,9 @@ struct btrfs_fs_info { >> struct rb_root qgroup_tree; >> spinlock_t qgroup_lock; >> >> + /* protect user change operations for quota */ >> + struct mutex quota_lock; >> + >> /* list of dirty qgroups to be written at next commit */ >> struct list_head dirty_qgroups; >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index fe82d08..4552f14 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -2250,6 +2250,7 @@ int open_ctree(struct super_block *sb, >> mutex_init(&fs_info->dev_replace.lock); >> >> spin_lock_init(&fs_info->qgroup_lock); >> + mutex_init(&fs_info->quota_lock); >> fs_info->qgroup_tree = RB_ROOT; >> INIT_LIST_HEAD(&fs_info->dirty_qgroups); >> fs_info->qgroup_seq = 1; >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 222ce84..e2950f1 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent, >> >> if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0) >> goto out_up_read; >> - >> + mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock); >> if (snap_src) { >> error = create_snapshot(snap_src, dir, dentry, name, namelen, >> async_transid, readonly, inherit); >> @@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent, >> } >> if (!error) >> fsnotify_mkdir(dir, dentry); >> + mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock); > > You are completely serializing subvolume operations here. I''d prefer if > you''d move the lock to a lower level to only protect the quota > operations. Can''t you move the lock completely to qgroup.c?I have thought about this. It really should not start a transaction if the checks fail.So i have all the necessary checks before a transaction! For user quota change operations, i think it is ok to serialize the operations. The only place may be create_snapshot()/create_subvolume(). Maybe i can move the mutex_lock to the place where btrfs_qgroup_inherit() is called. Thanks, wang> >> out_up_read: >> up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem); >> out_dput: >> @@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) >> goto drop_write; >> } >> >> + mutex_lock(&root->fs_info->quota_lock); >> down_read(&root->fs_info->subvol_sem); >> if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) { >> trans = btrfs_start_transaction(root, 2); >> @@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg) >> out: >> kfree(sa); >> up_read(&root->fs_info->subvol_sem); >> + mutex_unlock(&root->fs_info->quota_lock); >> drop_write: >> mnt_drop_write_file(file); >> return ret; >> @@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) >> goto drop_write; >> } >> >> + mutex_lock(&root->fs_info->quota_lock); >> trans = btrfs_join_transaction(root); >> if (IS_ERR(trans)) { >> ret = PTR_ERR(trans); >> @@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) >> >> out: >> kfree(sa); >> + mutex_unlock(&root->fs_info->quota_lock); >> drop_write: >> mnt_drop_write_file(file); >> return ret; >> @@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) >> ret = -EINVAL; >> goto out; >> } >> - >> + mutex_lock(&root->fs_info->quota_lock); >> trans = btrfs_join_transaction(root); >> if (IS_ERR(trans)) { >> ret = PTR_ERR(trans); >> - goto out; >> + goto out_unlock; >> } >> >> /* FIXME: check if the IDs really exist */ >> @@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg) >> if (err && !ret) >> ret = err; >> >> +out_unlock: >> + mutex_unlock(&root->fs_info->quota_lock); >> out: >> kfree(sa); >> drop_write: >> @@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) >> ret = PTR_ERR(sa); >> goto drop_write; >> } >> - >> + mutex_lock(&root->fs_info->quota_lock); >> trans = btrfs_join_transaction(root); >> if (IS_ERR(trans)) { >> ret = PTR_ERR(trans); >> @@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg) >> >> out: >> kfree(sa); >> + mutex_unlock(&root->fs_info->quota_lock); >> drop_write: >> mnt_drop_write_file(file); >> return ret; >> >-- 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
Jan Schmidt
2013-Apr-02 12:19 UTC
Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
Hi Wang, On Thu, March 28, 2013 at 11:53 (+0100), Wang Shilong wrote:> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > > This patch introduces mutex lock ''quota_lock'', and makes > all the user change for quota protected by quota_lock.Can you please add a few lines why this lock is needed? I.e., which ioctls fail without that kind of synchronization?> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> > Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> > --- > fs/btrfs/ctree.h | 3 +++ > fs/btrfs/disk-io.c | 1 + > fs/btrfs/ioctl.c | 16 ++++++++++++---- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 6e81860..a11a8ed 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1584,6 +1584,9 @@ struct btrfs_fs_info { > struct rb_root qgroup_tree; > spinlock_t qgroup_lock; > > + /* protect user change operations for quota */ > + struct mutex quota_lock;Having fs_info->qgroup_lock and fs_info->quota_lock going to be a major source of confusion. I''d call the new one qgroup_ioctl_lock or ioctl_qgroup_lock instead. Furthermore, the term "quota" was intentionally left unused to leave room for other quota implementations later (user quota). And, please, use --thread with git send-email for related patches to get correct headers. Thanks, -Jan> [snip]-- 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
Wang Shilong
2013-Apr-02 12:26 UTC
Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
Hello Jan,> Hi Wang, > > On Thu, March 28, 2013 at 11:53 (+0100), Wang Shilong wrote: >> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com> >> >> This patch introduces mutex lock ''quota_lock'', and makes >> all the user change for quota protected by quota_lock. > > Can you please add a few lines why this lock is needed? I.e., which ioctls fail > without that kind of synchronization?I have explained it clearly in [PATCH V2 0/6]. http://marc.info/?l=linux-btrfs&m=136446806332680&w=2> >> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com> >> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> >> --- >> fs/btrfs/ctree.h | 3 +++ >> fs/btrfs/disk-io.c | 1 + >> fs/btrfs/ioctl.c | 16 ++++++++++++---- >> 3 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >> index 6e81860..a11a8ed 100644 >> --- a/fs/btrfs/ctree.h >> +++ b/fs/btrfs/ctree.h >> @@ -1584,6 +1584,9 @@ struct btrfs_fs_info { >> struct rb_root qgroup_tree; >> spinlock_t qgroup_lock; >> >> + /* protect user change operations for quota */ >> + struct mutex quota_lock; > > Having fs_info->qgroup_lock and fs_info->quota_lock going to be a major source > of confusion. I''d call the new one qgroup_ioctl_lock or ioctl_qgroup_lock instead.It is reasonable, but i will move the mutex lock to group.c to avoid serialize operations.> > Furthermore, the term "quota" was intentionally left unused to leave room for > other quota implementations later (user quota). > > And, please, use --thread with git send-email for related patches to get correct > headers.Thanks for correcting me~. Thanks, Wang> > Thanks, > -Jan > >> [snip]-- 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