Message ID | 20230628132155.1560425-7-libaokun1@huawei.com |
---|---|
State | Superseded |
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. 🤔 > 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 Sure, I'll just remove it in the next version and inline the only remaining code into remove_dquot_ref(). Thanks!
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. 🤔 OK, let me phrase it in a "changelog" way :): remove_inode_dquot_ref() currently does not release the last dquot reference but instead adds the dquot to tofree_head list. This is because dqput() can sleep while dropping of the last dquot reference (writing back the dquot and calling ->release_dquot()) and that must not happen under dq_list_lock. Now that dqput() queues the final dquot cleanup into a workqueue, remove_inode_dquot_ref() can call dqput() unconditionally and we can significantly simplify it. Honza
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. 🤔 > OK, let me phrase it in a "changelog" way :): > > remove_inode_dquot_ref() currently does not release the last dquot > reference but instead adds the dquot to tofree_head list. This is because > dqput() can sleep while dropping of the last dquot reference (writing back > the dquot and calling ->release_dquot()) and that must not happen under > dq_list_lock. Now that dqput() queues the final dquot cleanup into a > workqueue, remove_inode_dquot_ref() can call dqput() unconditionally > and we can significantly simplify it. > > Honza I suddenly understand what you mean, you mean that now dqput() doesn't have any possible sleep operation. So it can be called in spin_lock at will. I was confused because I understood that dqput() cannot be called in the context of possible sleep. Thank you very much for the detailed explanation! Now there is only the problem in patch 5. Thanks!
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(-)