Re: [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jan 26, 2026 at 05:50:53AM +0100, Christoph Hellwig wrote:
> Issuing more reads on errors is not a good idea, especially when the
> most common error here is -ENOMEM.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/verity/pagecache.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/verity/pagecache.c b/fs/verity/pagecache.c
> index 1efcdde20b73..63393f0f5834 100644
> --- a/fs/verity/pagecache.c
> +++ b/fs/verity/pagecache.c
> @@ -22,7 +22,8 @@ struct page *generic_read_merkle_tree_page(struct inode *inode, pgoff_t index,
>  	struct folio *folio;
>  
>  	folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
> -	if (IS_ERR(folio) || !folio_test_uptodate(folio)) {
> +	if (PTR_ERR(folio) == -ENOENT ||
> +	    !(IS_ERR(folio) && !folio_test_uptodate(folio))) {

I don't understand this logic at all.  If @folio is actually an
ERR_PTR, then we dereference the non-folio to see if it's not uptodate?

I think (given the previous revisions) that what you want is to initiate
readahead if either there's no folio at all (ENOENT) or if there is a
folio but it's not uptodate?  But not if there's some other error
(ENOMEM, EL3HLT, EFSCORRUPTED, etc)?

So maybe you want:

	folio = __filemap_get_folio(...);
	if (!IS_ERR(folio)) {
		if (folio_test_uptodate(folio))
			return folio_file_page(folio);
		folio_put(folio);
	} else if (PTR_ERR(folio) == -ENOENT) {
		return ERR_CAST(folio);
	}

	if (num_ra_pages > 1)
		page_cache_ra_unbounded(&ractl, num_ra_pages, 0);
	folio = read_mapping_folio(inode->i_mapping, index, NULL);
	if (IS_ERR(folio))
		return ERR_CAST(folio);

	return folio_file_page(folio);

<confused>

--D

>  		DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
>  
>  		if (!IS_ERR(folio))
> -- 
> 2.47.3
> 
>