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 <nbori...@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.
