Re: [PATCH 2/2] iomap: convert iomap_unshare_iter to use large folios

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

"Darrick J. Wong" <djwong@kernel.org> writes:

> From: Darrick J. Wong <djwong@kernel.org>
>
> Convert iomap_unshare_iter to create large folios if possible, since the
> write and zeroing paths already do that.  I think this got missed in the
> conversion of the write paths that landed in 6.6-rc1.
>
> Cc: ritesh.list@gmail.com, willy@infradead.org
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/iomap/buffered-io.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)

Looks right to me. Feel free to add - 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

(small nit below)

>
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 0350830fc989..db889bdfd327 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1263,7 +1263,6 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
>  	loff_t pos = iter->pos;
>  	loff_t length = iomap_length(iter);
> -	long status = 0;
>  	loff_t written = 0;
>  
>  	/* don't bother with blocks that are not shared to start with */
> @@ -1274,9 +1273,10 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
>  		return length;
>  
>  	do {
> -		unsigned long offset = offset_in_page(pos);
> -		unsigned long bytes = min_t(loff_t, PAGE_SIZE - offset, length);
>  		struct folio *folio;
> +		int status;
> +		size_t offset;
> +		size_t bytes = min_t(u64, SIZE_MAX, length);
>  
>  		status = iomap_write_begin(iter, pos, bytes, &folio);
>  		if (unlikely(status))
> @@ -1284,18 +1284,22 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
>  		if (iter->iomap.flags & IOMAP_F_STALE)

Just noticed that we already have "iomap = &iter->iomap" at the start of this
function. We need not dereference for iomap again here.
(Not related to this patch, but I thought I will mention while we are
still at it)

>  			break;
>  
> -		status = iomap_write_end(iter, pos, bytes, bytes, folio);
> -		if (WARN_ON_ONCE(status == 0))
> +		offset = offset_in_folio(folio, pos);
> +		if (bytes > folio_size(folio) - offset)
> +			bytes = folio_size(folio) - offset;
> +
> +		bytes = iomap_write_end(iter, pos, bytes, bytes, folio);
> +		if (WARN_ON_ONCE(bytes == 0))
>  			return -EIO;
>  
>  		cond_resched();
>  
> -		pos += status;
> -		written += status;
> -		length -= status;
> +		pos += bytes;
> +		written += bytes;
> +		length -= bytes;
>  
>  		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
> -	} while (length);
> +	} while (length > 0);
>  
>  	return written;
>  }