[v3.1,0/9] btrfs: Refactor delayed ref parameter list
mbox series

Message ID 20190404064537.4031-1-wqu@suse.com
Headers show
Series
  • btrfs: Refactor delayed ref parameter list
Related show

Message

Qu Wenruo April 4, 2019, 6:45 a.m. UTC
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.

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 |  40 ++++++---
 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, 380 insertions(+), 213 deletions(-)

Comments

David Sterba April 5, 2019, 12:06 p.m. UTC | #1
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.
David Sterba April 23, 2019, 11:42 a.m. UTC | #2
On Fri, Apr 05, 2019 at 02:06:09PM +0200, David Sterba wrote:
> >               |      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.

Added to misc-next, ie. the argument cleanup and there's the
qgroup+metadata balance improvement. Thanks.