Re: [PATCH 07/15] btrfs: factor out a helper to end a single sector from submit_data_read_repair

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

On 2022/5/17 22:50, Christoph Hellwig wrote:
> Add a helper to end I/O on a single sector, which will come in handy with
> the new read repair code.

The code looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just one thing to mention, I also considered such refactor, but the main
reason to prevent me doing this is, there are already more than enough
helpers, and without a good enough naming, it can be messy easily.

Although it looks like we're fine with the new helper for now.

Thanks,
Qu
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/extent_io.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 75466747a252c..f96d5b7071813 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2740,6 +2740,20 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
>   		btrfs_subpage_end_reader(fs_info, page, start, len);
>   }
>
> +static void end_sector_io(struct page *page, u64 offset, bool uptodate)
> +{
> +	struct inode *inode = page->mapping->host;
> +	u32 sectorsize = btrfs_sb(inode->i_sb)->sectorsize;
> +	struct extent_state *cached = NULL;
> +
> +	end_page_read(page, uptodate, offset, sectorsize);
> +	if (uptodate)
> +		set_extent_uptodate(&BTRFS_I(inode)->io_tree, offset,
> +				offset + sectorsize - 1, &cached, GFP_ATOMIC);
> +	unlock_extent_cached_atomic(&BTRFS_I(inode)->io_tree, offset,
> +			offset + sectorsize - 1, &cached);
> +}
> +
>   static void submit_data_read_repair(struct inode *inode, struct bio *failed_bio,
>   		u32 bio_offset, const struct bio_vec *bvec, int failed_mirror,
>   		unsigned int error_bitmap)
> @@ -2770,7 +2784,6 @@ static void submit_data_read_repair(struct inode *inode, struct bio *failed_bio,
>   	/* Iterate through all the sectors in the range */
>   	for (i = 0; i < nr_bits; i++) {
>   		const unsigned int offset = i * sectorsize;
> -		struct extent_state *cached = NULL;
>   		bool uptodate = false;
>   		int ret;
>
> @@ -2801,16 +2814,7 @@ static void submit_data_read_repair(struct inode *inode, struct bio *failed_bio,
>   		 * will not be properly unlocked.
>   		 */
>   next:
> -		end_page_read(page, uptodate, start + offset, sectorsize);
> -		if (uptodate)
> -			set_extent_uptodate(&BTRFS_I(inode)->io_tree,
> -					start + offset,
> -					start + offset + sectorsize - 1,
> -					&cached, GFP_ATOMIC);
> -		unlock_extent_cached_atomic(&BTRFS_I(inode)->io_tree,
> -				start + offset,
> -				start + offset + sectorsize - 1,
> -				&cached);
> +		end_sector_io(page, start + offset, uptodate);
>   	}
>   }
>