Re: [PATCH v1 02/16] iomap: rename cur_folio_in_bio to folio_unlockedOM

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

On Fri, Aug 29, 2025 at 04:56:13PM -0700, Joanne Koong wrote:
> The purpose of struct iomap_readpage_ctx's cur_folio_in_bio is to track
> if the folio needs to be unlocked or not. Rename this to folio_unlocked
> to make the purpose more clear and so that when iomap read/readahead
> logic is made generic, the name also makes sense for filesystems that
> don't use bios.

Hrmmm.  The problem is, "cur_folio_in_bio" captures the meaning that the
(locked) folio is attached to the bio, so the bio_io_end function has to
unlock the folio.  The readahead context is basically borrowing the
folio and cannot unlock the folio itelf.

The name folio_unlocked doesn't capture the change in ownership, it just
fixates on the lock state which (imo) is a side effect of the folio lock
ownership.

> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/iomap/buffered-io.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index f8bdb2428819..4b173aad04ed 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -352,7 +352,7 @@ static void iomap_read_end_io(struct bio *bio)
>  
>  struct iomap_readpage_ctx {
>  	struct folio		*cur_folio;
> -	bool			cur_folio_in_bio;
> +	bool			folio_unlocked;

Maybe this ought to be called cur_folio_borrowed?

	/*
	 * Folio readahead can transfer ownership of a folio lock to
	 * an external reader (e.g. bios) with the expectation that
	 * the new owner will unlock the folio when the readahead is
	 * complete.  Under these circumstances, the readahead context
	 * is merely borrowing the folio and must not unlock it.
	 */
	bool			cur_folio_borrowed;

>  	struct bio		*bio;
>  	struct readahead_control *rac;
>  };
> @@ -367,7 +367,7 @@ static void iomap_read_folio_range_async(const struct iomap_iter *iter,
>  	loff_t length = iomap_length(iter);
>  	sector_t sector;
>  
> -	ctx->cur_folio_in_bio = true;
> +	ctx->folio_unlocked = true;

	ctx->cur_folio_borrowed = true;

>  	if (ifs) {
>  		spin_lock_irq(&ifs->state_lock);
>  		ifs->read_bytes_pending += plen;
> @@ -480,9 +480,9 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
>  
>  	if (ctx.bio) {
>  		submit_bio(ctx.bio);
> -		WARN_ON_ONCE(!ctx.cur_folio_in_bio);
> +		WARN_ON_ONCE(!ctx.folio_unlocked);
>  	} else {
> -		WARN_ON_ONCE(ctx.cur_folio_in_bio);
> +		WARN_ON_ONCE(ctx.folio_unlocked);
>  		folio_unlock(folio);
>  	}
>  
> @@ -503,13 +503,13 @@ static int iomap_readahead_iter(struct iomap_iter *iter,
>  	while (iomap_length(iter)) {
>  		if (ctx->cur_folio &&
>  		    offset_in_folio(ctx->cur_folio, iter->pos) == 0) {
> -			if (!ctx->cur_folio_in_bio)
> +			if (!ctx->folio_unlocked)
>  				folio_unlock(ctx->cur_folio);
>  			ctx->cur_folio = NULL;
>  		}
>  		if (!ctx->cur_folio) {
>  			ctx->cur_folio = readahead_folio(ctx->rac);
> -			ctx->cur_folio_in_bio = false;
> +			ctx->folio_unlocked = false;

			ctx->cur_folio_borrowed = false;

>  		}
>  		ret = iomap_readpage_iter(iter, ctx);
>  		if (ret)
> @@ -552,10 +552,8 @@ void iomap_readahead(struct readahead_control *rac, const struct iomap_ops *ops)
>  
>  	if (ctx.bio)
>  		submit_bio(ctx.bio);
> -	if (ctx.cur_folio) {
> -		if (!ctx.cur_folio_in_bio)
> -			folio_unlock(ctx.cur_folio);
> -	}
> +	if (ctx.cur_folio && !ctx.folio_unlocked)
> +		folio_unlock(ctx.cur_folio);

	if (ctx.cur_folio && !ctx.cur_folio_borrowed)
		folio_unlock(ctx.cur_folio);

>  }
>  EXPORT_SYMBOL_GPL(iomap_readahead);
>  
> -- 
> 2.47.3
> 
>