Re: [PATCH v3 4/8] iomap: simplify ->read_folio_range() error handling for reads

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

On Tue, Nov 04, 2025 at 12:51:15PM -0800, Joanne Koong wrote:
> Instead of requiring that the caller calls iomap_finish_folio_read()
> even if the ->read_folio_range() callback returns an error, account for
> this internally in iomap instead, which makes the interface simpler and
> makes it match writeback's ->read_folio_range() error handling
> expectations.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  .../filesystems/iomap/operations.rst          |  7 +++--
>  fs/fuse/file.c                                | 10 ++-----
>  fs/iomap/buffered-io.c                        | 27 +++++++++----------
>  include/linux/iomap.h                         |  5 ++--
>  4 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> index 4d30723be7fa..64f4baf5750e 100644
> --- a/Documentation/filesystems/iomap/operations.rst
> +++ b/Documentation/filesystems/iomap/operations.rst
> @@ -149,10 +149,9 @@ These ``struct kiocb`` flags are significant for buffered I/O with iomap:
>  iomap calls these functions:
>  
>    - ``read_folio_range``: Called to read in the range. This must be provided
> -    by the caller. The caller is responsible for calling
> -    iomap_finish_folio_read() after reading in the folio range. This should be
> -    done even if an error is encountered during the read. This returns 0 on
> -    success or a negative error on failure.
> +    by the caller. If this succeeds, iomap_finish_folio_read() must be called
> +    after the range is read in, regardless of whether the read succeeded or
> +    failed.
>  
>    - ``submit_read``: Submit any pending read requests. This function is
>      optional.
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index b343a6f37563..7bcb650a9f26 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -922,13 +922,6 @@ static int fuse_iomap_read_folio_range_async(const struct iomap_iter *iter,
>  
>  	if (ctx->rac) {
>  		ret = fuse_handle_readahead(folio, ctx->rac, data, pos, len);
> -		/*
> -		 * If fuse_handle_readahead was successful, fuse_readpages_end
> -		 * will do the iomap_finish_folio_read, else we need to call it
> -		 * here
> -		 */
> -		if (ret)
> -			iomap_finish_folio_read(folio, off, len, ret);
>  	} else {
>  		/*
>  		 *  for non-readahead read requests, do reads synchronously
> @@ -936,7 +929,8 @@ static int fuse_iomap_read_folio_range_async(const struct iomap_iter *iter,
>  		 *  out-of-order reads
>  		 */
>  		ret = fuse_do_readfolio(file, folio, off, len);
> -		iomap_finish_folio_read(folio, off, len, ret);
> +		if (!ret)
> +			iomap_finish_folio_read(folio, off, len, ret);
>  	}
>  	return ret;
>  }
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index e3171462ba08..0f14d2a91f49 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -398,7 +398,8 @@ static void iomap_read_init(struct folio *folio)
>  		 * finished reading in the entire folio.
>  		 */
>  		spin_lock_irq(&ifs->state_lock);
> -		ifs->read_bytes_pending += len + 1;
> +		WARN_ON_ONCE(ifs->read_bytes_pending != 0);
> +		ifs->read_bytes_pending = len + 1;
>  		spin_unlock_irq(&ifs->state_lock);
>  	}
>  }
> @@ -414,19 +415,8 @@ static void iomap_read_init(struct folio *folio)
>   */
>  static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
>  {
> -	struct iomap_folio_state *ifs;
> -
> -	/*
> -	 * If there are no bytes submitted, this means we are responsible for
> -	 * unlocking the folio here, since no IO helper has taken ownership of
> -	 * it.
> -	 */
> -	if (!bytes_submitted) {
> -		folio_unlock(folio);
> -		return;
> -	}
> +	struct iomap_folio_state *ifs = folio->private;
>  
> -	ifs = folio->private;
>  	if (ifs) {
>  		bool end_read, uptodate;
>  		/*
> @@ -451,6 +441,15 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
>  		spin_unlock_irq(&ifs->state_lock);
>  		if (end_read)
>  			folio_end_read(folio, uptodate);
> +	} else if (!bytes_submitted) {
> +		/*
> +		 * If there were no bytes submitted, this means we are
> +		 * responsible for unlocking the folio here, since no IO helper
> +		 * has taken ownership of it. If there were bytes submitted,
> +		 * then the IO helper will end the read via
> +		 * iomap_finish_folio_read().
> +		 */
> +		folio_unlock(folio);
>  	}
>  }
>  
> @@ -498,10 +497,10 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
>  		} else {
>  			if (!*bytes_submitted)
>  				iomap_read_init(folio);
> -			*bytes_submitted += plen;
>  			ret = ctx->ops->read_folio_range(iter, ctx, plen);
>  			if (ret)
>  				return ret;
> +			*bytes_submitted += plen;

Hrmm.  Is this the main change of this patch?  We don't increment
bytes_submitted if ->read_folio_range returns an error, which then means
that fuse doesn't have to call iomap_finish_folio_read to decrement
*bytes_submitted?

(and apparently the bio read_folio_range can't fail so no changes are
needed there)

--D

>  		}
>  
>  		ret = iomap_iter_advance(iter, plen);
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index b49e47f069db..520e967cb501 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -495,9 +495,8 @@ struct iomap_read_ops {
>  	/*
>  	 * Read in a folio range.
>  	 *
> -	 * The caller is responsible for calling iomap_finish_folio_read() after
> -	 * reading in the folio range. This should be done even if an error is
> -	 * encountered during the read.
> +	 * If this succeeds, iomap_finish_folio_read() must be called after the
> +	 * range is read in, regardless of whether the read succeeded or failed.
>  	 *
>  	 * Returns 0 on success or a negative error on failure.
>  	 */
> -- 
> 2.47.3
> 
>