[RFC 0/2] Begin switching iomap from apply to iter

[RFC 0/2] Begin switching iomap from apply to iter

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

I don't like the 'apply' design pattern.  It splits the logic between
two functions, and from a performance point of view, it introduces an
indirect function call which is more expensive due to Spectre.  This rfc
only converts one function over to use the new API as a demonstration.
I don't want to waste time on it if people aren't interested in this
approach.

I haven't tested this at all; it's probably buggy.

Matthew Wilcox (Oracle) (2):
  iomap: Add iomap_iter API
  iomap: Convert page_mkwrite to iter API

 fs/iomap/Makefile      |  2 +-
 fs/iomap/buffered-io.c | 51 ++++++++++----------------
 fs/iomap/iter.c        | 81 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/iomap.h  | 24 +++++++++++++
 4 files changed, 125 insertions(+), 33 deletions(-)
 create mode 100644 fs/iomap/iter.c

-- 
2.25.1

[PATCH 1/2] iomap: Add iomap_iter API

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

To replace the apply API

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/Makefile     |  2 +-
 fs/iomap/iter.c       | 81 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/iomap.h | 24 +++++++++++++
 3 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 fs/iomap/iter.c

diff --git a/fs/iomap/Makefile b/fs/iomap/Makefile
index eef2722d93a1..477e5e79f874 100644
--- a/fs/iomap/Makefile
+++ b/fs/iomap/Makefile
@@ -9,7 +9,7 @@ ccflags-y += -I $(srctree)/$(src)		# needed for trace events
 obj-$(CONFIG_FS_IOMAP)		+= iomap.o
 
 iomap-y				+= trace.o \
-				   apply.o \
+				   apply.o iter.o \
 				   buffered-io.o \
 				   direct-io.o \
 				   fiemap.o \
diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
new file mode 100644
index 000000000000..1d668fdd928e
--- /dev/null
+++ b/fs/iomap/iter.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ * Copyright (c) 2016-2018 Christoph Hellwig.
+ */
+#include <linux/module.h>
+#include <linux/compiler.h>
+#include <linux/fs.h>
+#include <linux/iomap.h>
+#include "trace.h"
+
+/*
+ * Execute a iomap write on a segment of the mapping that spans a
+ * contiguous range of pages that have identical block mapping state.
+ *
+ * This avoids the need to map pages individually, do individual allocations
+ * for each page and most importantly avoid the need for filesystem specific
+ * locking per page. Instead, all the operations are amortised over the entire
+ * range of pages. It is assumed that the filesystems will lock whatever
+ * resources they require in the iomap_begin call, and release them in the
+ * iomap_end call.
+ */
+loff_t iomap_iter(struct iomap_iter *iter, loff_t written)
+{
+	const struct iomap_ops *ops = iter->ops;
+	struct iomap *iomap = &iter->iomap;
+	struct iomap *srcmap = &iter->srcmap;
+	loff_t end, ret = 0;
+
+	trace_iomap_apply(iter->inode, iter->pos, iter->len, iter->flags,
+			iter->ops, NULL, _RET_IP_);
+
+	if (written != IOMAP_FIRST_CALL) {
+		if (ops->iomap_end)
+			ret = ops->iomap_end(iter->inode, iter->pos,
+					iter->len, written > 0 ? written : 0,
+					iter->flags, iomap);
+		if (written < 0)
+			return written;
+		if (ret < 0)
+			return ret;
+		iter->pos += written;
+		iter->len -= written;
+	}
+
+	/*
+	 * Need to map a range from start position for length bytes. This can
+	 * span multiple pages - it is only guaranteed to return a range of a
+	 * single type of pages (e.g. all into a hole, all mapped or all
+	 * unwritten). Failure at this point has nothing to undo.
+	 *
+	 * If allocation is required for this range, reserve the space now so
+	 * that the allocation is guaranteed to succeed later on. Once we copy
+	 * the data into the page cache pages, then we cannot fail otherwise we
+	 * expose transient stale data. If the reserve fails, we can safely
+	 * back out at this point as there is nothing to undo.
+	 */
+	ret = ops->iomap_begin(iter->inode, iter->pos, iter->len,
+			iter->flags, iomap, srcmap);
+	if (ret)
+		return ret;
+	if (WARN_ON(iomap->offset > iter->pos))
+		return -EIO;
+	if (WARN_ON(iomap->offset + iomap->length <= iter->pos))
+		return -EIO;
+	if (WARN_ON(iomap->length == 0))
+		return -EIO;
+
+	trace_iomap_apply_dstmap(iter->inode, iomap);
+	if (srcmap->type != IOMAP_HOLE)
+		trace_iomap_apply_srcmap(iter->inode, srcmap);
+
+	/*
+	 * Cut down the length to the one actually provided by the filesystem,
+	 * as it might not be able to give us the whole size that we requested.
+	 */
+	end = iomap->offset + iomap->length;
+	if (srcmap->type != IOMAP_HOLE)
+		end = min_t(loff_t, end, srcmap->offset + srcmap->length);
+	return min(iter->len, end - iter->pos);
+}
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 8b09463dae0d..ec00a2268f14 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -142,6 +142,30 @@ struct iomap_ops {
 			ssize_t written, unsigned flags, struct iomap *iomap);
 };
 
