| Message ID | 20250204133044.80551-3-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: incremental per-operation iter advance | expand |
On Tue, Feb 04, 2025 at 08:30:36AM -0500, Brian Foster wrote: > In preparation for more granular iomap_iter advancing, break out > some of the logic associated with higher level iteration from > iomap_advance_iter(). Specifically, factor the iomap reset code into > a separate helper and lift the iomap.length check into the calling > code, similar to how ->iomap_end() calls are handled. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/iter.c | 49 ++++++++++++++++++++++++++----------------------- > 1 file changed, 26 insertions(+), 23 deletions(-) > > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c > index 3790918646af..731ea7267f27 100644 > --- a/fs/iomap/iter.c > +++ b/fs/iomap/iter.c > @@ -7,6 +7,13 @@ > #include <linux/iomap.h> > #include "trace.h" > > +static inline void iomap_iter_reset_iomap(struct iomap_iter *iter) > +{ > + iter->processed = 0; > + memset(&iter->iomap, 0, sizeof(iter->iomap)); > + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); > +} > + > /* > * Advance to the next range we need to map. > * > @@ -14,32 +21,24 @@ > * processed - it was aborted because the extent the iomap spanned may have been > * changed during the operation. In this case, the iteration behaviour is to > * remap the unprocessed range of the iter, and that means we may need to remap > - * even when we've made no progress (i.e. iter->processed = 0). Hence the > - * "finished iterating" case needs to distinguish between > - * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we > - * need to remap the entire remaining range. > + * even when we've made no progress (i.e. count = 0). Hence the "finished > + * iterating" case needs to distinguish between (count = 0) meaning we are done > + * and (count = 0 && stale) meaning we need to remap the entire remaining range. > */ > -static inline int iomap_iter_advance(struct iomap_iter *iter) > +static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count) > { > bool stale = iter->iomap.flags & IOMAP_F_STALE; > int ret = 1; > > - /* handle the previous iteration (if any) */ > - if (iter->iomap.length) { > - if (iter->processed < 0) > - return iter->processed; > - if (WARN_ON_ONCE(iter->processed > iomap_length(iter))) > - return -EIO; > - iter->pos += iter->processed; > - iter->len -= iter->processed; > - if (!iter->len || (!iter->processed && !stale)) > - ret = 0; > - } > + if (count < 0) > + return count; > + if (WARN_ON_ONCE(count > iomap_length(iter))) > + return -EIO; > + iter->pos += count; > + iter->len -= count; > + if (!iter->len || (!count && !stale)) > + ret = 0; > > - /* clear the per iteration state */ > - iter->processed = 0; > - memset(&iter->iomap, 0, sizeof(iter->iomap)); > - memset(&iter->srcmap, 0, sizeof(iter->srcmap)); Are there any consequences to not resetting the iter if iter->iomap.length is zero? I think the answer is "no" because callers are supposed to initialize the iter with zeroes and filesystems are never supposed to return zero-length iomaps from ->begin_iomap, right? If the answers are "no" and "yes" then Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > return ret; > } > > @@ -82,10 +81,14 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > return ret; > } > > + /* advance and clear state from the previous iteration */ > trace_iomap_iter(iter, ops, _RET_IP_); > - ret = iomap_iter_advance(iter); > - if (ret <= 0) > - return ret; > + if (iter->iomap.length) { > + ret = iomap_iter_advance(iter, iter->processed); > + iomap_iter_reset_iomap(iter); > + if (ret <= 0) > + return ret; > + } > > ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags, > &iter->iomap, &iter->srcmap); > -- > 2.48.1 > >
On Tue, Feb 04, 2025 at 11:30:56AM -0800, Darrick J. Wong wrote: > On Tue, Feb 04, 2025 at 08:30:36AM -0500, Brian Foster wrote: > > In preparation for more granular iomap_iter advancing, break out > > some of the logic associated with higher level iteration from > > iomap_advance_iter(). Specifically, factor the iomap reset code into > > a separate helper and lift the iomap.length check into the calling > > code, similar to how ->iomap_end() calls are handled. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > --- > > fs/iomap/iter.c | 49 ++++++++++++++++++++++++++----------------------- > > 1 file changed, 26 insertions(+), 23 deletions(-) > > > > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c > > index 3790918646af..731ea7267f27 100644 > > --- a/fs/iomap/iter.c > > +++ b/fs/iomap/iter.c > > @@ -7,6 +7,13 @@ > > #include <linux/iomap.h> > > #include "trace.h" > > > > +static inline void iomap_iter_reset_iomap(struct iomap_iter *iter) > > +{ > > + iter->processed = 0; > > + memset(&iter->iomap, 0, sizeof(iter->iomap)); > > + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); > > +} > > + > > /* > > * Advance to the next range we need to map. > > * > > @@ -14,32 +21,24 @@ > > * processed - it was aborted because the extent the iomap spanned may have been > > * changed during the operation. In this case, the iteration behaviour is to > > * remap the unprocessed range of the iter, and that means we may need to remap > > - * even when we've made no progress (i.e. iter->processed = 0). Hence the > > - * "finished iterating" case needs to distinguish between > > - * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we > > - * need to remap the entire remaining range. > > + * even when we've made no progress (i.e. count = 0). Hence the "finished > > + * iterating" case needs to distinguish between (count = 0) meaning we are done > > + * and (count = 0 && stale) meaning we need to remap the entire remaining range. > > */ > > -static inline int iomap_iter_advance(struct iomap_iter *iter) > > +static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count) > > { > > bool stale = iter->iomap.flags & IOMAP_F_STALE; > > int ret = 1; > > > > - /* handle the previous iteration (if any) */ > > - if (iter->iomap.length) { > > - if (iter->processed < 0) > > - return iter->processed; > > - if (WARN_ON_ONCE(iter->processed > iomap_length(iter))) > > - return -EIO; > > - iter->pos += iter->processed; > > - iter->len -= iter->processed; > > - if (!iter->len || (!iter->processed && !stale)) > > - ret = 0; > > - } > > + if (count < 0) > > + return count; > > + if (WARN_ON_ONCE(count > iomap_length(iter))) > > + return -EIO; > > + iter->pos += count; > > + iter->len -= count; > > + if (!iter->len || (!count && !stale)) > > + ret = 0; > > > > - /* clear the per iteration state */ > > - iter->processed = 0; > > - memset(&iter->iomap, 0, sizeof(iter->iomap)); > > - memset(&iter->srcmap, 0, sizeof(iter->srcmap)); > > Are there any consequences to not resetting the iter if > iter->iomap.length is zero? I think the answer is "no" because callers > are supposed to initialize the iter with zeroes and filesystems are > never supposed to return zero-length iomaps from ->begin_iomap, right? > That matches my understanding.. > If the answers are "no" and "yes" then > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> > Thanks. Brian > --D > > > return ret; > > } > > > > @@ -82,10 +81,14 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > > return ret; > > } > > > > + /* advance and clear state from the previous iteration */ > > trace_iomap_iter(iter, ops, _RET_IP_); > > - ret = iomap_iter_advance(iter); > > - if (ret <= 0) > > - return ret; > > + if (iter->iomap.length) { > > + ret = iomap_iter_advance(iter, iter->processed); > > + iomap_iter_reset_iomap(iter); > > + if (ret <= 0) > > + return ret; > > + } > > > > ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags, > > &iter->iomap, &iter->srcmap); > > -- > > 2.48.1 > > > > >
diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c index 3790918646af..731ea7267f27 100644 --- a/fs/iomap/iter.c +++ b/fs/iomap/iter.c @@ -7,6 +7,13 @@ #include <linux/iomap.h> #include "trace.h" +static inline void iomap_iter_reset_iomap(struct iomap_iter *iter) +{ + iter->processed = 0; + memset(&iter->iomap, 0, sizeof(iter->iomap)); + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); +} + /* * Advance to the next range we need to map. * @@ -14,32 +21,24 @@ * processed - it was aborted because the extent the iomap spanned may have been * changed during the operation. In this case, the iteration behaviour is to * remap the unprocessed range of the iter, and that means we may need to remap - * even when we've made no progress (i.e. iter->processed = 0). Hence the - * "finished iterating" case needs to distinguish between - * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we - * need to remap the entire remaining range. + * even when we've made no progress (i.e. count = 0). Hence the "finished + * iterating" case needs to distinguish between (count = 0) meaning we are done + * and (count = 0 && stale) meaning we need to remap the entire remaining range. */ -static inline int iomap_iter_advance(struct iomap_iter *iter) +static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count) { bool stale = iter->iomap.flags & IOMAP_F_STALE; int ret = 1; - /* handle the previous iteration (if any) */ - if (iter->iomap.length) { - if (iter->processed < 0) - return iter->processed; - if (WARN_ON_ONCE(iter->processed > iomap_length(iter))) - return -EIO; - iter->pos += iter->processed; - iter->len -= iter->processed; - if (!iter->len || (!iter->processed && !stale)) - ret = 0; - } + if (count < 0) + return count; + if (WARN_ON_ONCE(count > iomap_length(iter))) + return -EIO; + iter->pos += count; + iter->len -= count; + if (!iter->len || (!count && !stale)) + ret = 0; - /* clear the per iteration state */ - iter->processed = 0; - memset(&iter->iomap, 0, sizeof(iter->iomap)); - memset(&iter->srcmap, 0, sizeof(iter->srcmap)); return ret; } @@ -82,10 +81,14 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) return ret; } + /* advance and clear state from the previous iteration */ trace_iomap_iter(iter, ops, _RET_IP_); - ret = iomap_iter_advance(iter); - if (ret <= 0) - return ret; + if (iter->iomap.length) { + ret = iomap_iter_advance(iter, iter->processed); + iomap_iter_reset_iomap(iter); + if (ret <= 0) + return ret; + } ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags, &iter->iomap, &iter->srcmap);