Re: [PATCH 01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage()

[Nikolay Borisov <nborisov@suse.com>]

On 3.11.20 г. 15:30 ч., Qu Wenruo wrote:
> In end_bio_extent_readpage() we had a strange dance around
> extent_start/extent_len.
> 
> Hides behind the strange dance is, it's just calling
> endio_readpage_release_extent() on each bvec range.
> 
> Here is an example to explain the original work flow:
>   Bio is for inode 257, containing 2 pages, for range [1M, 1M+8K)
> 
>   end_bio_extent_extent_readpage() entered
>   |- extent_start = 0;
>   |- extent_end = 0;
>   |- bio_for_each_segment_all() {
>   |  |- /* Got the 1st bvec */
>   |  |- start = SZ_1M;
>   |  |- end = SZ_1M + SZ_4K - 1;
>   |  |- update = 1;
>   |  |- if (extent_len == 0) {
>   |  |  |- extent_start = start; /* SZ_1M */
>   |  |  |- extent_len = end + 1 - start; /* SZ_1M */
>   |  |  }
>   |  |
>   |  |- /* Got the 2nd bvec */
>   |  |- start = SZ_1M + 4K;
>   |  |- end = SZ_1M + 4K - 1;
>   |  |- update = 1;
>   |  |- if (extent_start + extent_len == start) {
>   |  |  |- extent_len += end + 1 - start; /* SZ_8K */
>   |  |  }
>   |  } /* All bio vec iterated */
>   |
>   |- if (extent_len) {
>      |- endio_readpage_release_extent(tree, extent_start, extent_len,
> 				      update);
> 	/* extent_start == SZ_1M, extent_len == SZ_8K, uptodate = 1 */
> 
> As the above flow shows, the existing code in end_bio_extent_readpage()
> is just accumulate extent_start/extent_len, and when the contiguous range
> breaks, call endio_readpage_release_extent() for the range.
> 
> The contiguous range breaks at two locations:
> - The total else {} branch
>   This means we had a page in a bio where it's not contiguous.
>   Currently this branch will never be triggered. As all our bio is
>   submitted as contiguous pages.
> 

The endio routine cares about logical file contiguity as evidenced by
the fact it uses page_offset() to calculate 'start', however our recent
discussion on irc with the contiguity in csums bios clearly showed that
we can have bios which contains pages that are contiguous in their disk
byte nr but not in their logical offset, in fact Josef even mentioned on
slack that a single bio can contain pages for different inodes so long
as their logical disk byte nr are contiguous. I think this is not an
issue in this case because you are doing the unlock on a bvec
granularity but just the above statement is somewhat misleading.

> - After the bio_for_each_segment_all() loop ends
>   This is the normal call sites where we iterated all bvecs of a bio,
>   and all pages should be contiguous, thus we can call
>   endio_readpage_release_extent() on the full range.
> 
> The original code has also considered cases like (!uptodate), so it will
> mark the uptodate range with EXTENT_UPTODATE.
> 
> So this patch will remove the extent_start/extent_len dancing, replace
> it with regular endio_readpage_release_extent() call on each bvec.
> 
> This brings one behavior change:
> - Temporary memory usage increase
>   Unlike the old call which only modify the extent tree once, now we
>   update the extent tree for each bvec.

I suspect for large bios with a lot of bvecs this would likely increase
latency because we will now invoke endio_readpage_release_extent
proportionally to the number of bvecs.

> 
>   Although the end result is the same, since we may need more extent
>   state split/allocation, we need more temporary memory during that
>   bvec iteration.

Also bear in mind that this happens in a critical endio context, which
uses GFP_ATOMIC allocations so if we get ENOSPC it would be rather bad.

> 
> But considering how streamline the new code is, the temporary memory
> usage increase should be acceptable.

I definitely like the new code however without quantifying what's the
increase of number of calls of endio_readpage_release_extent I'd rather
not merge it.

On a different note, one minor cleanup that could be done is replace all
those "end + 1 - start" expressions with simply "len" as this is
effectively the length of the current bvec.

<snip>