| Message ID | 20210809061244.1196573-12-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/30] iomap: fix a trivial comment typo in trace.h | expand |
On Mon, Aug 09, 2021 at 08:12:25AM +0200, Christoph Hellwig wrote: > The iomap_iter struct provides a convenient way to package up and > maintain all the arguments to the various mapping and operation > functions. It is operated on using the iomap_iter() function that > is called in loop until the whole range has been processed. Compared > to the existing iomap_apply() function this avoid an indirect call > for each iteration. > > For now iomap_iter() calls back into the existing ->iomap_begin and > ->iomap_end methods, but in the future this could be further optimized > to avoid indirect calls entirely. > > Based on an earlier patch from Matthew Wilcox <willy@infradead.org>. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/Makefile | 1 + > fs/iomap/core.c | 79 +++++++++++++++++++++++++++++++++++++++++++ > fs/iomap/trace.h | 37 +++++++++++++++++++- > include/linux/iomap.h | 56 ++++++++++++++++++++++++++++++ > 4 files changed, 172 insertions(+), 1 deletion(-) > create mode 100644 fs/iomap/core.c > > diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile > index eef2722d93a183..6b56b10ded347a 100644 > --- a/fs/iomap/Makefile > +++ b/fs/iomap/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_FS_IOMAP) += iomap.o > > iomap-y += trace.o \ > apply.o \ > + core.o \ This creates a discontinuity in the iomap git history. Can you add these new functions to iomap/apply.c, then when the old apply code is removed later in the series rename the file to core.c? At least that way 'git log --follow fs/iomap/core.c' will walk back into the current history of fs/iomap/apply.c and the older pre-disaggregation fs/iomap.c without having to take the tree back in time to find those files... Cheers, Dave.
On Tue, Aug 10, 2021 at 08:10:47AM +1000, Dave Chinner wrote: > On Mon, Aug 09, 2021 at 08:12:25AM +0200, Christoph Hellwig wrote: > > The iomap_iter struct provides a convenient way to package up and > > maintain all the arguments to the various mapping and operation > > functions. It is operated on using the iomap_iter() function that > > is called in loop until the whole range has been processed. Compared > > to the existing iomap_apply() function this avoid an indirect call > > for each iteration. > > > > For now iomap_iter() calls back into the existing ->iomap_begin and > > ->iomap_end methods, but in the future this could be further optimized > > to avoid indirect calls entirely. > > > > Based on an earlier patch from Matthew Wilcox <willy@infradead.org>. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > fs/iomap/Makefile | 1 + > > fs/iomap/core.c | 79 +++++++++++++++++++++++++++++++++++++++++++ > > fs/iomap/trace.h | 37 +++++++++++++++++++- > > include/linux/iomap.h | 56 ++++++++++++++++++++++++++++++ > > 4 files changed, 172 insertions(+), 1 deletion(-) > > create mode 100644 fs/iomap/core.c > > > > diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile > > index eef2722d93a183..6b56b10ded347a 100644 > > --- a/fs/iomap/Makefile > > +++ b/fs/iomap/Makefile > > @@ -10,6 +10,7 @@ obj-$(CONFIG_FS_IOMAP) += iomap.o > > > > iomap-y += trace.o \ > > apply.o \ > > + core.o \ > > This creates a discontinuity in the iomap git history. Can you add > these new functions to iomap/apply.c, then when the old apply code > is removed later in the series rename the file to core.c? At least > that way 'git log --follow fs/iomap/core.c' will walk back into the > current history of fs/iomap/apply.c and the older pre-disaggregation > fs/iomap.c without having to take the tree back in time to find > those files... ...or put the new code in apply.c, remove iomap_apply, and don't bother with the renaming at all? I don't see much reason to break the git history. This is effectively a new epoch in iomap, but that is plainly obvious from the function declarations. I'll wander through the rest of the unreviewed patches tomorrow morning, these are merely my off-the-cuff impressions. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Aug 09, 2021 at 11:45:09PM -0700, Darrick J. Wong wrote: > > fs/iomap.c without having to take the tree back in time to find > > those files... > > ...or put the new code in apply.c, remove iomap_apply, and don't bother > with the renaming at all? > > I don't see much reason to break the git history. This is effectively a > new epoch in iomap, but that is plainly obvious from the function > declarations. > > I'll wander through the rest of the unreviewed patches tomorrow morning, > these are merely my off-the-cuff impressions. We could do all that, but why? There is no code even left from the apply area.
On Mon, Aug 09, 2021 at 08:12:25AM +0200, Christoph Hellwig wrote: > The iomap_iter struct provides a convenient way to package up and > maintain all the arguments to the various mapping and operation > functions. It is operated on using the iomap_iter() function that > is called in loop until the whole range has been processed. Compared > to the existing iomap_apply() function this avoid an indirect call > for each iteration. > > For now iomap_iter() calls back into the existing ->iomap_begin and > ->iomap_end methods, but in the future this could be further optimized > to avoid indirect calls entirely. > > Based on an earlier patch from Matthew Wilcox <willy@infradead.org>. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/Makefile | 1 + > fs/iomap/core.c | 79 +++++++++++++++++++++++++++++++++++++++++++ > fs/iomap/trace.h | 37 +++++++++++++++++++- > include/linux/iomap.h | 56 ++++++++++++++++++++++++++++++ > 4 files changed, 172 insertions(+), 1 deletion(-) > create mode 100644 fs/iomap/core.c > > diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile > index eef2722d93a183..6b56b10ded347a 100644 > --- a/fs/iomap/Makefile > +++ b/fs/iomap/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_FS_IOMAP) += iomap.o > > iomap-y += trace.o \ > apply.o \ > + core.o \ > buffered-io.o \ > direct-io.o \ > fiemap.o \ > diff --git a/fs/iomap/core.c b/fs/iomap/core.c > new file mode 100644 > index 00000000000000..89a87a1654e8e6 > --- /dev/null > +++ b/fs/iomap/core.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 Christoph Hellwig. > + */ > +#include <linux/fs.h> > +#include <linux/iomap.h> > +#include "trace.h" > + > +static inline int iomap_iter_advance(struct iomap_iter *iter) > +{ > + /* 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) > + return 0; > + } > + > + /* clear the state for the next iteration */ > + iter->processed = 0; > + memset(&iter->iomap, 0, sizeof(iter->iomap)); > + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); > + return 1; > +} > + > +static inline void iomap_iter_done(struct iomap_iter *iter) I wonder why this is a separate function, since it only has debugging warnings and tracepoints? > +{ > + WARN_ON_ONCE(iter->iomap.offset > iter->pos); > + WARN_ON_ONCE(iter->iomap.length == 0); > + WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos); > + > + trace_iomap_iter_dstmap(iter->inode, &iter->iomap); > + if (iter->srcmap.type != IOMAP_HOLE) > + trace_iomap_iter_srcmap(iter->inode, &iter->srcmap); > +} > + > +/** > + * iomap_iter - iterate over a ranges in a file > + * @iter: iteration structue > + * @ops: iomap ops provided by the file system > + * > + * Iterate over filesystem-provided space mappings for the provided file range. > + * > + * This function handles cleanup of resources acquired for iteration when the > + * filesystem indicates there are no more space mappings, which means that this > + * function must be called in a loop that continues as long it returns a > + * positive value. If 0 or a negative value is returned, the caller must not > + * return to the loop body. Within a loop body, there are two ways to break out > + * of the loop body: leave @iter.processed unchanged, or set it to a negative > + * errno. Thanks for improving the documentation. Modulo the question about iomap_iter_done, I guess this looks all right to me. As far as apply.c vs. core.c, I'm not wildly passionate about either naming choice (I would have called it iter.c) but ... fmeh. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + */ > +int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > +{ > + int ret; > + > + if (iter->iomap.length && ops->iomap_end) { > + ret = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter), > + iter->processed > 0 ? iter->processed : 0, > + iter->flags, &iter->iomap); > + if (ret < 0 && !iter->processed) > + return ret; > + } > + > + trace_iomap_iter(iter, ops, _RET_IP_); > + ret = iomap_iter_advance(iter); > + if (ret <= 0) > + return ret; > + > + ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags, > + &iter->iomap, &iter->srcmap); > + if (ret < 0) > + return ret; > + iomap_iter_done(iter); > + return 1; > +} > diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h > index e9cd5cc0d6ba40..1012d7af6b689b 100644 > --- a/fs/iomap/trace.h > +++ b/fs/iomap/trace.h > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > /* > - * Copyright (c) 2009-2019 Christoph Hellwig > + * Copyright (c) 2009-2021 Christoph Hellwig > * > * NOTE: none of these tracepoints shall be considered a stable kernel ABI > * as they can change at any time. > @@ -140,6 +140,8 @@ DEFINE_EVENT(iomap_class, name, \ > TP_ARGS(inode, iomap)) > DEFINE_IOMAP_EVENT(iomap_apply_dstmap); > DEFINE_IOMAP_EVENT(iomap_apply_srcmap); > +DEFINE_IOMAP_EVENT(iomap_iter_dstmap); > +DEFINE_IOMAP_EVENT(iomap_iter_srcmap); > > TRACE_EVENT(iomap_apply, > TP_PROTO(struct inode *inode, loff_t pos, loff_t length, > @@ -179,6 +181,39 @@ TRACE_EVENT(iomap_apply, > __entry->actor) > ); > > +TRACE_EVENT(iomap_iter, > + TP_PROTO(struct iomap_iter *iter, const void *ops, > + unsigned long caller), > + TP_ARGS(iter, ops, caller), > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(u64, ino) > + __field(loff_t, pos) > + __field(loff_t, length) > + __field(unsigned int, flags) > + __field(const void *, ops) > + __field(unsigned long, caller) > + ), > + TP_fast_assign( > + __entry->dev = iter->inode->i_sb->s_dev; > + __entry->ino = iter->inode->i_ino; > + __entry->pos = iter->pos; > + __entry->length = iomap_length(iter); > + __entry->flags = iter->flags; > + __entry->ops = ops; > + __entry->caller = caller; > + ), > + TP_printk("dev %d:%d ino 0x%llx pos %lld length %lld flags %s (0x%x) ops %ps caller %pS", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + __entry->ino, > + __entry->pos, > + __entry->length, > + __print_flags(__entry->flags, "|", IOMAP_FLAGS_STRINGS), > + __entry->flags, > + __entry->ops, > + (void *)__entry->caller) > +); > + > #endif /* _IOMAP_TRACE_H */ > > #undef TRACE_INCLUDE_PATH > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index 76bfc5d16ef49d..aac4176ea16439 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -161,6 +161,62 @@ struct iomap_ops { > ssize_t written, unsigned flags, struct iomap *iomap); > }; > > +/** > + * struct iomap_iter - Iterate through a range of a file > + * @inode: Set at the start of the iteration and should not change. > + * @pos: The current file position we are operating on. It is updated by > + * calls to iomap_iter(). Treat as read-only in the body. > + * @len: The remaining length of the file segment we're operating on. > + * It is updated at the same time as @pos. > + * @processed: The number of bytes processed by the body in the most recent > + * iteration, or a negative errno. 0 causes the iteration to stop. > + * @flags: Zero or more of the iomap_begin flags above. > + * @iomap: Map describing the I/O iteration > + * @srcmap: Source map for COW operations > + */ > +struct iomap_iter { > + struct inode *inode; > + loff_t pos; > + u64 len; > + s64 processed; > + unsigned flags; > + struct iomap iomap; > + struct iomap srcmap; > +}; > + > +int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops); > + > +/** > + * iomap_length - length of the current iomap iteration > + * @iter: iteration structure > + * > + * Returns the length that the operation applies to for the current iteration. > + */ > +static inline u64 iomap_length(const struct iomap_iter *iter) > +{ > + u64 end = iter->iomap.offset + iter->iomap.length; > + > + if (iter->srcmap.type != IOMAP_HOLE) > + end = min(end, iter->srcmap.offset + iter->srcmap.length); > + return min(iter->len, end - iter->pos); > +} > + > +/** > + * iomap_iter_srcmap - return the source map for the current iomap iteration > + * @i: iteration structure > + * > + * Write operations on file systems with reflink support might require a > + * source and a destination map. This function retourns the source map > + * for a given operation, which may or may no be identical to the destination > + * map in &i->iomap. > + */ > +static inline struct iomap *iomap_iter_srcmap(struct iomap_iter *i) > +{ > + if (i->srcmap.type != IOMAP_HOLE) > + return &i->srcmap; > + return &i->iomap; > +} > + > /* > * Main iomap iterator function. > */ > -- > 2.30.2 >
On Tue, Aug 10, 2021 at 05:31:18PM -0700, Darrick J. Wong wrote: > > +static inline void iomap_iter_done(struct iomap_iter *iter) > > I wonder why this is a separate function, since it only has debugging > warnings and tracepoints? The reason for these two sub-helpers was to force me to structure the code so that Matthews original idea of replacing ->iomap_begin and ->iomap_end with a single next callback so that iomap_iter could be inlined into callers and the indirect calls could be elided is still possible. This would only be useful for a few specific methods (probably dax and direct I/O) where we care so much, but it seemed like a nice idea conceptually so I would not want to break it. OTOH we could just remove this function for now and do that once needed. > Modulo the question about iomap_iter_done, I guess this looks all right > to me. As far as apply.c vs. core.c, I'm not wildly passionate about > either naming choice (I would have called it iter.c) but ... fmeh. iter.c is also my preference, but in the end I don't care too much.
On Wed, Aug 11, 2021 at 07:38:56AM +0200, Christoph Hellwig wrote: > On Tue, Aug 10, 2021 at 05:31:18PM -0700, Darrick J. Wong wrote: > > > +static inline void iomap_iter_done(struct iomap_iter *iter) > > > > I wonder why this is a separate function, since it only has debugging > > warnings and tracepoints? > > The reason for these two sub-helpers was to force me to structure the > code so that Matthews original idea of replacing ->iomap_begin and > ->iomap_end with a single next callback so that iomap_iter could > be inlined into callers and the indirect calls could be elided is > still possible. This would only be useful for a few specific > methods (probably dax and direct I/O) where we care so much, but it > seemed like a nice idea conceptually so I would not want to break it. > > OTOH we could just remove this function for now and do that once needed. <shrug> > > Modulo the question about iomap_iter_done, I guess this looks all right > > to me. As far as apply.c vs. core.c, I'm not wildly passionate about > > either naming choice (I would have called it iter.c) but ... fmeh. > > iter.c is also my preference, but in the end I don't care too much. Ok. My plan for this is to change this patch to add the new iter code to apply.c, and change patch 24 to remove iomap_apply. I'll add a patch on the end to rename apply.c to iter.c, which will avoid breaking the history. I'll send the updated patches as replies to this series to avoid spamming the list, since I also have a patchset of bugfixes to send out and don't want to overwhelm everyone. --D
On Wed, Aug 11, 2021 at 12:17:08PM -0700, Darrick J. Wong wrote: > > iter.c is also my preference, but in the end I don't care too much. > > Ok. My plan for this is to change this patch to add the new iter code > to apply.c, and change patch 24 to remove iomap_apply. I'll add a patch > on the end to rename apply.c to iter.c, which will avoid breaking the > history. What history? There is no shared code, so no shared history and. > > I'll send the updated patches as replies to this series to avoid > spamming the list, since I also have a patchset of bugfixes to send out > and don't want to overwhelm everyone. Just as a clear statement: I think this dance is obsfucation and doesn't help in any way. But if that's what it takes..
On Thu, Aug 12, 2021 at 08:49:14AM +0200, Christoph Hellwig wrote: > On Wed, Aug 11, 2021 at 12:17:08PM -0700, Darrick J. Wong wrote: > > > iter.c is also my preference, but in the end I don't care too much. > > > > Ok. My plan for this is to change this patch to add the new iter code > > to apply.c, and change patch 24 to remove iomap_apply. I'll add a patch > > on the end to rename apply.c to iter.c, which will avoid breaking the > > history. > > What history? There is no shared code, so no shared history and. The history of the gluecode that enables us to walk a bunch of extent mappings. In the beginning it was the _apply function, but now in our spectre-weary world, you've switched it to a direct loop to reduce the number of indirect calls in the hot path by 30-50%. As you correctly point out, there's no /code/ shared by the two implementations, but Dave and I would like to preserve the continuity from one to the next. > > I'll send the updated patches as replies to this series to avoid > > spamming the list, since I also have a patchset of bugfixes to send out > > and don't want to overwhelm everyone. > > Just as a clear statement: I think this dance is obsfucation and doesn't > help in any way. But if that's what it takes.. I /would/ appreciate it if you'd rvb (or at least ack) patch 31 so I can get the 5.15 iomap changes finalized next week. Pretty please? :) --D
On Thu, Aug 12, 2021 at 11:20:17AM -0700, Darrick J. Wong wrote: > The history of the gluecode that enables us to walk a bunch of extent > mappings. In the beginning it was the _apply function, but now in our > spectre-weary world, you've switched it to a direct loop to reduce the > number of indirect calls in the hot path by 30-50%. > > As you correctly point out, there's no /code/ shared by the two > implementations, but Dave and I would like to preserve the continuity > from one to the next. > > > > I'll send the updated patches as replies to this series to avoid > > > spamming the list, since I also have a patchset of bugfixes to send out > > > and don't want to overwhelm everyone. > > > > Just as a clear statement: I think this dance is obsfucation and doesn't > > help in any way. But if that's what it takes.. > > I /would/ appreciate it if you'd rvb (or at least ack) patch 31 so I can > get the 5.15 iomap changes finalized next week. Pretty please? :) I think it is a really stupid idea, so certainly no rvb or ack from me. If you feel you want to do it this way go ahead, but I do not in any way approve of it.
On Sun, Aug 8, 2021 at 11:23 PM Christoph Hellwig <hch@lst.de> wrote: > > The iomap_iter struct provides a convenient way to package up and > maintain all the arguments to the various mapping and operation > functions. It is operated on using the iomap_iter() function that > is called in loop until the whole range has been processed. Compared > to the existing iomap_apply() function this avoid an indirect call > for each iteration. > > For now iomap_iter() calls back into the existing ->iomap_begin and > ->iomap_end methods, but in the future this could be further optimized > to avoid indirect calls entirely. > > Based on an earlier patch from Matthew Wilcox <willy@infradead.org>. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/Makefile | 1 + > fs/iomap/core.c | 79 +++++++++++++++++++++++++++++++++++++++++++ > fs/iomap/trace.h | 37 +++++++++++++++++++- > include/linux/iomap.h | 56 ++++++++++++++++++++++++++++++ > 4 files changed, 172 insertions(+), 1 deletion(-) > create mode 100644 fs/iomap/core.c > > diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile > index eef2722d93a183..6b56b10ded347a 100644 > --- a/fs/iomap/Makefile > +++ b/fs/iomap/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_FS_IOMAP) += iomap.o > > iomap-y += trace.o \ > apply.o \ > + core.o \ > buffered-io.o \ > direct-io.o \ > fiemap.o \ > diff --git a/fs/iomap/core.c b/fs/iomap/core.c > new file mode 100644 > index 00000000000000..89a87a1654e8e6 > --- /dev/null > +++ b/fs/iomap/core.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 Christoph Hellwig. > + */ > +#include <linux/fs.h> > +#include <linux/iomap.h> > +#include "trace.h" > + > +static inline int iomap_iter_advance(struct iomap_iter *iter) > +{ > + /* 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) > + return 0; > + } > + > + /* clear the state for the next iteration */ > + iter->processed = 0; > + memset(&iter->iomap, 0, sizeof(iter->iomap)); > + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); > + return 1; > +} > + > +static inline void iomap_iter_done(struct iomap_iter *iter) > +{ > + WARN_ON_ONCE(iter->iomap.offset > iter->pos); > + WARN_ON_ONCE(iter->iomap.length == 0); > + WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos); > + > + trace_iomap_iter_dstmap(iter->inode, &iter->iomap); > + if (iter->srcmap.type != IOMAP_HOLE) > + trace_iomap_iter_srcmap(iter->inode, &iter->srcmap); Given most of the iomap_iter users don't care about srcmap, i.e. are not COW cases, they are leaving srcmap zero initialized. Should the IOMAP types be incremented by one so that there is no IOMAP_HOLE confusion? In other words, fold something like this? diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 479c1da3e221..b9c62d0909b0 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -21,14 +21,23 @@ struct page; struct vm_area_struct; struct vm_fault; -/* - * Types of block ranges for iomap mappings: +/** + * enum iomap_type - Types of block ranges for iomap mappings + * @IOMAP_NONE: invalid iomap + * @IOMAP_HOLE: no blocks allocated, need allocation + * @IOMAP_DELALLOC: delayed allocation blocks + * @IOMAP_MAPPED: blocks allocated at @addr + * @IOMAP_UNWRITTEN: blocks allocated at @addr in unwritten state + * @IOMAP_INLINE: data inline in the inode */ -#define IOMAP_HOLE 0 /* no blocks allocated, need allocation */ -#define IOMAP_DELALLOC 1 /* delayed allocation blocks */ -#define IOMAP_MAPPED 2 /* blocks allocated at @addr */ -#define IOMAP_UNWRITTEN 3 /* blocks allocated at @addr in unwritten state */ -#define IOMAP_INLINE 4 /* data inline in the inode */ +enum iomap_type { + IOMAP_NONE = 0, + IOMAP_HOLE, + IOMAP_DELALLOC + IOMAP_MAPPED, + IOMAP_UNWRITTEN, + IOMAP_INLINE, +}; /* * Flags reported by the file system from iomap_begin: > +} > + > +/** > + * iomap_iter - iterate over a ranges in a file > + * @iter: iteration structue s/structue/structure/ Other than the minor stuff above, looks good to me: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On Thu, Aug 19, 2021 at 02:25:52PM -0700, Dan Williams wrote: > Given most of the iomap_iter users don't care about srcmap, i.e. are > not COW cases, they are leaving srcmap zero initialized. Should the > IOMAP types be incremented by one so that there is no IOMAP_HOLE > confusion? In other words, fold something like this? A hole really means nothing to read from the source. The existing code also relies on that.
On Thu, Aug 19, 2021 at 9:12 PM Christoph Hellwig <hch@lst.de> wrote: > > On Thu, Aug 19, 2021 at 02:25:52PM -0700, Dan Williams wrote: > > Given most of the iomap_iter users don't care about srcmap, i.e. are > > not COW cases, they are leaving srcmap zero initialized. Should the > > IOMAP types be incremented by one so that there is no IOMAP_HOLE > > confusion? In other words, fold something like this? > > A hole really means nothing to read from the source. The existing code > also relies on that. Ok, I've since found iomap_iter_srcmap(). Sorry for the noise.
diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile index eef2722d93a183..6b56b10ded347a 100644 --- a/fs/iomap/Makefile +++ b/fs/iomap/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_FS_IOMAP) += iomap.o iomap-y += trace.o \ apply.o \ + core.o \ buffered-io.o \ direct-io.o \ fiemap.o \ diff --git a/fs/iomap/core.c b/fs/iomap/core.c new file mode 100644 index 00000000000000..89a87a1654e8e6 --- /dev/null +++ b/fs/iomap/core.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2021 Christoph Hellwig. + */ +#include <linux/fs.h> +#include <linux/iomap.h> +#include "trace.h" + +static inline int iomap_iter_advance(struct iomap_iter *iter) +{ + /* 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) + return 0; + } + + /* clear the state for the next iteration */ + iter->processed = 0; + memset(&iter->iomap, 0, sizeof(iter->iomap)); + memset(&iter->srcmap, 0, sizeof(iter->srcmap)); + return 1; +} + +static inline void iomap_iter_done(struct iomap_iter *iter) +{ + WARN_ON_ONCE(iter->iomap.offset > iter->pos); + WARN_ON_ONCE(iter->iomap.length == 0); + WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos); + + trace_iomap_iter_dstmap(iter->inode, &iter->iomap); + if (iter->srcmap.type != IOMAP_HOLE) + trace_iomap_iter_srcmap(iter->inode, &iter->srcmap); +} + +/** + * iomap_iter - iterate over a ranges in a file + * @iter: iteration structue + * @ops: iomap ops provided by the file system + * + * Iterate over filesystem-provided space mappings for the provided file range. + * + * This function handles cleanup of resources acquired for iteration when the + * filesystem indicates there are no more space mappings, which means that this + * function must be called in a loop that continues as long it returns a + * positive value. If 0 or a negative value is returned, the caller must not + * return to the loop body. Within a loop body, there are two ways to break out + * of the loop body: leave @iter.processed unchanged, or set it to a negative + * errno. + */ +int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) +{ + int ret; + + if (iter->iomap.length && ops->iomap_end) { + ret = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter), + iter->processed > 0 ? iter->processed : 0, + iter->flags, &iter->iomap); + if (ret < 0 && !iter->processed) + return ret; + } + + trace_iomap_iter(iter, ops, _RET_IP_); + ret = iomap_iter_advance(iter); + if (ret <= 0) + return ret; + + ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags, + &iter->iomap, &iter->srcmap); + if (ret < 0) + return ret; + iomap_iter_done(iter); + return 1; +} diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h index e9cd5cc0d6ba40..1012d7af6b689b 100644 --- a/fs/iomap/trace.h +++ b/fs/iomap/trace.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Copyright (c) 2009-2019 Christoph Hellwig + * Copyright (c) 2009-2021 Christoph Hellwig * * NOTE: none of these tracepoints shall be considered a stable kernel ABI * as they can change at any time. @@ -140,6 +140,8 @@ DEFINE_EVENT(iomap_class, name, \ TP_ARGS(inode, iomap)) DEFINE_IOMAP_EVENT(iomap_apply_dstmap); DEFINE_IOMAP_EVENT(iomap_apply_srcmap); +DEFINE_IOMAP_EVENT(iomap_iter_dstmap); +DEFINE_IOMAP_EVENT(iomap_iter_srcmap); TRACE_EVENT(iomap_apply, TP_PROTO(struct inode *inode, loff_t pos, loff_t length, @@ -179,6 +181,39 @@ TRACE_EVENT(iomap_apply, __entry->actor) ); +TRACE_EVENT(iomap_iter, + TP_PROTO(struct iomap_iter *iter, const void *ops, + unsigned long caller), + TP_ARGS(iter, ops, caller), + TP_STRUCT__entry( + __field(dev_t, dev) + __field(u64, ino) + __field(loff_t, pos) + __field(loff_t, length) + __field(unsigned int, flags) + __field(const void *, ops) + __field(unsigned long, caller) + ), + TP_fast_assign( + __entry->dev = iter->inode->i_sb->s_dev; + __entry->ino = iter->inode->i_ino; + __entry->pos = iter->pos; + __entry->length = iomap_length(iter); + __entry->flags = iter->flags; + __entry->ops = ops; + __entry->caller = caller; + ), + TP_printk("dev %d:%d ino 0x%llx pos %lld length %lld flags %s (0x%x) ops %ps caller %pS", + MAJOR(__entry->dev), MINOR(__entry->dev), + __entry->ino, + __entry->pos, + __entry->length, + __print_flags(__entry->flags, "|", IOMAP_FLAGS_STRINGS), + __entry->flags, + __entry->ops, + (void *)__entry->caller) +); + #endif /* _IOMAP_TRACE_H */ #undef TRACE_INCLUDE_PATH diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 76bfc5d16ef49d..aac4176ea16439 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -161,6 +161,62 @@ struct iomap_ops { ssize_t written, unsigned flags, struct iomap *iomap); }; +/** + * struct iomap_iter - Iterate through a range of a file + * @inode: Set at the start of the iteration and should not change. + * @pos: The current file position we are operating on. It is updated by + * calls to iomap_iter(). Treat as read-only in the body. + * @len: The remaining length of the file segment we're operating on. + * It is updated at the same time as @pos. + * @processed: The number of bytes processed by the body in the most recent + * iteration, or a negative errno. 0 causes the iteration to stop. + * @flags: Zero or more of the iomap_begin flags above. + * @iomap: Map describing the I/O iteration + * @srcmap: Source map for COW operations + */ +struct iomap_iter { + struct inode *inode; + loff_t pos; + u64 len; + s64 processed; + unsigned flags; + struct iomap iomap; + struct iomap srcmap; +}; + +int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops); + +/** + * iomap_length - length of the current iomap iteration + * @iter: iteration structure + * + * Returns the length that the operation applies to for the current iteration. + */ +static inline u64 iomap_length(const struct iomap_iter *iter) +{ + u64 end = iter->iomap.offset + iter->iomap.length; + + if (iter->srcmap.type != IOMAP_HOLE) + end = min(end, iter->srcmap.offset + iter->srcmap.length); + return min(iter->len, end - iter->pos); +} + +/** + * iomap_iter_srcmap - return the source map for the current iomap iteration + * @i: iteration structure + * + * Write operations on file systems with reflink support might require a + * source and a destination map. This function retourns the source map + * for a given operation, which may or may no be identical to the destination + * map in &i->iomap. + */ +static inline struct iomap *iomap_iter_srcmap(struct iomap_iter *i) +{ + if (i->srcmap.type != IOMAP_HOLE) + return &i->srcmap; + return &i->iomap; +} + /* * Main iomap iterator function. */
The iomap_iter struct provides a convenient way to package up and maintain all the arguments to the various mapping and operation functions. It is operated on using the iomap_iter() function that is called in loop until the whole range has been processed. Compared to the existing iomap_apply() function this avoid an indirect call for each iteration. For now iomap_iter() calls back into the existing ->iomap_begin and ->iomap_end methods, but in the future this could be further optimized to avoid indirect calls entirely. Based on an earlier patch from Matthew Wilcox <willy@infradead.org>. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/Makefile | 1 + fs/iomap/core.c | 79 +++++++++++++++++++++++++++++++++++++++++++ fs/iomap/trace.h | 37 +++++++++++++++++++- include/linux/iomap.h | 56 ++++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 fs/iomap/core.c