Re: [PATCH v3.1 0/9] btrfs: Refactor delayed ref parameter list

[David Sterba <dsterba@suse.cz>]

On Thu, Apr 04, 2019 at 02:45:28PM +0800, Qu Wenruo wrote:
> 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 56d46f96de92ec69963acb7b1d9aed83d2a56a7b (david/misc-next-with-write-checks, david/misc-next)
> Author: Nikolay Borisov <nborisov@suse.com>
> Date:   Wed Mar 27 14:24:18 2019 +0200
> 
>     btrfs: Switch btrfs_trim_free_extents to find_first_clear_extent_bit
> 
> 
> 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 qgroups |
> -------------------------------------------------------------
> 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.
> 
> v3.1:
> - Rebase to misc-next branch.

This does not seem to exhibit the problems I recall from previous
testing, so I'll queue it for merge to misc-next unless there are more
review comments.