| Message ID | e6d30c5f77867f20ca19244e5c37881188855d6e.1693391268.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: qgroup: reduce GFP_ATOMIC usage for ulist | expand |
On Wed, Aug 30, 2023 at 06:37:27PM +0800, Qu Wenruo wrote: > For function qgroup_update_refcnt(), we use @tmp list to iterate all the > involved qgroups of a subvolume. > > It's a perfect match for qgroup_iterator facility, as that @tmp ulist > has a very limited lifespan (just inside the while() loop). > > By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory > allocation and no more error handling needed. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/qgroup.c | 37 +++++++++++-------------------------- > 1 file changed, 11 insertions(+), 26 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 08f4fc622180..6fcf302744e2 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2463,13 +2463,11 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans, > * Walk all of the roots that points to the bytenr and adjust their refcnts. > */ > static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, > - struct ulist *roots, struct ulist *tmp, > - struct ulist *qgroups, u64 seq, int update_old) > + struct ulist *roots, struct ulist *qgroups, > + u64 seq, int update_old) > { > struct ulist_node *unode; > struct ulist_iterator uiter; > - struct ulist_node *tmp_unode; > - struct ulist_iterator tmp_uiter; > struct btrfs_qgroup *qg; > int ret = 0; > > @@ -2477,40 +2475,35 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, > return 0; > ULIST_ITER_INIT(&uiter); > while ((unode = ulist_next(roots, &uiter))) { > + LIST_HEAD(tmp); > + > qg = find_qgroup_rb(fs_info, unode->val); > if (!qg) > continue; > > - ulist_reinit(tmp); > ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg), > GFP_ATOMIC); > if (ret < 0) > return ret; > - ret = ulist_add(tmp, qg->qgroupid, qgroup_to_aux(qg), GFP_ATOMIC); > - if (ret < 0) > - return ret; > - ULIST_ITER_INIT(&tmp_uiter); > - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { > + qgroup_iterator_add(&tmp, qg); > + list_for_each_entry(qg, &tmp, iterator) { > struct btrfs_qgroup_list *glist; > > - qg = unode_aux_to_qgroup(tmp_unode); > if (update_old) > btrfs_qgroup_update_old_refcnt(qg, seq, 1); > else > btrfs_qgroup_update_new_refcnt(qg, seq, 1); > + > list_for_each_entry(glist, &qg->groups, next_group) { > ret = ulist_add(qgroups, glist->group->qgroupid, > qgroup_to_aux(glist->group), > GFP_ATOMIC); Thinking out loud, could we use the same trick and put another global list on the qgroups to handle this one? Or some other "single global allocation up to #qgroups" trick. > if (ret < 0) > return ret; > - ret = ulist_add(tmp, glist->group->qgroupid, > - qgroup_to_aux(glist->group), > - GFP_ATOMIC); > - if (ret < 0) > - return ret; > + qgroup_iterator_add(&tmp, glist->group); > } > } > + qgroup_iterator_clean(&tmp); > } > return 0; > } > @@ -2675,7 +2668,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, > { > struct btrfs_fs_info *fs_info = trans->fs_info; > struct ulist *qgroups = NULL; > - struct ulist *tmp = NULL; > u64 seq; > u64 nr_new_roots = 0; > u64 nr_old_roots = 0; > @@ -2714,12 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, > ret = -ENOMEM; > goto out_free; > } > - tmp = ulist_alloc(GFP_NOFS); > - if (!tmp) { > - ret = -ENOMEM; > - goto out_free; > - } > - > mutex_lock(&fs_info->qgroup_rescan_lock); > if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) { > if (fs_info->qgroup_rescan_progress.objectid <= bytenr) { > @@ -2734,13 +2720,13 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, > seq = fs_info->qgroup_seq; > > /* Update old refcnts using old_roots */ > - ret = qgroup_update_refcnt(fs_info, old_roots, tmp, qgroups, seq, > + ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq, > UPDATE_OLD); > if (ret < 0) > goto out; > > /* Update new refcnts using new_roots */ > - ret = qgroup_update_refcnt(fs_info, new_roots, tmp, qgroups, seq, > + ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq, > UPDATE_NEW); > if (ret < 0) > goto out; > @@ -2755,7 +2741,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, > out: > spin_unlock(&fs_info->qgroup_lock); > out_free: > - ulist_free(tmp); > ulist_free(qgroups); > ulist_free(old_roots); > ulist_free(new_roots); > -- > 2.41.0 >
On 2023/8/31 06:09, Boris Burkov wrote: > On Wed, Aug 30, 2023 at 06:37:27PM +0800, Qu Wenruo wrote: >> For function qgroup_update_refcnt(), we use @tmp list to iterate all the >> involved qgroups of a subvolume. >> >> It's a perfect match for qgroup_iterator facility, as that @tmp ulist >> has a very limited lifespan (just inside the while() loop). >> >> By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory >> allocation and no more error handling needed. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/qgroup.c | 37 +++++++++++-------------------------- >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index 08f4fc622180..6fcf302744e2 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -2463,13 +2463,11 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans, >> * Walk all of the roots that points to the bytenr and adjust their refcnts. >> */ >> static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, >> - struct ulist *roots, struct ulist *tmp, >> - struct ulist *qgroups, u64 seq, int update_old) >> + struct ulist *roots, struct ulist *qgroups, >> + u64 seq, int update_old) >> { >> struct ulist_node *unode; >> struct ulist_iterator uiter; >> - struct ulist_node *tmp_unode; >> - struct ulist_iterator tmp_uiter; >> struct btrfs_qgroup *qg; >> int ret = 0; >> >> @@ -2477,40 +2475,35 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, >> return 0; >> ULIST_ITER_INIT(&uiter); >> while ((unode = ulist_next(roots, &uiter))) { >> + LIST_HEAD(tmp); >> + >> qg = find_qgroup_rb(fs_info, unode->val); >> if (!qg) >> continue; >> >> - ulist_reinit(tmp); >> ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg), >> GFP_ATOMIC); >> if (ret < 0) >> return ret; >> - ret = ulist_add(tmp, qg->qgroupid, qgroup_to_aux(qg), GFP_ATOMIC); >> - if (ret < 0) >> - return ret; >> - ULIST_ITER_INIT(&tmp_uiter); >> - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { >> + qgroup_iterator_add(&tmp, qg); >> + list_for_each_entry(qg, &tmp, iterator) { >> struct btrfs_qgroup_list *glist; >> >> - qg = unode_aux_to_qgroup(tmp_unode); >> if (update_old) >> btrfs_qgroup_update_old_refcnt(qg, seq, 1); >> else >> btrfs_qgroup_update_new_refcnt(qg, seq, 1); >> + >> list_for_each_entry(glist, &qg->groups, next_group) { >> ret = ulist_add(qgroups, glist->group->qgroupid, >> qgroup_to_aux(glist->group), >> GFP_ATOMIC); > > Thinking out loud, could we use the same trick and put another global > list on the qgroups to handle this one? Or some other "single global > allocation up to #qgroups" trick. I'm considering this as the last resort method. Currently I tried to move this qgroups collecting code into one dedicate function other than doing two jobs inside one function. But that idea doesn't work as expected, I'm hoping to at least understand why before adding a new list_head. Thanks, Qu > >> if (ret < 0) >> return ret; >> - ret = ulist_add(tmp, glist->group->qgroupid, >> - qgroup_to_aux(glist->group), >> - GFP_ATOMIC); >> - if (ret < 0) >> - return ret; >> + qgroup_iterator_add(&tmp, glist->group); >> } >> } >> + qgroup_iterator_clean(&tmp); >> } >> return 0; >> } >> @@ -2675,7 +2668,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, >> { >> struct btrfs_fs_info *fs_info = trans->fs_info; >> struct ulist *qgroups = NULL; >> - struct ulist *tmp = NULL; >> u64 seq; >> u64 nr_new_roots = 0; >> u64 nr_old_roots = 0; >> @@ -2714,12 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, >> ret = -ENOMEM; >> goto out_free; >> } >> - tmp = ulist_alloc(GFP_NOFS); >> - if (!tmp) { >> - ret = -ENOMEM; >> - goto out_free; >> - } >> - >> mutex_lock(&fs_info->qgroup_rescan_lock); >> if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) { >> if (fs_info->qgroup_rescan_progress.objectid <= bytenr) { >> @@ -2734,13 +2720,13 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, >> seq = fs_info->qgroup_seq; >> >> /* Update old refcnts using old_roots */ >> - ret = qgroup_update_refcnt(fs_info, old_roots, tmp, qgroups, seq, >> + ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq, >> UPDATE_OLD); >> if (ret < 0) >> goto out; >> >> /* Update new refcnts using new_roots */ >> - ret = qgroup_update_refcnt(fs_info, new_roots, tmp, qgroups, seq, >> + ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq, >> UPDATE_NEW); >> if (ret < 0) >> goto out; >> @@ -2755,7 +2741,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, >> out: >> spin_unlock(&fs_info->qgroup_lock); >> out_free: >> - ulist_free(tmp); >> ulist_free(qgroups); >> ulist_free(old_roots); >> ulist_free(new_roots); >> -- >> 2.41.0 >>
Regarding style guidelines, the subject should not be overly long, like in this patch, "btrfs: qgroup: use qgroup_iterator in qgroup_update_refcnt" would work too.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 08f4fc622180..6fcf302744e2 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2463,13 +2463,11 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans, * Walk all of the roots that points to the bytenr and adjust their refcnts. */ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, - struct ulist *roots, struct ulist *tmp, - struct ulist *qgroups, u64 seq, int update_old) + struct ulist *roots, struct ulist *qgroups, + u64 seq, int update_old) { struct ulist_node *unode; struct ulist_iterator uiter; - struct ulist_node *tmp_unode; - struct ulist_iterator tmp_uiter; struct btrfs_qgroup *qg; int ret = 0; @@ -2477,40 +2475,35 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info, return 0; ULIST_ITER_INIT(&uiter); while ((unode = ulist_next(roots, &uiter))) { + LIST_HEAD(tmp); + qg = find_qgroup_rb(fs_info, unode->val); if (!qg) continue; - ulist_reinit(tmp); ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg), GFP_ATOMIC); if (ret < 0) return ret; - ret = ulist_add(tmp, qg->qgroupid, qgroup_to_aux(qg), GFP_ATOMIC); - if (ret < 0) - return ret; - ULIST_ITER_INIT(&tmp_uiter); - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { + qgroup_iterator_add(&tmp, qg); + list_for_each_entry(qg, &tmp, iterator) { struct btrfs_qgroup_list *glist; - qg = unode_aux_to_qgroup(tmp_unode); if (update_old) btrfs_qgroup_update_old_refcnt(qg, seq, 1); else btrfs_qgroup_update_new_refcnt(qg, seq, 1); + list_for_each_entry(glist, &qg->groups, next_group) { ret = ulist_add(qgroups, glist->group->qgroupid, qgroup_to_aux(glist->group), GFP_ATOMIC); if (ret < 0) return ret; - ret = ulist_add(tmp, glist->group->qgroupid, - qgroup_to_aux(glist->group), - GFP_ATOMIC); - if (ret < 0) - return ret; + qgroup_iterator_add(&tmp, glist->group); } } + qgroup_iterator_clean(&tmp); } return 0; } @@ -2675,7 +2668,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, { struct btrfs_fs_info *fs_info = trans->fs_info; struct ulist *qgroups = NULL; - struct ulist *tmp = NULL; u64 seq; u64 nr_new_roots = 0; u64 nr_old_roots = 0; @@ -2714,12 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, ret = -ENOMEM; goto out_free; } - tmp = ulist_alloc(GFP_NOFS); - if (!tmp) { - ret = -ENOMEM; - goto out_free; - } - mutex_lock(&fs_info->qgroup_rescan_lock); if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) { if (fs_info->qgroup_rescan_progress.objectid <= bytenr) { @@ -2734,13 +2720,13 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, seq = fs_info->qgroup_seq; /* Update old refcnts using old_roots */ - ret = qgroup_update_refcnt(fs_info, old_roots, tmp, qgroups, seq, + ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq, UPDATE_OLD); if (ret < 0) goto out; /* Update new refcnts using new_roots */ - ret = qgroup_update_refcnt(fs_info, new_roots, tmp, qgroups, seq, + ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq, UPDATE_NEW); if (ret < 0) goto out; @@ -2755,7 +2741,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, out: spin_unlock(&fs_info->qgroup_lock); out_free: - ulist_free(tmp); ulist_free(qgroups); ulist_free(old_roots); ulist_free(new_roots);
For function qgroup_update_refcnt(), we use @tmp list to iterate all the involved qgroups of a subvolume. It's a perfect match for qgroup_iterator facility, as that @tmp ulist has a very limited lifespan (just inside the while() loop). By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory allocation and no more error handling needed. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/qgroup.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)