Re: [PATCH v3 07/15] iomap: track read/readahead folio ownership internally

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

On Tue, Sep 16, 2025 at 04:44:17PM -0700, Joanne Koong wrote:
> The purpose of "struct iomap_read_folio_ctx->cur_folio_in_bio" is to
> track folio ownership to know who is responsible for unlocking it.
> Rename "cur_folio_in_bio" to "cur_folio_owned" to better reflect this
> purpose and so that this can be generically used later on by filesystems
> that are not block-based.

Hrmm, well if this is becoming a private variable, then I'd shorten it
to "folio_owned" since it's not really in the ctx/cur anymore, but I
might be bikeshedding now. :)

> Since "struct iomap_read_folio_ctx" will be made a public interface
> later on when read/readahead takes in caller-provided callbacks, track
> the folio ownership state internally instead of exposing it in "struct
> iomap_read_folio_ctx" to make the interface simpler for end users.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/iomap/buffered-io.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 6c5a631848b7..587bbdbd24bc 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -352,7 +352,6 @@ static void iomap_read_end_io(struct bio *bio)
>  
>  struct iomap_read_folio_ctx {
>  	struct folio		*cur_folio;
> -	bool			cur_folio_in_bio;
>  	void			*read_ctx;
>  	struct readahead_control *rac;
>  };
> @@ -376,7 +375,6 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
>  	sector_t sector;
>  	struct bio *bio = ctx->read_ctx;
>  
> -	ctx->cur_folio_in_bio = true;
>  	if (ifs) {
>  		spin_lock_irq(&ifs->state_lock);
>  		ifs->read_bytes_pending += plen;
> @@ -413,7 +411,7 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
>  }
>  
>  static int iomap_read_folio_iter(struct iomap_iter *iter,
> -		struct iomap_read_folio_ctx *ctx)
> +		struct iomap_read_folio_ctx *ctx, bool *cur_folio_owned)
>  {
>  	const struct iomap *iomap = &iter->iomap;
>  	loff_t pos = iter->pos;
> @@ -450,6 +448,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
>  			folio_zero_range(folio, poff, plen);
>  			iomap_set_range_uptodate(folio, poff, plen);
>  		} else {
> +			*cur_folio_owned = true;
>  			iomap_bio_read_folio_range(iter, ctx, pos, plen);
>  		}
>  
> @@ -472,16 +471,22 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
>  	struct iomap_read_folio_ctx ctx = {
>  		.cur_folio	= folio,
>  	};
> +	/*
> +	 * If an external IO helper takes ownership of the folio, it is
> +	 * responsible for unlocking it when the read completes.

Not sure what "external" means here -- I think for your project it means
a custom folio read method supplied by a filesystem, but for the exist
code (xfs submitting unaltered bios), that part is still mostly internal
to iomap.

If we were *only* refactoring code I would suggest s/external/async/
because that's what the bio code does, but a filesystem supplying its
own folio read function could very well fill the folio synchronously and
it'd still have to unlock the folio.

Maybe just get rid of the word external?  The rest of the code changes
look fine to me.

--D

> +	 */
> +	bool cur_folio_owned = false;
>  	int ret;
>  
>  	trace_iomap_readpage(iter.inode, 1);
>  
>  	while ((ret = iomap_iter(&iter, ops)) > 0)
> -		iter.status = iomap_read_folio_iter(&iter, &ctx);
> +		iter.status = iomap_read_folio_iter(&iter, &ctx,
> +				&cur_folio_owned);
>  
>  	iomap_bio_submit_read(&ctx);
>  
> -	if (!ctx.cur_folio_in_bio)
> +	if (!cur_folio_owned)
>  		folio_unlock(folio);
>  
>  	/*
> @@ -494,12 +499,13 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
>  EXPORT_SYMBOL_GPL(iomap_read_folio);
>  
>  static int iomap_readahead_iter(struct iomap_iter *iter,
> -		struct iomap_read_folio_ctx *ctx)
> +		struct iomap_read_folio_ctx *ctx,
> +		bool *cur_folio_owned)
>  {
>  	int ret;
>  
>  	while (iomap_length(iter)) {
> -		if (ctx->cur_folio && !ctx->cur_folio_in_bio)
> +		if (ctx->cur_folio && !*cur_folio_owned)
>  			folio_unlock(ctx->cur_folio);
>  		ctx->cur_folio = readahead_folio(ctx->rac);
>  		/*
> @@ -508,8 +514,8 @@ static int iomap_readahead_iter(struct iomap_iter *iter,
>  		 */
>  		if (WARN_ON_ONCE(!ctx->cur_folio))
>  			return -EINVAL;
> -		ctx->cur_folio_in_bio = false;
> -		ret = iomap_read_folio_iter(iter, ctx);
> +		*cur_folio_owned = false;
> +		ret = iomap_read_folio_iter(iter, ctx, cur_folio_owned);
>  		if (ret)
>  			return ret;
>  	}
> @@ -542,15 +548,21 @@ void iomap_readahead(struct readahead_control *rac, const struct iomap_ops *ops)
>  	struct iomap_read_folio_ctx ctx = {
>  		.rac	= rac,
>  	};
> +	/*
> +	 * If an external IO helper takes ownership of the folio, it is
> +	 * responsible for unlocking it when the read completes.
> +	 */
> +	bool cur_folio_owned = false;
>  
>  	trace_iomap_readahead(rac->mapping->host, readahead_count(rac));
>  
>  	while (iomap_iter(&iter, ops) > 0)
> -		iter.status = iomap_readahead_iter(&iter, &ctx);
> +		iter.status = iomap_readahead_iter(&iter, &ctx,
> +					&cur_folio_owned);
>  
>  	iomap_bio_submit_read(&ctx);
>  
> -	if (ctx.cur_folio && !ctx.cur_folio_in_bio)
> +	if (ctx.cur_folio && !cur_folio_owned)
>  		folio_unlock(ctx.cur_folio);
>  }
>  EXPORT_SYMBOL_GPL(iomap_readahead);
> -- 
> 2.47.3
> 
>