| Message ID | 20230628132155.1560425-3-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:50, Baokun Li wrote: > Add a new global dquot list that obeys the following rules: > > 1). A dquot is added to this list when its last reference count is about > to be dropped. > 2). The reference count of the dquot in the list is greater than or equal > to 1 ( due to possible race with dqget()). > 3). When a dquot is removed from this list, a reference count is always > subtracted, and if the reference count is then 0, the dquot is added > to the free_dquots list. > > This list is used to safely perform the final cleanup before releasing > the last reference count, to avoid various contention issues caused by > performing cleanup directly in dqput(), and to avoid the performance impact > caused by calling synchronize_srcu(&dquot_srcu) directly in dqput(). Here > it is just defining the list and implementing the corresponding operation > function, which we will use later. > > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Baokun Li <libaokun1@huawei.com> I think you can merge this patch with patch 5. It is not like separating this bit helps in review or anything... > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 108ba9f1e420..a8b43b5b5623 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -226,12 +226,21 @@ static void put_quota_format(struct quota_format_type *fmt) > /* > * Dquot List Management: > * The quota code uses four lists for dquot management: the inuse_list, ^^^ five now :) > - * free_dquots, dqi_dirty_list, and dquot_hash[] array. A single dquot > - * structure may be on some of those lists, depending on its current state. > + * releasing_dquots, free_dquots, dqi_dirty_list, and dquot_hash[] array. > + * A single dquot structure may be on some of those lists, depending on > + * its current state. > * > * All dquots are placed to the end of inuse_list when first created, and this > * list is used for invalidate operation, which must look at every dquot. > * > + * When the last reference of a dquot will be dropped, the dquot will be > + * added to releasing_dquots. We'd then queue work item which would call > + * synchronize_srcu() and after that perform the final cleanup of all the > + * dquots on the list. Both releasing_dquots and free_dquots use the > + * dq_free list_head in the dquot struct. when a dquot is removed from ^^^ Capital W please > + * releasing_dquots, a reference count is always subtracted, and if > + * dq_count == 0 at that point, the dquot will be added to the free_dquots. > + * Honza
On 2023/6/29 18:29, Jan Kara wrote: > On Wed 28-06-23 21:21:50, Baokun Li wrote: >> Add a new global dquot list that obeys the following rules: >> >> 1). A dquot is added to this list when its last reference count is about >> to be dropped. >> 2). The reference count of the dquot in the list is greater than or equal >> to 1 ( due to possible race with dqget()). >> 3). When a dquot is removed from this list, a reference count is always >> subtracted, and if the reference count is then 0, the dquot is added >> to the free_dquots list. >> >> This list is used to safely perform the final cleanup before releasing >> the last reference count, to avoid various contention issues caused by >> performing cleanup directly in dqput(), and to avoid the performance impact >> caused by calling synchronize_srcu(&dquot_srcu) directly in dqput(). Here >> it is just defining the list and implementing the corresponding operation >> function, which we will use later. >> >> Suggested-by: Jan Kara <jack@suse.cz> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> > I think you can merge this patch with patch 5. It is not like separating > this bit helps in review or anything... OK, I just don't want to cram a lot of stuff into patch 5, I will merge this patch into patch 5 in the next version. >> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c >> index 108ba9f1e420..a8b43b5b5623 100644 >> --- a/fs/quota/dquot.c >> +++ b/fs/quota/dquot.c >> @@ -226,12 +226,21 @@ static void put_quota_format(struct quota_format_type *fmt) >> /* >> * Dquot List Management: >> * The quota code uses four lists for dquot management: the inuse_list, > ^^^ five now :) Yes, indeed, I forgot to correct here. > >> - * free_dquots, dqi_dirty_list, and dquot_hash[] array. A single dquot >> - * structure may be on some of those lists, depending on its current state. >> + * releasing_dquots, free_dquots, dqi_dirty_list, and dquot_hash[] array. >> + * A single dquot structure may be on some of those lists, depending on >> + * its current state. >> * >> * All dquots are placed to the end of inuse_list when first created, and this >> * list is used for invalidate operation, which must look at every dquot. >> * >> + * When the last reference of a dquot will be dropped, the dquot will be >> + * added to releasing_dquots. We'd then queue work item which would call >> + * synchronize_srcu() and after that perform the final cleanup of all the >> + * dquots on the list. Both releasing_dquots and free_dquots use the >> + * dq_free list_head in the dquot struct. when a dquot is removed from > ^^^ Capital W please Good catch！Very sorry for the oversight here. >> + * releasing_dquots, a reference count is always subtracted, and if >> + * dq_count == 0 at that point, the dquot will be added to the free_dquots. >> + * > Honza Thank you very much for your careful REVIEW! I will fix those in the next version!
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 108ba9f1e420..a8b43b5b5623 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -226,12 +226,21 @@ static void put_quota_format(struct quota_format_type *fmt) /* * Dquot List Management: * The quota code uses four lists for dquot management: the inuse_list, - * free_dquots, dqi_dirty_list, and dquot_hash[] array. A single dquot - * structure may be on some of those lists, depending on its current state. + * releasing_dquots, free_dquots, dqi_dirty_list, and dquot_hash[] array. + * A single dquot structure may be on some of those lists, depending on + * its current state. * * All dquots are placed to the end of inuse_list when first created, and this * list is used for invalidate operation, which must look at every dquot. * + * When the last reference of a dquot will be dropped, the dquot will be + * added to releasing_dquots. We'd then queue work item which would call + * synchronize_srcu() and after that perform the final cleanup of all the + * dquots on the list. Both releasing_dquots and free_dquots use the + * dq_free list_head in the dquot struct. when a dquot is removed from + * releasing_dquots, a reference count is always subtracted, and if + * dq_count == 0 at that point, the dquot will be added to the free_dquots. + * * Unused dquots (dq_count == 0) are added to the free_dquots list when freed, * and this list is searched whenever we need an available dquot. Dquots are * removed from the list as soon as they are used again, and @@ -250,6 +259,7 @@ static void put_quota_format(struct quota_format_type *fmt) static LIST_HEAD(inuse_list); static LIST_HEAD(free_dquots); +static LIST_HEAD(releasing_dquots); static unsigned int dq_hash_bits, dq_hash_mask; static struct hlist_head *dquot_hash; @@ -305,6 +315,13 @@ static inline void put_dquot_last(struct dquot *dquot) dqstats_inc(DQST_FREE_DQUOTS); } +static inline void put_releasing_dquots(struct dquot *dquot) +{ + list_add_tail(&dquot->dq_free, &releasing_dquots); + /* dquot will be moved to free_dquots during shrink. */ + dqstats_inc(DQST_FREE_DQUOTS); +} + static inline void remove_free_dquot(struct dquot *dquot) { if (list_empty(&dquot->dq_free))
Add a new global dquot list that obeys the following rules: 1). A dquot is added to this list when its last reference count is about to be dropped. 2). The reference count of the dquot in the list is greater than or equal to 1 ( due to possible race with dqget()). 3). When a dquot is removed from this list, a reference count is always subtracted, and if the reference count is then 0, the dquot is added to the free_dquots list. This list is used to safely perform the final cleanup before releasing the last reference count, to avoid various contention issues caused by performing cleanup directly in dqput(), and to avoid the performance impact caused by calling synchronize_srcu(&dquot_srcu) directly in dqput(). Here it is just defining the list and implementing the corresponding operation function, which we will use later. Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Baokun Li <libaokun1@huawei.com> --- fs/quota/dquot.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)