Re: [PATCH 08/15] btrfs: refactor end_bio_extent_readpage

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/5/17 22:50, Christoph Hellwig wrote:
> Untangle the goto mess and remove the pointless 'ret' local variable.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/extent_io.c | 87 +++++++++++++++++++++-----------------------
>   1 file changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f96d5b7071813..1ba2d4b194f2e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3028,7 +3028,6 @@ static void end_bio_extent_readpage(struct bio *bio)
>   	 */
>   	u32 bio_offset = 0;
>   	int mirror;
> -	int ret;
>   	struct bvec_iter_all iter_all;
>
>   	ASSERT(!bio_flagged(bio, BIO_CLONED));
> @@ -3039,6 +3038,7 @@ static void end_bio_extent_readpage(struct bio *bio)
>   		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   		const u32 sectorsize = fs_info->sectorsize;
>   		unsigned int error_bitmap = (unsigned int)-1;
> +		bool repair = false;
>   		u64 start;
>   		u64 end;
>   		u32 len;
> @@ -3076,55 +3076,23 @@ static void end_bio_extent_readpage(struct bio *bio)
>   			if (is_data_inode(inode)) {
>   				error_bitmap = btrfs_verify_data_csum(bbio,
>   						bio_offset, page, start, end);
> -				ret = error_bitmap;
> +				if (error_bitmap)
> +					uptodate = false;
>   			} else {
> -				ret = btrfs_validate_metadata_buffer(bbio,
> -					page, start, end, mirror);
> +				if (btrfs_validate_metadata_buffer(bbio,
> +						page, start, end, mirror))
> +					uptodate = false;
>   			}
> -			if (ret)
> -				uptodate = false;
> -			else
> -				clean_io_failure(BTRFS_I(inode)->root->fs_info,
> -						 failure_tree, tree, start,
> -						 page,
> -						 btrfs_ino(BTRFS_I(inode)), 0);
>   		}
>
> -		if (likely(uptodate))
> -			goto readpage_ok;
> -
> -		if (is_data_inode(inode)) {
> -			/*
> -			 * If we failed to submit the IO at all we'll have a
> -			 * mirror_num == 0, in which case we need to just mark
> -			 * the page with an error and unlock it and carry on.
> -			 */
> -			if (mirror == 0)
> -				goto readpage_ok;
> -
> -			/*
> -			 * submit_data_read_repair() will handle all the good
> -			 * and bad sectors, we just continue to the next bvec.
> -			 */
> -			submit_data_read_repair(inode, bio, bio_offset, bvec,
> -						mirror, error_bitmap);
> -
> -			ASSERT(bio_offset + len > bio_offset);
> -			bio_offset += len;
> -			continue;
> -		} else {
> -			struct extent_buffer *eb;
> -
> -			eb = find_extent_buffer_readpage(fs_info, page, start);
> -			set_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags);
> -			eb->read_mirror = mirror;
> -			atomic_dec(&eb->io_pages);
> -		}
> -readpage_ok:
>   		if (likely(uptodate)) {
>   			loff_t i_size = i_size_read(inode);
>   			pgoff_t end_index = i_size >> PAGE_SHIFT;
>
> +			clean_io_failure(BTRFS_I(inode)->root->fs_info,
> +					 failure_tree, tree, start, page,
> +					 btrfs_ino(BTRFS_I(inode)), 0);
> +
>   			/*
>   			 * Zero out the remaining part if this range straddles
>   			 * i_size.
> @@ -3141,14 +3109,41 @@ static void end_bio_extent_readpage(struct bio *bio)
>   				zero_user_segment(page, zero_start,
>   						  offset_in_page(end) + 1);
>   			}
> +		} else if (is_data_inode(inode)) {
> +			/*
> +			 * Only try to repair bios that actually made it to a
> +			 * device.  If the bio failed to be submitted mirror
> +			 * is 0 and we need to fail it without retrying.
> +			 */
> +			if (mirror > 0)
> +				repair = true;

In fact, you can do it even better, by also checking the number of
copies the bio has.

That what I missed in my all patchset, and completely relies on the
mirror based loop to exit.

> +		} else {
> +			struct extent_buffer *eb;
> +
> +			eb = find_extent_buffer_readpage(fs_info, page, start);
> +			set_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags);
> +			eb->read_mirror = mirror;
> +			atomic_dec(&eb->io_pages);
>   		}
> +
> +		if (repair) {
> +			/*
> +			 * submit_data_read_repair() will handle all the good
> +			 * and bad sectors, we just continue to the next bvec.
> +			 */
> +			submit_data_read_repair(inode, bio, bio_offset, bvec,
> +						mirror, error_bitmap);
> +		} else {
> +			/* Update page status and unlock */
> +			end_page_read(page, uptodate, start, len);
> +			endio_readpage_release_extent(&processed,
> +					BTRFS_I(inode), start, end,
> +					PageUptodate(page));

Another reason I'm not introducing the end_sector_io().

Here the code looks super familiar with end_sector_io(), but due to the
@processed interface, it's not compatible with end_sector_io(), or we
will do the unlock with way more io_tree operations with much smaller
range, thus reduces performance.

Thanks,
Qu

> +		}
> +
>   		ASSERT(bio_offset + len > bio_offset);
>   		bio_offset += len;
>
> -		/* Update page status and unlock */
> -		end_page_read(page, uptodate, start, len);
> -		endio_readpage_release_extent(&processed, BTRFS_I(inode),
> -					      start, end, PageUptodate(page));
>   	}
>   	/* Release the last extent */
>   	endio_readpage_release_extent(&processed, NULL, 0, 0, false);