+struct iomap_iter {
+	struct inode *inode;
+	const struct iomap_ops *ops;
+	loff_t pos;
+	loff_t len;
+	unsigned flags;
+	struct iomap iomap;
+	struct iomap srcmap;
+};
+
+#define DEFINE_IOMAP_ITER(name, _inode, _pos, _len, _flags, _ops)	\
+	struct iomap_iter name = {					\
+		.inode = _inode,					\
+		.ops = _ops,						\
+		.pos = _pos,						\
+		.len = _len,						\
+		.flags = _flags,					\
+	}
+
+/* Magic value for first call to iterator */
+#define IOMAP_FIRST_CALL	LLONG_MIN
+
+loff_t iomap_iter(struct iomap_iter *, loff_t written);
+
 /*
  * Main iomap iterator function.
  */
-- 
2.25.1

Re: [PATCH 1/2] iomap: Add iomap_iter API

[Christoph Hellwig]

> +loff_t iomap_iter(struct iomap_iter *iter, loff_t written)
> +{
> +	const struct iomap_ops *ops = iter->ops;
> +	struct iomap *iomap = &iter->iomap;
> +	struct iomap *srcmap = &iter->srcmap;

I think it makes sense to only have members in the iter structure
that this function modifies.  That is, just pass inode, ops and flags
as explicit parameters.

OTOH the len argument / return value seems like something that would
seems useful in the iter structure.  That would require renaming the
current len to something like total_len..

> +/* Magic value for first call to iterator */
> +#define IOMAP_FIRST_CALL	LLONG_MIN

Can we find a way to make a a zero initialized field the indicatator
of the first call?  That way we don't need any knowledge of magic
values in the callers.  And also don't need any special initializer
value, but just leave it to the caller to initialize .pos and
.total_len, and be done with it.

Re: [PATCH 1/2] iomap: Add iomap_iter API

[Matthew Wilcox]

On Wed, Apr 01, 2020 at 08:42:48AM -0700, Christoph Hellwig wrote:
> > +loff_t iomap_iter(struct iomap_iter *iter, loff_t written)
> > +{
> > +	const struct iomap_ops *ops = iter->ops;
> > +	struct iomap *iomap = &iter->iomap;
> > +	struct iomap *srcmap = &iter->srcmap;
> 
> I think it makes sense to only have members in the iter structure
> that this function modifies.  That is, just pass inode, ops and flags
> as explicit parameters.

One of the annoying things we do when looking at the disassembly is
spend a lot of instructions shuffling arguments around.  Passing as many
arguments as possible in a struct minimises that.  Ideally we'd pass
the iomap_iter to iomap_begin() and iomap_end().  Agreed passing the
ops there makes no sense, but I'd like to keep inode and flags in
the iomap_iter struct so they don't need to be passed to begin/end
as explicit arguments.

> OTOH the len argument / return value seems like something that would
> seems useful in the iter structure.  That would require renaming the
> current len to something like total_len..

I'm inclined to go with seg_len and op_len.

> > +/* Magic value for first call to iterator */
> > +#define IOMAP_FIRST_CALL	LLONG_MIN
> 
> Can we find a way to make a a zero initialized field the indicatator
> of the first call?  That way we don't need any knowledge of magic
> values in the callers.  And also don't need any special initializer
> value, but just leave it to the caller to initialize .pos and
> .total_len, and be done with it.

Yeah; this was just a quick hack.  I'll do something neater.

Re: [PATCH 1/2] iomap: Add iomap_iter API

[Jan Kara]

On Wed 01-04-20 11:42:45, Matthew Wilcox wrote:
> On Wed, Apr 01, 2020 at 08:42:48AM -0700, Christoph Hellwig wrote:
> > > +loff_t iomap_iter(struct iomap_iter *iter, loff_t written)
> > > +{
> > > +	const struct iomap_ops *ops = iter->ops;
> > > +	struct iomap *iomap = &iter->iomap;
> > > +	struct iomap *srcmap = &iter->srcmap;
> > 
> > I think it makes sense to only have members in the iter structure
> > that this function modifies.  That is, just pass inode, ops and flags
> > as explicit parameters.
> 
> One of the annoying things we do when looking at the disassembly is
> spend a lot of instructions shuffling arguments around.  Passing as many
> arguments as possible in a struct minimises that.

Somewhat late to the game but ... from the conversions of "explicit
arguments to struct of arguments" I've seen (e.g. in xarray) compilers seem
to generate somewhat slower code when arguments are passed in structs. From
the profiling I did it just seems that when arguments are passed directly,
they are in registers which is generally the fastest access you can get.
When you pass arguments in structs, compilers just fetch the value from
stack which is slower even if its cached. And when the argument is not used
frequently or there's something else cache heavy going on, you may have to
go to L2 or L3 which is when you feel the pain... E.g. I've observed some
of the xarray functions which were "logically" identical to their
radix-tree counterparts generate non-negligible amount of cache misses when
reading their arguments from the passed struct.

I don't think iomap is as CPU sensitive as xarray (generally there's much
heavier work that happens in the filesystem) so I'd just strive for code
simplicity here. But I wanted to mention this so that it's clear that
pushing arguments to structs isn't free either.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] iomap: Add iomap_iter API

[Matthew Wilcox]

On Wed, Apr 01, 2020 at 08:42:48AM -0700, Christoph Hellwig wrote:
> OTOH the len argument / return value seems like something that would
> seems useful in the iter structure.  That would require renaming the
> current len to something like total_len..

Ah, I remembered why I didn't do it that way.  For more complicated
users (eg readahead ...), we want to be able to pass an errno here.
Then iomap_iter() will call ops->iomap_end() and return the errno
you passed in, and we can terminate the loop.

Re: [PATCH 1/2] iomap: Add iomap_iter API

[Christoph Hellwig]

On Wed, Apr 01, 2020 at 12:20:38PM -0700, Matthew Wilcox wrote:
> On Wed, Apr 01, 2020 at 08:42:48AM -0700, Christoph Hellwig wrote:
> > OTOH the len argument / return value seems like something that would
> > seems useful in the iter structure.  That would require renaming the
> > current len to something like total_len..
> 
> Ah, I remembered why I didn't do it that way.  For more complicated
> users (eg readahead ...), we want to be able to pass an errno here.
> Then iomap_iter() will call ops->iomap_end() and return the errno
> you passed in, and we can terminate the loop.

Once we have the iter struct we can and should separate the written
counter from the errno, as that is a much cleaner interface.  The prime
reason I avoided that earlier was to keep the number of arguments down,
and even that was a bad reason in retrospective.

[PATCH 2/2] iomap: Convert page_mkwrite to iter API

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/buffered-io.c | 51 ++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 7c84c4c027c4..d91d1d5a0f84 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1051,50 +1051,37 @@ iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
 }
 EXPORT_SYMBOL_GPL(iomap_truncate_page);
 
-static loff_t
-iomap_page_mkwrite_actor(struct inode *inode, loff_t pos, loff_t length,
-		void *data, struct iomap *iomap, struct iomap *srcmap)
-{
-	struct page *page = data;
-	int ret;
-
-	if (iomap->flags & IOMAP_F_BUFFER_HEAD) {
-		ret = __block_write_begin_int(page, pos, length, NULL, iomap);
-		if (ret)
-			return ret;
-		block_commit_write(page, 0, length);
-	} else {
-		WARN_ON_ONCE(!PageUptodate(page));
-		iomap_page_create(inode, page);
-		set_page_dirty(page);
-	}
-
-	return length;
-}
-
 vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vmf->vma->vm_file);
