| Message ID | 20230628132155.1560425-7-libaokun1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | quota: fix race condition between dqput() and dquot_mark_dquot_dirty() | expand |
On Wed 28-06-23 21:21:54, Baokun Li wrote: > Now when dqput() drops the last reference count, it will call > synchronize_srcu(&dquot_srcu) in quota_release_workfn() to ensure that > no other user will use the dquot after the last reference count is dropped, > so we don't need to call synchronize_srcu(&dquot_srcu) in drop_dquot_ref() > and remove the corresponding logic directly to simplify the code. Nice simplification! It is also important that dqput() now cannot sleep which was another reason for the logic with tofree_head in remove_inode_dquot_ref(). Probably this is good to mention in the changelog. > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > fs/quota/dquot.c | 33 ++++++--------------------------- > 1 file changed, 6 insertions(+), 27 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index e8e702ac64e5..df028fb2ce72 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1090,8 +1090,7 @@ static int add_dquot_ref(struct super_block *sb, int type) > * Remove references to dquots from inode and add dquot to list for freeing > * if we have the last reference to dquot > */ > -static void remove_inode_dquot_ref(struct inode *inode, int type, > - struct list_head *tofree_head) > +static void remove_inode_dquot_ref(struct inode *inode, int type) > { > struct dquot **dquots = i_dquot(inode); > struct dquot *dquot = dquots[type]; > @@ -1100,21 +1099,7 @@ static void remove_inode_dquot_ref(struct inode *inode, int type, > return; > > dquots[type] = NULL; > - if (list_empty(&dquot->dq_free)) { > - /* > - * The inode still has reference to dquot so it can't be in the > - * free list > - */ > - spin_lock(&dq_list_lock); > - list_add(&dquot->dq_free, tofree_head); > - spin_unlock(&dq_list_lock); > - } else { > - /* > - * Dquot is already in a list to put so we won't drop the last > - * reference here. > - */ > - dqput(dquot); > - } > + dqput(dquot); > } I think you can also just drop remove_inode_dquot_ref() as it is trivial now and inline it at its only callsite... Honza
On 2023/6/29 19:08, Jan Kara wrote: > On Wed 28-06-23 21:21:54, Baokun Li wrote: >> Now when dqput() drops the last reference count, it will call >> synchronize_srcu(&dquot_srcu) in quota_release_workfn() to ensure that >> no other user will use the dquot after the last reference count is dropped, >> so we don't need to call synchronize_srcu(&dquot_srcu) in drop_dquot_ref() >> and remove the corresponding logic directly to simplify the code. > Nice simplification! It is also important that dqput() now cannot sleep > which was another reason for the logic with tofree_head in > remove_inode_dquot_ref(). I don't understand this sentence very well, so I would appreciate it if you could explain it in detail.
On Thu 29-06-23 20:13:05, Baokun Li wrote: > On 2023/6/29 19:08, Jan Kara wrote: > > On Wed 28-06-23 21:21:54, Baokun Li wrote: > > > Now when dqput() drops the last reference count, it will call > > > synchronize_srcu(&dquot_srcu) in quota_release_workfn() to ensure that > > > no other user will use the dquot after the last reference count is dropped, > > > so we don't need to call synchronize_srcu(&dquot_srcu) in drop_dquot_ref() > > > and remove the corresponding logic directly to simplify the code. > > Nice simplification! It is also important that dqput() now cannot sleep > > which was another reason for the logic with tofree_head in > > remove_inode_dquot_ref(). > > I don't understand this sentence very well, so I would appreciate it > > if you could explain it in detail.
On 2023/6/29 22:09, Jan Kara wrote: > On Thu 29-06-23 20:13:05, Baokun Li wrote: >> On 2023/6/29 19:08, Jan Kara wrote: >>> On Wed 28-06-23 21:21:54, Baokun Li wrote: >>>> Now when dqput() drops the last reference count, it will call >>>> synchronize_srcu(&dquot_srcu) in quota_release_workfn() to ensure that >>>> no other user will use the dquot after the last reference count is dropped, >>>> so we don't need to call synchronize_srcu(&dquot_srcu) in drop_dquot_ref() >>>> and remove the corresponding logic directly to simplify the code. >>> Nice simplification! It is also important that dqput() now cannot sleep >>> which was another reason for the logic with tofree_head in >>> remove_inode_dquot_ref(). >> I don't understand this sentence very well, so I would appreciate it >> >> if you could explain it in detail.
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index e8e702ac64e5..df028fb2ce72 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1090,8 +1090,7 @@ static int add_dquot_ref(struct super_block *sb, int type) * Remove references to dquots from inode and add dquot to list for freeing * if we have the last reference to dquot */ -static void remove_inode_dquot_ref(struct inode *inode, int type, - struct list_head *tofree_head) +static void remove_inode_dquot_ref(struct inode *inode, int type) { struct dquot **dquots = i_dquot(inode); struct dquot *dquot = dquots[type]; @@ -1100,21 +1099,7 @@ static void remove_inode_dquot_ref(struct inode *inode, int type, return; dquots[type] = NULL; - if (list_empty(&dquot->dq_free)) { - /* - * The inode still has reference to dquot so it can't be in the - * free list - */ - spin_lock(&dq_list_lock); - list_add(&dquot->dq_free, tofree_head); - spin_unlock(&dq_list_lock); - } else { - /* - * Dquot is already in a list to put so we won't drop the last - * reference here. - */ - dqput(dquot); - } + dqput(dquot); } /* @@ -1137,8 +1122,7 @@ static void put_dquot_list(struct list_head *tofree_head) } } -static void remove_dquot_ref(struct super_block *sb, int type, - struct list_head *tofree_head) +static void remove_dquot_ref(struct super_block *sb, int type) { struct inode *inode; #ifdef CONFIG_QUOTA_DEBUG @@ -1159,7 +1143,7 @@ static void remove_dquot_ref(struct super_block *sb, int type, if (unlikely(inode_get_rsv_space(inode) > 0)) reserved = 1; #endif - remove_inode_dquot_ref(inode, type, tofree_head); + remove_inode_dquot_ref(inode, type); } spin_unlock(&dq_data_lock); } @@ -1176,13 +1160,8 @@ static void remove_dquot_ref(struct super_block *sb, int type, /* Gather all references from inodes and drop them */ static void drop_dquot_ref(struct super_block *sb, int type) { - LIST_HEAD(tofree_head); - - if (sb->dq_op) { - remove_dquot_ref(sb, type, &tofree_head); - synchronize_srcu(&dquot_srcu); - put_dquot_list(&tofree_head); - } + if (sb->dq_op) + remove_dquot_ref(sb, type); } static inline
Now when dqput() drops the last reference count, it will call synchronize_srcu(&dquot_srcu) in quota_release_workfn() to ensure that no other user will use the dquot after the last reference count is dropped, so we don't need to call synchronize_srcu(&dquot_srcu) in drop_dquot_ref() and remove the corresponding logic directly to simplify the code. Signed-off-by: Baokun Li <libaokun1@huawei.com> --- fs/quota/dquot.c | 33 ++++++--------------------------- 1 file changed, 6 insertions(+), 27 deletions(-)