| Message ID | 20190211051653.3167-1-wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: Refactor delayed ref parameter list | expand |
On Mon, Feb 11, 2019 at 01:16:44PM +0800, Qu Wenruo wrote: > Qu Wenruo (9): > btrfs: delayed-ref: Introduce better documented delayed ref structures > btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref > btrfs: delayed-ref: Use btrfs_ref to refactor > btrfs_add_delayed_tree_ref() > btrfs: delayed-ref: Use btrfs_ref to refactor > btrfs_add_delayed_data_ref() > btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() > btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() > btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() > btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() If you would like to get the above merged (ie. the parameter cleanup) to 5.2, please refresh and resend on top of current misc-next. I tested this patchset back then and it did not pass fstests but I haven't investigated. As this patchset does only a transformation, I'm suspecting some typo so another look from you would be helpful. Thanks.
On 2019/4/4 上午12:29, David Sterba wrote: > On Mon, Feb 11, 2019 at 01:16:44PM +0800, Qu Wenruo wrote: >> Qu Wenruo (9): >> btrfs: delayed-ref: Introduce better documented delayed ref structures >> btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref >> btrfs: delayed-ref: Use btrfs_ref to refactor >> btrfs_add_delayed_tree_ref() >> btrfs: delayed-ref: Use btrfs_ref to refactor >> btrfs_add_delayed_data_ref() >> btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() >> btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() >> btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() >> btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() > > If you would like to get the above merged (ie. the parameter cleanup) to > 5.2, please refresh and resend on top of current misc-next. I tested > this patchset back then and it did not pass fstests but I haven't > investigated. As this patchset does only a transformation, I'm > suspecting some typo so another look from you would be helpful. Thanks. I'll rebase and retest the patchset. Thanks for the mentioning. Qu
On 2019/4/4 上午12:29, David Sterba wrote: > On Mon, Feb 11, 2019 at 01:16:44PM +0800, Qu Wenruo wrote: >> Qu Wenruo (9): >> btrfs: delayed-ref: Introduce better documented delayed ref structures >> btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref >> btrfs: delayed-ref: Use btrfs_ref to refactor >> btrfs_add_delayed_tree_ref() >> btrfs: delayed-ref: Use btrfs_ref to refactor >> btrfs_add_delayed_data_ref() >> btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() >> btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() >> btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() >> btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() > > If you would like to get the above merged (ie. the parameter cleanup) to > 5.2, please refresh and resend on top of current misc-next. I tested > this patchset back then and it did not pass fstests but I haven't > investigated. As this patchset does only a transformation, I'm > suspecting some typo so another look from you would be helpful. Thanks. I have rebased the patchset and re-test with full fstests run. It passes with no new regression. Although without the initial report, I'm not 100% sure what's causing problem for your old test. While the new patchset passes full fstests, I'm going to send out the patch anyway, hoping the initial fail is from my previous inode mode check bug. Thanks, Qu
This patchset can be fetched from github: https://github.com/adam900710/linux/tree/refactor_delayed_ref_parameter Which is based on David's misc-next branch, the base commit is: commit eb3e579e28f1c58e79176fbf5afe1cf3ee227190 (david/misc-next, david/misc-5.1) Author: Anand Jain <anand.jain@oracle.com> Date: Fri Feb 8 15:39:37 2019 +0800 Current delayed ref interface has several problems: - Longer and longer parameter lists bytenr num_bytes parent ---- So far so good ref_root owner offset ---- I don't feel well now for_reloc ^^^^ This parameter only makes sense for qgroup code, but we need to pass the parameter a long way down. This makes later parameter list add more and more tricky. - Different interpretation for the same parameter Above @owner for data ref is inode who owns this extent, while for tree ref, it's level. They are even in different size range. For level we only need 0~8, while for ino it's BTRFS_FIRST_FREE_OBJECTID~BTRFS_LAST_FREE_OBJECTID, so it's still possible to distinguish them, but it's never a straight-forward thing to grasp. And @offset doesn't even makes sense for tree ref. Such parameter reuse may look clever as an hidden union, but it destroys code readability. This patchset will change the way how we pass parameters for delayed ref. Instead of calling delayed ref interface like: ret = btrfs_inc_extent_ref(trans, root, bytenr, num_bytes, parent, ref_root, owner, offset); Or ret = btrfs_inc_extent_ref(trans, root, bytenr, nodesize, parent, level, ref_root, 0); We now call like: btrfs_init_generic_ref(&ref, bytenr, num_bytes, root->root_key.objectid, parent); btrfs_init_data_ref(&ref, ref_root, owner, offset); ret = btrfs_inc_extent_ref(trans, &ref); Or btrfs_init_generic_ref(&ref, bytenr, num_bytes, root->root_key.objectid, parent); btrfs_init_tree_ref(&ref, level, ref_root); ret = btrfs_inc_extent_ref(trans, &ref); To determine if a ref is tree or data, instead of calling like: if (owner < BTRFS_FIRST_FREE_OBJECTID) { } else { } We do it straight-forward: if (ref->type == BTRFS_REF_METADATA) { } else { } And for new members determining some minor behavior, we don't need to add a new parameter to btrfs_add_delayed_tree|data_ref() or btrfs_inc_extent_ref(), we just assign them after generic/data/tree init, like: btrfs_init_generic_ref(&ref, bytenr, num_bytes, root->root_key.objectid, parent); ref->real_root = root->root_key.objectid; ref->skip_qgroup = true; btrfs_init_data_ref(&ref, ref_root, owner, offset); ret = btrfs_inc_extent_ref(trans, &ref); This should improve the code readability and make later code easier to write. Furthermore, with the help of btrfs_ref::real_root parameter, qgroup can skip quit a lot of delayed tree/data ref for reloc tree, which makes qgroup + balance as fast as quota disabled: Test VM: - vRAM 8G - vCPU 8 - block dev vitrio-blk, 'unsafe' cache mode - host block 850evo Test workload - Copy 4G data from /usr/ to one subvolume - Create 16 snapshots of that subvolume, and modify 3 files in each snapshot - Enable quota, rescan - Time "btrfs balance start -m" | base | w/ patchset | no qgrouos | ------------------------------------------------------------- relocated | 23765 | 23772 | 23811 | qgroup dirty | 124498 | 70 | 0 | time (sec) | 23.353 | 3.505 | 3.421 | Changelog: v2: - Better documentation for btrfs_ref declaration - Rebase to newer delayed subtree rescan patchset - Add reviewed-by tags - Remove unnecessary ASSERT() for NULL pointer. v3: - Rebase to misc-next branch as that branch has all prerequisite now. - Update benchmark result, compare with qgroups disabled case directly. Qu Wenruo (9): btrfs: delayed-ref: Introduce better documented delayed ref structures btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref() btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref() btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() btrfs: qgroup: Don't scan leaf if we're modifying reloc tree fs/btrfs/ctree.h | 10 +-- fs/btrfs/delayed-ref.c | 44 ++++++--- fs/btrfs/delayed-ref.h | 126 ++++++++++++++++++++++++-- fs/btrfs/extent-tree.c | 199 ++++++++++++++++++++--------------------- fs/btrfs/file.c | 39 ++++---- fs/btrfs/inode.c | 23 +++-- fs/btrfs/ioctl.c | 15 ++-- fs/btrfs/ref-verify.c | 53 ++++++----- fs/btrfs/ref-verify.h | 10 +-- fs/btrfs/relocation.c | 67 +++++++++----- fs/btrfs/tree-log.c | 11 ++- 11 files changed, 382 insertions(+), 215 deletions(-)