Re: [PATCH v5 09/10] iomap: advance the iter directly on unshare range

Darrick J. Wong
2025-02-05T19:16:11Z

On Wed, Feb 05, 2025 at 08:58:20AM -0500, Brian Foster wrote:
> Modify unshare range to advance the iter directly. Replace the local
> pos and length calculations with direct advances and loop based on
> iter state instead.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/iomap/buffered-io.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 678c189faa58..f953bf66beb1 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1267,20 +1267,19 @@ EXPORT_SYMBOL_GPL(iomap_write_delalloc_release);
> static loff_t iomap_unshare_iter(struct iomap_iter *iter)
> {
> struct iomap *iomap = &iter->iomap;
> - loff_t pos = iter->pos;
> - loff_t length = iomap_length(iter);
> - loff_t written = 0;
> + u64 bytes = iomap_length(iter);
> + int status;
>
> if (!iomap_want_unshare_iter(iter))
> - return length;
> + return iomap_iter_advance(iter, &bytes);
>
> do {
> struct folio *folio;
> - int status;
> size_t offset;
> - size_t bytes = min_t(u64, SIZE_MAX, length);
> + loff_t pos = iter->pos;

Do we still need the local variable here?

Otherwise looks right to me, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> bool ret;
>
> + bytes = min_t(u64, SIZE_MAX, bytes);
> status = iomap_write_begin(iter, pos, bytes, &folio);
> if (unlikely(status))
> return status;
> @@ -1298,14 +1297,14 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
>
> cond_resched();
>
> - pos += bytes;
> - written += bytes;
> - length -= bytes;
> -
> balance_dirty_pages_ratelimited(iter->inode->i_mapping);
> - } while (length > 0);
>
> - return written;
> + status = iomap_iter_advance(iter, &bytes);
> + if (status)
> + break;
> + } while (bytes > 0);
> +
> + return status;
> }
>
> int
> --
> 2.48.1
>
>