On 12/12/19 7:26 PM, Darrick J. Wong wrote:

On Thu, Dec 12, 2019 at 12:01:33PM -0700, Jens Axboe wrote:

quoted

This adds support for RWF_UNCACHED for file systems using iomap to
perform buffered writes. We use the generic infrastructure for this,
by tracking pages we created and calling write_drop_cached_pages()
to issue writeback and prune those pages.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/iomap/apply.c       | 24 ++++++++++++++++++++++++
 fs/iomap/buffered-io.c | 23 +++++++++++++++++++----
 include/linux/iomap.h  |  5 +++++
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
index e76148db03b8..11b6812f7b37 100644
--- a/fs/iomap/apply.c
+++ b/fs/iomap/apply.c

@@ -92,5 +92,29 @@ iomap_apply(struct iomap_data *data, const struct iomap_ops *ops,
 				     data->flags, &iomap);
 	}
 
+	if (written && (data->flags & IOMAP_UNCACHED)) {

Hmmm... why is a chunk of buffered write(?) code landing in the iomap
apply function?

I'm going to say that Dave suggested it ;-)

The #define for IOMAP_UNCACHED doesn't have a comment, so I don't know
what this is supposed to mean.  Judging from the one place it gets set
in the buffered write function I gather that this is how you implement
the "write through page cache and immediately unmap the page if it
wasn't there before" behavior?

So based on that, I think you want ...

if IOMAP_WRITE && _UNCACHED && !_DIRECT && written > 0:
	flush and invalidate

Looking at the comments, I did think it was just for writes, but it
looks generic. I'll take the blame for that, we should only call into
that sync-and-invalidate code for buffered writes. I'll make that
change.

Since direct writes are never going to create page cache, right?

If they do, it's handled separately.

And in that case, why not put this at the end of iomap_write_actor?

(Sorry if this came up in the earlier discussions, I've been busy this
week and still have a long way to go for catching up...)

It did come up, the idea is to do it for the full range, not per chunk.
Does that help?

quoted

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 30f40145a9e9..30bb248e1d0d 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h

@@ -48,12 +48,16 @@ struct vm_fault;
  *
  * IOMAP_F_BUFFER_HEAD indicates that the file system requires the use of
  * buffer heads for this mapping.
+ *
+ * IOMAP_F_PAGE_CREATE indicates that pages had to be allocated to satisfy
+ * this operation.
  */
 #define IOMAP_F_NEW		0x01
 #define IOMAP_F_DIRTY		0x02
 #define IOMAP_F_SHARED		0x04
 #define IOMAP_F_MERGED		0x08
 #define IOMAP_F_BUFFER_HEAD	0x10
+#define IOMAP_F_PAGE_CREATE	0x20

I think these new flags need an update to the _STRINGS arrays in
fs/iomap/trace.h.

I'll add that.

quoted

 /*
  * Flags set by the core iomap code during operations:

@@ -121,6 +125,7 @@ struct iomap_page_ops {
 #define IOMAP_FAULT		(1 << 3) /* mapping for page fault */
 #define IOMAP_DIRECT		(1 << 4) /* direct I/O */
 #define IOMAP_NOWAIT		(1 << 5) /* do not block */
+#define IOMAP_UNCACHED		(1 << 6)

No comment?

Definitely, I'll add a comment.

Thanks for taking a look! I'll incorporate your suggestions.

-- 
Jens Axboe