Re: [PATCH RFC 00/39] btrfs: qgroup: Use backref cache based backref walk for commit roots

[Josef Bacik <josef@toxicpanda.com>]

On 3/17/20 4:10 AM, Qu Wenruo wrote:
> This patchset is based on an OLD misc-next branch, please inform me
> before trying to merge, so I can rebase it to latest misc-next.
> (There will be tons of conflicts)
> 
> The branch can be fetched from github for review/testing.
> https://github.com/adam900710/linux/tree/backref_cache_all
> 
> The patchset survives all the existing qgroup tests.
> 
> 
> === BACKGROUND ===
> One of the biggest problem for qgroup is its performance impact.
> Although we have improved it in since v5.0 kernel, there is still
> something slowing down qgroup, the backref walk.
> 
> Before this patchset, we use btrfs_find_all_roots() to iterate all roots
> referring to one extent.
> That function is doing a pretty good job, but it doesn't has any cache,
> which means even we're looking up the same extent, we still need to do
> the full backref walk.
> 
> On the other hand, relocation is doing its own backref cache, and
> provides a much faster backref walk.
> 
> So the patchset is mostly trying to make qgroup backref walk (at least
> commit root backref walk) to use the same mechanism provided by
> relocation.
> 
> === BENCHMARK ===
> For the performance improvement, the last patch has a benchmark.
> The following content is completely copied from that patch:
> ------
> Here is a small script to test it:
> 
>    mkfs.btrfs -f $dev
>    mount $dev -o space_cache=v2 $mnt
> 
>    btrfs subvolume create $mnt/src
> 
>    for ((i = 0; i < 64; i++)); do
>            for (( j = 0; j < 16; j++)); do
>                    xfs_io -f -c "pwrite 0 2k" $mnt/src/file_inline_$(($i * 16 + $j)) > /dev/null
>            done
>            xfs_io -f -c "pwrite 0 1M" $mnt/src/file_reg_$i > /dev/null
>            sync
>            btrfs subvol snapshot $mnt/src $mnt/snapshot_$i
>    done
>    sync
> 
>    btrfs quota enable $mnt
>    btrfs quota rescan -w $mnt
> 
> Here is the benchmark for above small tests.
> The performance material is the total execution time of get_old_roots()
> for patched kernel (*), and find_all_roots() for original kernel.
> 
> *: With CONFIG_BTRFS_FS_CHECK_INTEGRITY disabled, as get_old_roots()
>     will call find_all_roots() to verify the result if that config is
>     enabled.
> 
> 		|  Number of calls | Total exec time |
> ------------------------------------------------------
> find_all_roots()|  732		   | 529991034ns
> get_old_roots() |  732		   | 127998312ns
> ------------------------------------------------------
> diff		|  0.00 %	   | -75.8 %
> ------
> 
> 
> Patch 01~30 are mostly refactors and code movement, which exposes no
> behavior change.
> 
> Patch 31~32 are small behavior change only for qgroup backref cache.
> Patch 33~39 are the implementation of qgroup backref cache.
> 
> 
> === REASON FOR RFC ===
> The naming is currently my biggest concern.
> 
> Since the code movement involves exporting quite a lot of functions, in
> theory they should have btrfs_ prefix.
> (For all newly exported functions in backref.h)
> 
> But some functions like alloc_backref_node(), adding "btrfs_" prefix
> doesn't make it more clear, but just making it unnecessary long.
> 
> My current plan is to rename them using "btrfs_brc_" prefix (BackRef
> Cache), and remove the "cache" in the original name.
> 
> E.g:
> alloc_backref_node => btrfs_brc_alloc_node()
> backref_cache_release => btrfs_brc_release()
> link_backref_edge => btrfs_brc_link_edge()
> 
> But the abbr "brc" is pretty confusing and makes no sense by itself, so
> I'm not sure what's the best practice here.
> 

btrfs_backref_node_alloc()
btrfs_backref_node_free()
btrfs_backref_edge_link()

etc, rename the structures to btrfs_backref_* and then make the function names 
follow suit.  Thanks,

Josef