Re: [PATCH v2 14/18] btrfs: extent_io: make endio_readpage_update_page_status() to handle subpage case

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.20 г. 8:39 ч., Qu Wenruo wrote:
> To handle subpage status update, add the following new tricks:
> - Use btrfs_page_*() helpers to update page status
>   Now we can handle both cases well.
> 
> - No page unlock for subpage metadata
>   Since subpage metadata doesn't utilize page locking at all, skip it.
>   For subpage data locking, it's handled in later commits.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1ec9de2aa910..64a19c1884fc 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2841,15 +2841,26 @@ static void endio_readpage_release_extent(struct processed_extent *processed,
>  	processed->uptodate = uptodate;
>  }
>  
> -static void endio_readpage_update_page_status(struct page *page, bool uptodate)
> +static void endio_readpage_update_page_status(struct page *page, bool uptodate,
> +					      u64 start, u64 end)
>  {
> +	struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> +	u32 len;
> +
> +	ASSERT(page_offset(page) <= start &&
> +		end <= page_offset(page) + PAGE_SIZE - 1);

'start' in this case is
'start = page_offset(page) + bvec->bv_offset;' from
end_bio_extent_readpage, so it can't possibly be less than page_offset,
instead it will at least be equal to page_offset if bvec->bv_offset is 0
. However, can we really guarantee this ?


You are using the end only for the assert, and given you already have
the 'len' parameter calculated in the caller I'd rather have this
function take start/len, that would save you from recalculating the len
and also for someone looking at the code it would be apparent it's the
length of the currently processed bvec. I looked through the end of the
series and you never use 'end' just 'len'

<snip>