Re: [PATCH v3] btrfs: retry verity reads for not-uptodate Merkle folios

[Boris Burkov]

On Mon, Jul 20, 2026 at 02:28:20PM +0800, Yichong Chen wrote:
> btrfs_read_merkle_tree_page() can find a folio in the mapping that is
> not uptodate.  After taking the folio lock, the current code treats that
> state as a read error and returns -EIO.
> 
> That can make a previous transient read failure sticky.  If the failed
> read left a not-uptodate folio in the mapping, later callers find that
> folio and fail instead of retrying the read.
> 
> Keep the existing page-cache insertion and locking order, but retry the
> Merkle item read when a not-uptodate folio is found in the mapping.  Also
> unlock the folio when read_key_bytes() fails so that a later caller can
> lock it and retry the read.
> 
> Fixes: 06ed09351b67 ("btrfs: convert btrfs_read_merkle_tree_page() to use a folio")
> Signed-off-by: Yichong Chen <chenyichong@xxxxxxxxxxxxx>
> ---
> v3:
> - Keep the existing filemap_add_folio() and read ordering.
> - Retry the Merkle item read when a not-uptodate folio is found, as
>   suggested by Boris.
> - Unlock the folio on read_key_bytes() failure so later callers can retry.
> 
> 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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
> index 983365a73541..25f021b04ce4 100644
> --- a/fs/btrfs/verity.c
> +++ b/fs/btrfs/verity.c
> @@ -720,14 +720,17 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
>  			goto out;
>  
>  		folio_lock(folio);
> -		/* If it's not uptodate after we have the lock, we got a read error. */
> -		if (!folio_test_uptodate(folio)) {
> +		/* Folio was truncated from mapping. */
> +		if (!folio->mapping) {
>  			folio_unlock(folio);
>  			folio_put(folio);
> -			return ERR_PTR(-EIO);
> +			goto again;
>  		}
> -		folio_unlock(folio);
> -		goto out;

The filemap comment on this is helpful imo, otherwise it's sort of
surprising to see folio_test_uptodate() getting checked "for no good
reason" twice in a row. First outside the lock (presumably to not bother
locking if uptodate, so that one is an optimization, then we have this
one after we lock to be certain someone hasn't beaten us the the lock
and brought it uptodate.

So maybe a comment on the one before the lock? Or here as in filemap.

Other than that, it looks good, thank you for fixing this.

Reviewed-by: Boris Burkov <boris@xxxxxx>

> +		if (folio_test_uptodate(folio)) {
> +			folio_unlock(folio);
> +			goto out;
> +		}
> +		goto read_folio;
>  	}
>  
>  	folio = filemap_alloc_folio(mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS),
> @@ -744,6 +747,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
>  		return ERR_PTR(ret);
>  	}
>  
> +read_folio:
>  	/*
>  	 * Merkle item keys are indexed from byte 0 in the merkle tree.
>  	 * They have the form:
> @@ -753,6 +757,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
>  	ret = read_key_bytes(BTRFS_I(inode), BTRFS_VERITY_MERKLE_ITEM_KEY, off,
>  			     folio_address(folio), PAGE_SIZE, folio);
>  	if (ret < 0) {
> +		folio_unlock(folio);
>  		folio_put(folio);
>  		return ERR_PTR(ret);
>  	}
> -- 
> 2.51.0