From: Brian Foster <bfoster@redhat.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: brauner@kernel.org, hch@infradead.org, djwong@kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v1] iomap: simplify iomap_iter_advance()
Date: Wed, 17 Sep 2025 09:12:00 -0400 [thread overview]
Message-ID: <aMqzoK1BAq0ed-pB@bfoster> (raw)
In-Reply-To: <20250917004001.2602922-1-joannelkoong@gmail.com>
On Tue, Sep 16, 2025 at 05:40:01PM -0700, Joanne Koong wrote:
> Most callers of iomap_iter_advance() do not need the remaining length
> returned. Get rid of the extra iomap_length() call that
> iomap_iter_advance() does. If the caller wants the remaining length,
> they can make the call to iomap_length().
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
Indeed this does clean up some of the calls that create a local var
purely to work around the interface. OTOH, the reason I made the advance
update the length was so it was clear the remaining length would be used
correctly in those looping callers. Otherwise I'm not sure it's clear
the bytes/length value needs to be updated and that felt like an easy
mistake to make to me.
I don't really have a strong preference so I'll defer to whatever the
majority opinion is. I wonder though if something else worth considering
is to rename the current helper to __iomap_iter_advance() (or whatever)
and make your updated variant of iomap_iter_advance() into a wrapper of
it.
Brian
> fs/dax.c | 28 ++++++++++++----------------
> fs/iomap/buffered-io.c | 16 +++++++++-------
> fs/iomap/direct-io.c | 6 +++---
> fs/iomap/iter.c | 14 +++++---------
> fs/iomap/seek.c | 8 ++++----
> include/linux/iomap.h | 6 ++----
> 6 files changed, 35 insertions(+), 43 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 20ecf652c129..29e7a150b6f9 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1534,7 +1534,7 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
>
> /* already zeroed? we're done. */
> if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN)
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
>
> /*
> * invalidate the pages whose sharing state is to be changed
> @@ -1563,9 +1563,10 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
> if (ret < 0)
> return ret;
>
> - ret = iomap_iter_advance(iter, &length);
> + ret = iomap_iter_advance(iter, length);
> if (ret)
> return ret;
> + length = iomap_length(iter);
> } while (length > 0);
>
> if (did_zero)
> @@ -1624,7 +1625,7 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
>
> if (iomap->type == IOMAP_HOLE || iomap->type == IOMAP_UNWRITTEN) {
> done = iov_iter_zero(min(length, end - pos), iter);
> - return iomap_iter_advance(iomi, &done);
> + return iomap_iter_advance(iomi, done);
> }
> }
>
> @@ -1709,11 +1710,12 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
> map_len, iter);
>
> length = xfer;
> - ret = iomap_iter_advance(iomi, &length);
> + ret = iomap_iter_advance(iomi, length);
> if (!ret && xfer == 0)
> ret = -EFAULT;
> if (xfer < map_len)
> break;
> + length = iomap_length(iomi);
> }
> dax_read_unlock(id);
>
> @@ -1946,10 +1948,8 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, unsigned long *pfnp,
> ret |= VM_FAULT_MAJOR;
> }
>
> - if (!(ret & VM_FAULT_ERROR)) {
> - u64 length = PAGE_SIZE;
> - iter.status = iomap_iter_advance(&iter, &length);
> - }
> + if (!(ret & VM_FAULT_ERROR))
> + iter.status = iomap_iter_advance(&iter, PAGE_SIZE);
> }
>
> if (iomap_errp)
> @@ -2061,10 +2061,8 @@ static vm_fault_t dax_iomap_pmd_fault(struct vm_fault *vmf, unsigned long *pfnp,
> continue; /* actually breaks out of the loop */
>
> ret = dax_fault_iter(vmf, &iter, pfnp, &xas, &entry, true);
> - if (ret != VM_FAULT_FALLBACK) {
> - u64 length = PMD_SIZE;
> - iter.status = iomap_iter_advance(&iter, &length);
> - }
> + if (ret != VM_FAULT_FALLBACK)
> + iter.status = iomap_iter_advance(&iter, PMD_SIZE);
> }
>
> unlock_entry:
> @@ -2190,7 +2188,6 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
> const struct iomap *smap = &it_src->iomap;
> const struct iomap *dmap = &it_dest->iomap;
> loff_t pos1 = it_src->pos, pos2 = it_dest->pos;
> - u64 dest_len;
> void *saddr, *daddr;
> int id, ret;
>
> @@ -2223,10 +2220,9 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
> dax_read_unlock(id);
>
> advance:
> - dest_len = len;
> - ret = iomap_iter_advance(it_src, &len);
> + ret = iomap_iter_advance(it_src, len);
> if (!ret)
> - ret = iomap_iter_advance(it_dest, &dest_len);
> + ret = iomap_iter_advance(it_dest, len);
> return ret;
>
> out_unlock:
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index fd827398afd2..fe6588ab0922 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -373,7 +373,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
> ret = iomap_read_inline_data(iter, folio);
> if (ret)
> return ret;
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> }
>
> /* zero post-eof blocks as the page may be mapped */
> @@ -434,7 +434,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
> * iteration.
> */
> length = pos - iter->pos + plen;
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> }
>
> static int iomap_read_folio_iter(struct iomap_iter *iter,
> @@ -1036,7 +1036,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
> }
> } else {
> total_written += written;
> - iomap_iter_advance(iter, &written);
> + iomap_iter_advance(iter, written);
> }
> } while (iov_iter_count(i) && iomap_length(iter));
>
> @@ -1305,7 +1305,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
> int status;
>
> if (!iomap_want_unshare_iter(iter))
> - return iomap_iter_advance(iter, &bytes);
> + return iomap_iter_advance(iter, bytes);
>
> do {
> struct folio *folio;
> @@ -1329,9 +1329,10 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
>
> balance_dirty_pages_ratelimited(iter->inode->i_mapping);
>
> - status = iomap_iter_advance(iter, &bytes);
> + status = iomap_iter_advance(iter, bytes);
> if (status)
> break;
> + bytes = iomap_length(iter);
> } while (bytes > 0);
>
> return status;
> @@ -1404,9 +1405,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
> if (WARN_ON_ONCE(!ret))
> return -EIO;
>
> - status = iomap_iter_advance(iter, &bytes);
> + status = iomap_iter_advance(iter, bytes);
> if (status)
> break;
> + bytes = iomap_length(iter);
> } while (bytes > 0);
>
> if (did_zero)
> @@ -1518,7 +1520,7 @@ static int iomap_folio_mkwrite_iter(struct iomap_iter *iter,
> folio_mark_dirty(folio);
> }
>
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> }
>
> vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops,
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index b84f6af2eb4c..e3544a6719a7 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -496,7 +496,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> /* Undo iter limitation to current extent */
> iov_iter_reexpand(dio->submit.iter, orig_count - copied);
> if (copied)
> - return iomap_iter_advance(iter, &copied);
> + return iomap_iter_advance(iter, copied);
> return ret;
> }
>
> @@ -507,7 +507,7 @@ static int iomap_dio_hole_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> dio->size += length;
> if (!length)
> return -EFAULT;
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> }
>
> static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
> @@ -539,7 +539,7 @@ static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
> dio->size += copied;
> if (!copied)
> return -EFAULT;
> - return iomap_iter_advance(iomi, &copied);
> + return iomap_iter_advance(iomi, copied);
> }
>
> static int iomap_dio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index cef77ca0c20b..91d2024e00da 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -13,17 +13,13 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
> memset(&iter->srcmap, 0, sizeof(iter->srcmap));
> }
>
> -/*
> - * Advance the current iterator position and output the length remaining for the
> - * current mapping.
> - */
> -int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
> +/* Advance the current iterator position and decrement the remaining length */
> +int iomap_iter_advance(struct iomap_iter *iter, u64 count)
> {
> - if (WARN_ON_ONCE(*count > iomap_length(iter)))
> + if (WARN_ON_ONCE(count > iomap_length(iter)))
> return -EIO;
> - iter->pos += *count;
> - iter->len -= *count;
> - *count = iomap_length(iter);
> + iter->pos += count;
> + iter->len -= count;
> return 0;
> }
>
> diff --git a/fs/iomap/seek.c b/fs/iomap/seek.c
> index 56db2dd4b10d..6cbc587c93da 100644
> --- a/fs/iomap/seek.c
> +++ b/fs/iomap/seek.c
> @@ -16,13 +16,13 @@ static int iomap_seek_hole_iter(struct iomap_iter *iter,
> *hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
> iter->pos, iter->pos + length, SEEK_HOLE);
> if (*hole_pos == iter->pos + length)
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> return 0;
> case IOMAP_HOLE:
> *hole_pos = iter->pos;
> return 0;
> default:
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> }
> }
>
> @@ -59,12 +59,12 @@ static int iomap_seek_data_iter(struct iomap_iter *iter,
>
> switch (iter->iomap.type) {
> case IOMAP_HOLE:
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> case IOMAP_UNWRITTEN:
> *hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
> iter->pos, iter->pos + length, SEEK_DATA);
> if (*hole_pos < 0)
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, length);
> return 0;
> default:
> *hole_pos = iter->pos;
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 73dceabc21c8..4469b2318b08 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -245,7 +245,7 @@ struct iomap_iter {
> };
>
> int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
> -int iomap_iter_advance(struct iomap_iter *iter, u64 *count);
> +int iomap_iter_advance(struct iomap_iter *iter, u64 count);
>
> /**
> * iomap_length_trim - trimmed length of the current iomap iteration
> @@ -282,9 +282,7 @@ static inline u64 iomap_length(const struct iomap_iter *iter)
> */
> static inline int iomap_iter_advance_full(struct iomap_iter *iter)
> {
> - u64 length = iomap_length(iter);
> -
> - return iomap_iter_advance(iter, &length);
> + return iomap_iter_advance(iter, iomap_length(iter));
> }
>
> /**
> --
> 2.47.3
>
next prev parent reply other threads:[~2025-09-17 13:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-17 0:40 [PATCH v1] iomap: simplify iomap_iter_advance() Joanne Koong
2025-09-17 13:12 ` Brian Foster [this message]
2025-09-17 21:54 ` Joanne Koong
2025-09-18 11:31 ` Brian Foster
2025-09-18 14:27 ` Christoph Hellwig
2025-09-18 20:18 ` Brian Foster
2025-09-18 22:10 ` Joanne Koong
2025-09-19 11:35 ` Brian Foster
2025-09-19 18:03 ` Joanne Koong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aMqzoK1BAq0ed-pB@bfoster \
--to=bfoster@redhat.com \
--cc=brauner@kernel.org \
--cc=djwong@kernel.org \
--cc=hch@infradead.org \
--cc=joannelkoong@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.