Re: [PATCH v2 03/24] btrfs: extent_io: replace extent_start/extent_len with better structure for end_bio_extent_readpage()

[David Sterba <dsterba@suse.cz>]

On Fri, Nov 13, 2020 at 08:51:28PM +0800, 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.
> 
> However current behavior has something not really considered:
> - The inode can change
>   For bio, their pages don't need to have contig page_offset.
>   This means, even pages from different inode can be packed into one
>   bio.
> 
> - Bvec cross page boundary
>   There is a feature called multi-page bvec, where bvec->bv_len can go
>   beyond bvec->bv_page boundary.
> 
> - Poor readability
> 
> This patch will address the problem by:
> - Introduce a proper structure, processed_extent, to record processed
>   extent range
> - Integrate inode/start/end/uptodate check into
>   endio_readpage_release_extent()
> - Add more comment on each step.
>   This should greatly improve the readability, now in
>   end_bio_extent_readpage() there are only two
>   endio_readpage_release_extent() calls.
> 
> - Add inode contig check
>   Now we also ensure the inode is the same before checking the range
>   contig.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to misc-next.