Re: [PATCH v2] btrfs: add verity Merkle folio to page cache after reading it

On Tue, Jul 07, 2026 at 01:55:17PM +0800, Yichong Chen wrote:
> btrfs_read_merkle_tree_page() allocates a folio and adds it to the page
> cache before reading the Merkle tree data from btree items.

So does ext4 via (eventually) do_read_cache_folio().

> 
> If read_key_bytes() fails, the folio is still locked and present in the
> page cache.  A later read can find the locked, not-uptodate folio instead
> of retrying the read.

I think the better fix is to fall through to retrying when we see a
not-uptodate folio in the mapping after __filemap_get_folio(), which I
believe is how the ext4 code will behave in that case.

> 
> Avoid installing the folio in the page cache until after the Merkle tree
> read has succeeded.  This keeps the failure path simple: the newly
> allocated folio is not visible to the page cache yet and only needs to be
> released.

I don't think this order is correct. filemap_add_folio() locks the folio
so if you do it in this order, you no longer lock the folio while
reading which is a non-trivial change. I don't immediately see what is
wrong with that off the top of my head for read only verity past eof
pages but it feels sloppy at the very least.

Thanks,
Boris

> 
> Fixes: 06ed09351b67 ("btrfs: convert btrfs_read_merkle_tree_page() to use a folio")
> Signed-off-by: Yichong Chen <[email protected]>
> ---
> v2:
> - Avoid calling filemap_remove_folio(), which is not exported.
> - Add the folio to the page cache only after read_key_bytes() succeeds.
> 
>  fs/btrfs/verity.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
> index 983365a73541..d705ce3b386a 100644
> --- a/fs/btrfs/verity.c
> +++ b/fs/btrfs/verity.c
> @@ -735,15 +735,6 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
>  	if (!folio)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = filemap_add_folio(inode->i_mapping, folio, index, GFP_NOFS);
> -	if (ret) {
> -		folio_put(folio);
> -		/* Did someone else insert a folio here? */
> -		if (ret == -EEXIST)
> -			goto again;
> -		return ERR_PTR(ret);
> -	}
> -
>  	/*
>  	 * Merkle item keys are indexed from byte 0 in the merkle tree.
>  	 * They have the form:
> @@ -759,6 +750,15 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
>  	if (ret < PAGE_SIZE)
>  		folio_zero_segment(folio, ret, PAGE_SIZE);
>  
> +	ret = filemap_add_folio(inode->i_mapping, folio, index, GFP_NOFS);
> +	if (ret) {
> +		folio_put(folio);
> +		/* Did someone else insert a folio here? */
> +		if (ret == -EEXIST)
> +			goto again;
> +		return ERR_PTR(ret);
> +	}
> +
>  	folio_mark_uptodate(folio);
>  	folio_unlock(folio);
>  
> -- 
> 2.51.0