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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox