Re: [PATCH 2/2] btrfs: simplify the page uptodate preparation for prepare_pages()

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Sep 30, 2024 at 11:20:30AM +0930, Qu Wenruo wrote:
> Currently inside prepare_pages(), we handle the leading and tailing page
> differently, and skip the middle pages (if any).
> 
> This is to avoid reading pages which is fully covered by the dirty
> range.
> 
> Refactor the code by moving all checks (alignment check, range check,
> force read check) into prepare_uptodate_page().
> 
> So that prepare_pages() only need to iterate all the pages
> unconditionally.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c | 68 +++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 9555a3485670..cc8edf8da79d 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -858,36 +858,46 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>   */
>  static int prepare_uptodate_page(struct inode *inode,
>  				 struct page *page, u64 pos,
> -				 bool force_uptodate)
> +				 u64 len, bool force_uptodate)
>  {
>  	struct folio *folio = page_folio(page);
> +	u64 clamp_start = max_t(u64, pos, folio_pos(folio));
> +	u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
>  	int ret = 0;
>  
> -	if (((pos & (PAGE_SIZE - 1)) || force_uptodate) &&
> -	    !PageUptodate(page)) {
> -		ret = btrfs_read_folio(NULL, folio);
> -		if (ret)
> -			return ret;
> -		lock_page(page);
> -		if (!PageUptodate(page)) {
> -			unlock_page(page);
> -			return -EIO;
> -		}
> +	if (PageUptodate(page))
> +		return 0;
>  
> -		/*
> -		 * Since btrfs_read_folio() will unlock the folio before it
> -		 * returns, there is a window where btrfs_release_folio() can be
> -		 * called to release the page.  Here we check both inode
> -		 * mapping and PagePrivate() to make sure the page was not
> -		 * released.
> -		 *
> -		 * The private flag check is essential for subpage as we need
> -		 * to store extra bitmap using folio private.
> -		 */
> -		if (page->mapping != inode->i_mapping || !folio_test_private(folio)) {
> -			unlock_page(page);
> -			return -EAGAIN;
> -		}
> +	if (force_uptodate)
> +		goto read;
> +
> +	/* The dirty range fully cover the page, no need to read it out. */
> +	if (IS_ALIGNED(clamp_start, PAGE_SIZE) &&
> +	    IS_ALIGNED(clamp_end, PAGE_SIZE))
> +		return 0;
> +read:
> +	ret = btrfs_read_folio(NULL, folio);
> +	if (ret)
> +		return ret;
> +	lock_page(page);
> +	if (!PageUptodate(page)) {
> +		unlock_page(page);
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * Since btrfs_read_folio() will unlock the folio before it
> +	 * returns, there is a window where btrfs_release_folio() can be
> +	 * called to release the page.  Here we check both inode
> +	 * mapping and PagePrivate() to make sure the page was not
> +	 * released.
> +	 *
> +	 * The private flag check is essential for subpage as we need
> +	 * to store extra bitmap using folio private.
> +	 */
> +	if (page->mapping != inode->i_mapping || !folio_test_private(folio)) {
> +		unlock_page(page);
> +		return -EAGAIN;

Since you're reworking it anyway can you go ahead and convert this to only use
the folio?  Thanks, 

Josef