Re: [PATCH 24/32] btrfs: file-item: refactor btrfs_lookup_bio_sums() to handle out-of-order bvecs

[Nikolay Borisov <nborisov@suse.com>]

On 3.11.20 г. 15:31 ч., Qu Wenruo wrote:
> Refactor btrfs_lookup_bio_sums() by:
> - Remove the @file_offset parameter
>   There are two factors making the @file_offset parameter useless:
> 
>   * For csum lookup in csum tree, file offset makes no sense
>     We only need disk_bytenr, which is unrelated to file_offset
> 
>   * page_offset (file offset) of each bvec is not contiguous.
>     Pages can be added to the same bio as long as their on-disk bytenr
>     is contiguous, meaning we could have pages at differnt file offsets
>     in the same bio.
> 
>   Thus passing file_offset makes no sense any more.
>   The only user of file_offset is for data reloc inode, we will use
>   a new function, search_file_offset_in_bio(), to handle it.
> 
> - Extract the csum tree lookup into find_csum_tree_sums()

The function is no longer named find_csum_tree_sums but search_csum_tree
so update the changelog as well.

>   The new function will handle the csum search in csum tree.
>   The return value is the same as btrfs_find_ordered_sum(), returning
>   the found number of sectors who has checksum.

nit: s/who/which/


> 
> - Change how we do the main loop
>   The only needed info from bio is:
>   * the on-disk bytenr
>   * the length
> 
>   After extracting above info, we can do the search without bio
>   at all, which makes the main loop much simpler:
> 
> 	for (cur_disk_bytenr = orig_disk_bytenr;
> 	     cur_disk_bytenr < orig_disk_bytenr + orig_len;
> 	     cur_disk_bytenr += count * sectorsize) {
> 
> 		/* Lookup csum tree */
> 		count = find_csum_tree_sums(fs_info, path, cur_disk_bytenr,
> 					    search_len, csum_dst);

nit: update function name

> 		if (!count) {
> 			/* Csum hole handling */
> 		}
> 	}
> 
> - Use single variable as core to calculate all other offsets
>   Instead of all differnt type of variables, we use only one core

nit: s/differnt/different/

>   variable, cur_disk_bytenr, which represents the current disk bytenr.
> 
>   All involves values can be calculated from that core variable, and

nit: s/involves/involved/

>   all those variable will only be visible in the inner loop.
> 
> 	diff_sectors = div_u64(cur_disk_bytenr - orig_disk_bytenr,
> 			       sectorsize);
> 	cur_disk_bytenr = orig_disk_bytenr +
> 			  diff_sectors * sectorsize;
> 	csum_dst = csum + diff_sectors * csum_size;

this snippet also need to be either updated to reflect the latest state
of code name wise or simply be removed.

> 
> All above refactor makes btrfs_lookup_bio_sums() way more robust than it
> used to, especially related to the file offset lookup.
> Now file_offset lookup is only related to data reloc inode, other wise
> we don't need to bother file_offset at all.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This patch missed David's feedback @
https://lore.kernel.org/linux-btrfs/20201103194650.GD6756@twin.jikos.cz/
for v1 however it integrated feedback I gave to your original v2
posting. One thing which would help readability is making the  compound
division statements in search_csum_tree on a single line, even if they
break the 80 char limit, which is no 100 AFAIR, for btrfs we chose to
use longer lines than 80 if it made sense. I think this case is an
example where it does make sense.

<snip>