-	unsigned long length;
-	loff_t offset;
-	ssize_t ret;
+	DEFINE_IOMAP_ITER(iter, inode, page_offset(page), 0,
+			IOMAP_WRITE | IOMAP_FAULT, ops);
+	loff_t ret, len = IOMAP_FIRST_CALL;
 
 	lock_page(page);
 	ret = page_mkwrite_check_truncate(page, inode);
 	if (ret < 0)
 		goto out_unlock;
-	length = ret;
+	iter.len = ret;
 
-	offset = page_offset(page);
-	while (length > 0) {
-		ret = iomap_apply(inode, offset, length,
-				IOMAP_WRITE | IOMAP_FAULT, ops, page,
-				iomap_page_mkwrite_actor);
+	for (;;) {
+		ret = iomap_iter(&iter, len);
 		if (unlikely(ret <= 0))
 			goto out_unlock;
-		offset += ret;
-		length -= ret;
+		len = ret;
+
+		if (iter.iomap.flags & IOMAP_F_BUFFER_HEAD) {
+			ret = __block_write_begin_int(page, iter.pos, len,
+					NULL, &iter.iomap);
+			if (ret)
+				goto out_unlock;
+			block_commit_write(page, 0, len);
+		} else {
+			WARN_ON_ONCE(!PageUptodate(page));
+			iomap_page_create(inode, page);
+			set_page_dirty(page);
+		}
 	}
 
 	wait_for_stable_page(page);
-- 
2.25.1