| Message ID | cover.1693613265.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: qgroup: remove GFP_ATOMIC usage for ulist | expand |
On Sat, Sep 02, 2023 at 08:13:51AM +0800, Qu Wenruo wrote: > [CHANGELOG] > v3: > - Remove the no longer utilized memalloc_nofs_save() calls > - Remove the "@" prefix of variables in the subject line > - Rename iterator2 to nested_iterator and enhance the comments of it > > v2: > - Remove the final GFP_ATOMIC ulist usage > This is done by introducing a new list_head, btrfs_qgroup::iterator2, > allowing us to do nested iteration. > > This extra nested facility is needed as even if we move the qgroups > collection into one dedicated function, we're reusing the list for > iteration which can lead to unnecessary re-visit of the same qgroup. > > Thus we have to support one level of nested iteration. > > - Add reviewed by tags from Boris > > [REPO] > https://github.com/adam900710/linux/tree/qgroup_mutex > > Yep, the branch name shows my previous failed attempt to solve the > problem. > > [PROBLEM] > There are quite some GFP_ATOMIC usage for btrfs qgroups, most of them > are for ulists. > > Those ulists are just used as a temporary memory to trace the involved > qgroups. > > [ENHANCEMENT] > This patchset would address the problem by adding a new list_head called > iterator for btrfs_qgroup. > > And all call sites but one of ulist allocation can be migrated to use > the new qgroup_iterator facility to iterate qgroups without any memory > allocation. > > There is a special call site in btrfs_qgroup_account_extent() that it > wants to do nested qgroups iteration, thus a nested version is > introduced for this particular call site. > > And BTW since we can skip quite some memory allocation failure handling > (since there is no memory allocation), we also save some lines of code. > > Qu Wenruo (6): > btrfs: qgroup: iterate qgroups without memory allocation for > qgroup_reserve() > btrfs: qgroup: use qgroup_iterator facility for > btrfs_qgroup_free_refroot() > btrfs: qgroup: use qgroup_iterator facility for qgroup_convert_meta() > btrfs: qgroup: use qgroup_iterator facility for > __qgroup_excl_accounting() > btrfs: qgroup: use qgroup_iterator facility to replace tmp ulist of > qgroup_update_refcnt() > btrfs: qgroup: use qgroup_iterator_nested facility to replace qgroups > ulist of qgroup_update_refcnt() Added to misc-next, with some minor fixups, thanks. Getting rid of the allocations and error handling is great. I think qgroup_to_aux() is now unused and maybe there's more related to the ulists that can be cleaned up or removed.
On Wed, Sep 06, 2023 at 05:55:42PM +0200, David Sterba wrote: > On Sat, Sep 02, 2023 at 08:13:51AM +0800, Qu Wenruo wrote: > > [CHANGELOG] > > v3: > > - Remove the no longer utilized memalloc_nofs_save() calls > > - Remove the "@" prefix of variables in the subject line > > - Rename iterator2 to nested_iterator and enhance the comments of it > > > > v2: > > - Remove the final GFP_ATOMIC ulist usage > > This is done by introducing a new list_head, btrfs_qgroup::iterator2, > > allowing us to do nested iteration. > > > > This extra nested facility is needed as even if we move the qgroups > > collection into one dedicated function, we're reusing the list for > > iteration which can lead to unnecessary re-visit of the same qgroup. > > > > Thus we have to support one level of nested iteration. > > > > - Add reviewed by tags from Boris > > > > [REPO] > > https://github.com/adam900710/linux/tree/qgroup_mutex > > > > Yep, the branch name shows my previous failed attempt to solve the > > problem. > > > > [PROBLEM] > > There are quite some GFP_ATOMIC usage for btrfs qgroups, most of them > > are for ulists. > > > > Those ulists are just used as a temporary memory to trace the involved > > qgroups. > > > > [ENHANCEMENT] > > This patchset would address the problem by adding a new list_head called > > iterator for btrfs_qgroup. > > > > And all call sites but one of ulist allocation can be migrated to use > > the new qgroup_iterator facility to iterate qgroups without any memory > > allocation. > > > > There is a special call site in btrfs_qgroup_account_extent() that it > > wants to do nested qgroups iteration, thus a nested version is > > introduced for this particular call site. > > > > And BTW since we can skip quite some memory allocation failure handling > > (since there is no memory allocation), we also save some lines of code. > > > > Qu Wenruo (6): > > btrfs: qgroup: iterate qgroups without memory allocation for > > qgroup_reserve() > > btrfs: qgroup: use qgroup_iterator facility for > > btrfs_qgroup_free_refroot() > > btrfs: qgroup: use qgroup_iterator facility for qgroup_convert_meta() > > btrfs: qgroup: use qgroup_iterator facility for > > __qgroup_excl_accounting() > > btrfs: qgroup: use qgroup_iterator facility to replace tmp ulist of > > qgroup_update_refcnt() > > btrfs: qgroup: use qgroup_iterator_nested facility to replace qgroups > > ulist of qgroup_update_refcnt() > > Added to misc-next, with some minor fixups, thanks. Getting rid of the > allocations and error handling is great. I think qgroup_to_aux() is now > unused and maybe there's more related to the ulists that can be cleaned > up or removed. What's a bit unfortunate that now the size of btrfs_qgroup grows from 256 bytes to 288, which does not fit the 256 byte slab and goes to 512. I don't see how the size could be reduced so we may eventually utilize the remaining space for embedding other structures or caching some values.
[CHANGELOG] v3: - Remove the no longer utilized memalloc_nofs_save() calls - Remove the "@" prefix of variables in the subject line - Rename iterator2 to nested_iterator and enhance the comments of it v2: - Remove the final GFP_ATOMIC ulist usage This is done by introducing a new list_head, btrfs_qgroup::iterator2, allowing us to do nested iteration. This extra nested facility is needed as even if we move the qgroups collection into one dedicated function, we're reusing the list for iteration which can lead to unnecessary re-visit of the same qgroup. Thus we have to support one level of nested iteration. - Add reviewed by tags from Boris [REPO] https://github.com/adam900710/linux/tree/qgroup_mutex Yep, the branch name shows my previous failed attempt to solve the problem. [PROBLEM] There are quite some GFP_ATOMIC usage for btrfs qgroups, most of them are for ulists. Those ulists are just used as a temporary memory to trace the involved qgroups. [ENHANCEMENT] This patchset would address the problem by adding a new list_head called iterator for btrfs_qgroup. And all call sites but one of ulist allocation can be migrated to use the new qgroup_iterator facility to iterate qgroups without any memory allocation. There is a special call site in btrfs_qgroup_account_extent() that it wants to do nested qgroups iteration, thus a nested version is introduced for this particular call site. And BTW since we can skip quite some memory allocation failure handling (since there is no memory allocation), we also save some lines of code. Qu Wenruo (6): btrfs: qgroup: iterate qgroups without memory allocation for qgroup_reserve() btrfs: qgroup: use qgroup_iterator facility for btrfs_qgroup_free_refroot() btrfs: qgroup: use qgroup_iterator facility for qgroup_convert_meta() btrfs: qgroup: use qgroup_iterator facility for __qgroup_excl_accounting() btrfs: qgroup: use qgroup_iterator facility to replace tmp ulist of qgroup_update_refcnt() btrfs: qgroup: use qgroup_iterator_nested facility to replace qgroups ulist of qgroup_update_refcnt() fs/btrfs/qgroup.c | 337 +++++++++++++++++----------------------------- fs/btrfs/qgroup.h | 27 ++++ 2 files changed, 148 insertions(+), 216 deletions(-)