[PATCHSET v2 0/15] Uncached buffered IO

[PATCHSET v2 0/15] Uncached buffered IO

[Jens Axboe]

Hi,

5 years ago I posted patches adding support for RWF_UNCACHED, as a way
to do buffered IO that isn't page cache persistent. The approach back
then was to have private pages for IO, and then get rid of them once IO
was done. But that then runs into all the issues that O_DIRECT has, in
terms of synchronizing with the page cache.

So here's a new approach to the same concent, but using the page cache
as synchronization. That makes RWF_UNCACHED less special, in that it's
just page cache IO, except it prunes the ranges once IO is completed.

Why do this, you may ask? The tldr is that device speeds are only
getting faster, while reclaim is not. Doing normal buffered IO can be
very unpredictable, and suck up a lot of resources on the reclaim side.
This leads people to use O_DIRECT as a work-around, which has its own
set of restrictions in terms of size, offset, and length of IO. It's
also inherently synchronous, and now you need async IO as well. While
the latter isn't necessarily a big problem as we have good options
available there, it also should not be a requirement when all you want
to do is read or write some data without caching.

Even on desktop type systems, a normal NVMe device can fill the entire
page cache in seconds. On the big system I used for testing, there's a
lot more RAM, but also a lot more devices. As can be seen in some of the
results in the following patches, you can still fill RAM in seconds even
when there's 1TB of it. Hence this problem isn't solely a "big
hyperscaler system" issue, it's common across the board.

Common for both reads and writes with RWF_UNCACHED is that they use the
page cache for IO. Reads work just like a normal buffered read would,
with the only exception being that the touched ranges will get pruned
after data has been copied. For writes, the ranges will get writeback
kicked off before the syscall returns, and then writeback completion
will prune the range. Hence writes aren't synchronous, and it's easy to
pipeline writes using RWF_UNCACHED.

File systems need to support this. The patches add support for the
generic filemap helpers, and for iomap. Then ext4 and XFS are marked as
supporting it. The amount of code here is really trivial, and the only
reason the fs opt-in is necessary is to have an RWF_UNCACHED IO return
-EOPNOTSUPP just in case the fs doesn't use either the generic paths or
iomap. Adding "support" to other file systems should be trivial, most of
the time just a one-liner adding FOP_UNCACHED to the fop_flags in the
file_operations struct.

Performance results are in patch 7 for reads and patch 10 for writes,
with the tldr being that I see about a 65% improvement in performance
for both, with fully predictable IO times. CPU reduction is substantial
as well, with no kswapd activity at all for reclaim when using uncached
IO.

Using it from applications is trivial - just set RWF_UNCACHED for the
read or write, using pwritev2(2) or preadv2(2). For io_uring, same
thing, just set RWF_UNCACHED in sqe->rw_flags for a buffered read/write
operation. And that's it.

The goal with this patchset was to make it less special than before. I
think if you look at the diffstat you'll agree that this is the case.

Patches 1..6 are just prep patches, and should have no functional
changes at all. Patch 7 adds support for the filemap path for
RWF_UNCACHED reads, patch 10 adds support for filemap RWF_UNCACHED
writes, and patches 12..15 adds ext4 and xfs/iomap support iomap.

Git tree available here:

https://git.kernel.dk/cgit/linux/log/?h=buffered-uncached.5

 fs/ext4/ext4.h                 |  1 +
 fs/ext4/file.c                 |  2 +-
 fs/ext4/inline.c               |  7 ++-
 fs/ext4/inode.c                | 18 ++++++-
 fs/ext4/page-io.c              | 28 ++++++-----
 fs/iomap/buffered-io.c         | 15 +++++-
 fs/xfs/xfs_aops.c              |  7 ++-
 fs/xfs/xfs_file.c              |  4 +-
 include/linux/fs.h             | 10 +++-
 include/linux/iomap.h          |  4 +-
 include/linux/page-flags.h     |  5 ++
 include/linux/pagemap.h        | 34 +++++++++++++
 include/trace/events/mmflags.h |  3 +-
 include/uapi/linux/fs.h        |  6 ++-
 mm/filemap.c                   | 90 ++++++++++++++++++++++++++++------
 mm/readahead.c                 | 22 +++++++--
 mm/swap.c                      |  2 +
 mm/truncate.c                  |  9 ++--
 18 files changed, 218 insertions(+), 49 deletions(-)

Since v1
- Move iocb->ki_flags checking into filemap_create_folio()
- Use __folio_set_uncached() when marking a folio as uncached right
  after creation
- Shuffle patches around to not have a bisection issue
- Combine some patches
- Add FGP_UNCACHED folio get flag. If set, newly created folios will
  get marked as uncached.
- Add foliop_uncached to be able to pass whether this is an uncached
  folio creation or not into ->write_begin(). Fixes the issue of
  uncached writes pruning existing ranges. Not the prettiest, but the
  prospect of changing ->write_begin() is daunting. I did go that
  route but it's a lot of churn. Have the patch and the scars.
- Ensure that both ext4 and xfs punt to their workqueue handling
  writeback completion for uncached writebacks, as we need workqueue
  context to prune ranges.
- Add generic_uncached_write() helper to make it easy for file systems
  to prune the ranges.
- Add folio_end_uncached() writeback completion helper, and also check
  in_task() in there. Can trigger if the fs needs hints on punting to
  a safe context for this completion, but normal writeback races with
  us and starts writeback on a range that includes uncached folios.
- Rebase on current master

-- 
Jens Axboe

[PATCH 01/15] mm/filemap: change filemap_create_folio() to take a struct kiocb

[Jens Axboe]

Rather than pass in both the file and position directly from the kiocb,
just take a struct kiocb instead. While doing so, move the ki_flags
checking into filemap_create_folio() as well. In preparation for actually
needing the kiocb in the function.

No functional changes in this patch.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 mm/filemap.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 36d22968be9a..0b187938b999 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2460,15 +2460,17 @@ static int filemap_update_page(struct kiocb *iocb,
 	return error;
 }
 
-static int filemap_create_folio(struct file *file,
-		struct address_space *mapping, loff_t pos,
-		struct folio_batch *fbatch)
+static int filemap_create_folio(struct kiocb *iocb,
+		struct address_space *mapping, struct folio_batch *fbatch)
 {
 	struct folio *folio;
 	int error;
 	unsigned int min_order = mapping_min_folio_order(mapping);
 	pgoff_t index;
 
+	if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
+		return -EAGAIN;
+
 	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), min_order);
 	if (!folio)
 		return -ENOMEM;
@@ -2487,7 +2489,7 @@ static int filemap_create_folio(struct file *file,
 	 * well to keep locking rules simple.
 	 */
 	filemap_invalidate_lock_shared(mapping);
-	index = (pos >> (PAGE_SHIFT + min_order)) << min_order;
+	index = (iocb->ki_pos >> (PAGE_SHIFT + min_order)) << min_order;
 	error = filemap_add_folio(mapping, folio, index,
 			mapping_gfp_constraint(mapping, GFP_KERNEL));
 	if (error == -EEXIST)
@@ -2495,7 +2497,8 @@ static int filemap_create_folio(struct file *file,
 	if (error)
 		goto error;
 
-	error = filemap_read_folio(file, mapping->a_ops->read_folio, folio);
+	error = filemap_read_folio(iocb->ki_filp, mapping->a_ops->read_folio,
+					folio);
 	if (error)
 		goto error;
 
@@ -2551,9 +2554,7 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 		filemap_get_read_batch(mapping, index, last_index - 1, fbatch);
 	}
 	if (!folio_batch_count(fbatch)) {
-		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
-			return -EAGAIN;
-		err = filemap_create_folio(filp, mapping, iocb->ki_pos, fbatch);
+		err = filemap_create_folio(iocb, mapping, fbatch);
 		if (err == AOP_TRUNCATED_PAGE)
 			goto retry;
 		return err;
-- 
2.45.2

[PATCH 02/15] mm/readahead: add folio allocation helper

[Jens Axboe]

Just a wrapper around filemap_alloc_folio() for now, but add it in
preparation for modifying the folio based on the 'ractl' being passed
in.

No functional changes in this patch.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 mm/readahead.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 3dc6c7a128dd..003cfe79880d 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -188,6 +188,12 @@ static void read_pages(struct readahead_control *rac)
 	BUG_ON(readahead_count(rac));
 }
 
+static struct folio *ractl_alloc_folio(struct readahead_control *ractl,
+				       gfp_t gfp_mask, unsigned int order)
+{
+	return filemap_alloc_folio(gfp_mask, order);
+}
+
 /**
  * page_cache_ra_unbounded - Start unchecked readahead.
  * @ractl: Readahead control.
@@ -260,8 +266,8 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			continue;
 		}
 
-		folio = filemap_alloc_folio(gfp_mask,
-					    mapping_min_folio_order(mapping));
+		folio = ractl_alloc_folio(ractl, gfp_mask,
+					mapping_min_folio_order(mapping));
 		if (!folio)
 			break;
 
@@ -431,7 +437,7 @@ static inline int ra_alloc_folio(struct readahead_control *ractl, pgoff_t index,
 		pgoff_t mark, unsigned int order, gfp_t gfp)
 {
 	int err;
-	struct folio *folio = filemap_alloc_folio(gfp, order);
+	struct folio *folio = ractl_alloc_folio(ractl, gfp, order);
 
 	if (!folio)
 		return -ENOMEM;
@@ -753,7 +759,7 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, min_order);
+		folio = ractl_alloc_folio(ractl, gfp_mask, min_order);
 		if (!folio)
 			return;
 
@@ -782,7 +788,7 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, min_order);
+		folio = ractl_alloc_folio(ractl, gfp_mask, min_order);
 		if (!folio)
 			return;
 
-- 
2.45.2

[PATCH 03/15] mm: add PG_uncached page flag

[Jens Axboe]

Add a page flag that file IO can use to indicate that the IO being done
is uncached, as in it should not persist in the page cache after the IO
has been completed.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/page-flags.h     | 5 +++++
 include/trace/events/mmflags.h | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index cc839e4365c1..3c4003495929 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -110,6 +110,7 @@ enum pageflags {
 	PG_reclaim,		/* To be reclaimed asap */
 	PG_swapbacked,		/* Page is backed by RAM/swap */
 	PG_unevictable,		/* Page is "unevictable"  */
+	PG_uncached,		/* uncached read/write IO */
 #ifdef CONFIG_MMU
 	PG_mlocked,		/* Page is vma mlocked */
 #endif
@@ -562,6 +563,10 @@ PAGEFLAG(Reclaim, reclaim, PF_NO_TAIL)
 FOLIO_FLAG(readahead, FOLIO_HEAD_PAGE)
 	FOLIO_TEST_CLEAR_FLAG(readahead, FOLIO_HEAD_PAGE)
 
+FOLIO_FLAG(uncached, FOLIO_HEAD_PAGE)
+	FOLIO_TEST_CLEAR_FLAG(uncached, FOLIO_HEAD_PAGE)
+	__FOLIO_SET_FLAG(uncached, FOLIO_HEAD_PAGE)
+
 #ifdef CONFIG_HIGHMEM
 /*
  * Must use a macro here due to header dependency issues. page_zone() is not
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index bb8a59c6caa2..b60057284102 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -116,7 +116,8 @@
 	DEF_PAGEFLAG_NAME(head),					\
 	DEF_PAGEFLAG_NAME(reclaim),					\
 	DEF_PAGEFLAG_NAME(swapbacked),					\
-	DEF_PAGEFLAG_NAME(unevictable)					\
+	DEF_PAGEFLAG_NAME(unevictable),					\
+	DEF_PAGEFLAG_NAME(uncached)					\
 IF_HAVE_PG_MLOCK(mlocked)						\
 IF_HAVE_PG_HWPOISON(hwpoison)						\
 IF_HAVE_PG_IDLE(idle)							\
-- 
2.45.2

[PATCH 04/15] mm/readahead: add readahead_control->uncached member

[Jens Axboe]

If ractl->uncached is set to true, then folios created are marked as
uncached as well.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/pagemap.h | 1 +
 mm/readahead.c          | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 68a5f1ff3301..8afacb7520d4 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -1350,6 +1350,7 @@ struct readahead_control {
 	pgoff_t _index;
 	unsigned int _nr_pages;
 	unsigned int _batch_count;
+	bool uncached;
 	bool _workingset;
 	unsigned long _pflags;
 };
diff --git a/mm/readahead.c b/mm/readahead.c
index 003cfe79880d..8dbeab9bc1f0 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -191,7 +191,13 @@ static void read_pages(struct readahead_control *rac)
 static struct folio *ractl_alloc_folio(struct readahead_control *ractl,
 				       gfp_t gfp_mask, unsigned int order)
 {
-	return filemap_alloc_folio(gfp_mask, order);
+	struct folio *folio;
+
+	folio = filemap_alloc_folio(gfp_mask, order);
+	if (folio && ractl->uncached)
+		__folio_set_uncached(folio);
+
+	return folio;
 }
 
 /**
-- 
2.45.2

[PATCH 05/15] mm/filemap: use page_cache_sync_ra() to kick off read-ahead

[Jens Axboe]

Rather than use the page_cache_sync_readahead() helper, define our own
ractl and use page_cache_sync_ra() directly. In preparation for needing
to modify ractl inside filemap_get_pages().

No functional changes in this patch.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 mm/filemap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 0b187938b999..38dc94b761b7 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2528,7 +2528,6 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 {
 	struct file *filp = iocb->ki_filp;
 	struct address_space *mapping = filp->f_mapping;
-	struct file_ra_state *ra = &filp->f_ra;
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
 	pgoff_t last_index;
 	struct folio *folio;
@@ -2543,12 +2542,13 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 
 	filemap_get_read_batch(mapping, index, last_index - 1, fbatch);
 	if (!folio_batch_count(fbatch)) {
+		DEFINE_READAHEAD(ractl, filp, &filp->f_ra, mapping, index);
+
 		if (iocb->ki_flags & IOCB_NOIO)
 			return -EAGAIN;
 		if (iocb->ki_flags & IOCB_NOWAIT)
 			flags = memalloc_noio_save();
-		page_cache_sync_readahead(mapping, ra, filp, index,
-				last_index - index);
+		page_cache_sync_ra(&ractl, last_index - index);
 		if (iocb->ki_flags & IOCB_NOWAIT)
 			memalloc_noio_restore(flags);
 		filemap_get_read_batch(mapping, index, last_index - 1, fbatch);
-- 
2.45.2

[PATCH 06/15] mm/truncate: make invalidate_complete_folio2() public

[Jens Axboe]

Make invalidate_complete_folio2() be publicly available, and have it
take a gfp_t mask as well rather than hardcode GFP_KERNEL. The only
caller just passes in GFP_KERNEL, no functional changes in this patch.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/pagemap.h | 2 ++
 mm/truncate.c           | 9 +++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 8afacb7520d4..0122b3fbe2ac 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -34,6 +34,8 @@ int kiocb_invalidate_pages(struct kiocb *iocb, size_t count);
 void kiocb_invalidate_post_direct_write(struct kiocb *iocb, size_t count);
 int filemap_invalidate_pages(struct address_space *mapping,
 			     loff_t pos, loff_t end, bool nowait);
+int invalidate_complete_folio2(struct address_space *mapping,
+				struct folio *folio, gfp_t gfp_mask);
 
 int write_inode_now(struct inode *, int sync);
 int filemap_fdatawrite(struct address_space *);
diff --git a/mm/truncate.c b/mm/truncate.c
index 0668cd340a46..e084f7aa9370 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -546,13 +546,13 @@ EXPORT_SYMBOL(invalidate_mapping_pages);
  * shrink_folio_list() has a temp ref on them, or because they're transiently
  * sitting in the folio_add_lru() caches.
  */
-static int invalidate_complete_folio2(struct address_space *mapping,
-					struct folio *folio)
+int invalidate_complete_folio2(struct address_space *mapping,
+				struct folio *folio, gfp_t gfp_mask)
 {
 	if (folio->mapping != mapping)
 		return 0;
 
-	if (!filemap_release_folio(folio, GFP_KERNEL))
+	if (!filemap_release_folio(folio, gfp_mask))
 		return 0;
 
 	spin_lock(&mapping->host->i_lock);
@@ -650,7 +650,8 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 
 			ret2 = folio_launder(mapping, folio);
 			if (ret2 == 0) {
-				if (!invalidate_complete_folio2(mapping, folio))
+				if (!invalidate_complete_folio2(mapping, folio,
+								GFP_KERNEL))
 					ret2 = -EBUSY;
 			}
 			if (ret2 < 0)
-- 
2.45.2

[PATCH 07/15] fs: add RWF_UNCACHED iocb and FOP_UNCACHED file_operations flag

[Jens Axboe]

If a file system supports uncached buffered IO, it may set FOP_UNCACHED
and enable RWF_UNCACHED. If RWF_UNCACHED is attempted without the file
system supporting it, it'll get errored with -EOPNOTSUPP.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/fs.h      | 10 +++++++++-
 include/uapi/linux/fs.h |  6 +++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3559446279c1..5abc53991cd0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -320,6 +320,7 @@ struct readahead_control;
 #define IOCB_NOWAIT		(__force int) RWF_NOWAIT
 #define IOCB_APPEND		(__force int) RWF_APPEND
 #define IOCB_ATOMIC		(__force int) RWF_ATOMIC
+#define IOCB_UNCACHED		(__force int) RWF_UNCACHED
 
 /* non-RWF related bits - start at 16 */
 #define IOCB_EVENTFD		(1 << 16)
@@ -354,7 +355,8 @@ struct readahead_control;
 	{ IOCB_SYNC,		"SYNC" }, \
 	{ IOCB_NOWAIT,		"NOWAIT" }, \
 	{ IOCB_APPEND,		"APPEND" }, \
-	{ IOCB_ATOMIC,		"ATOMIC"}, \
+	{ IOCB_ATOMIC,		"ATOMIC" }, \
+	{ IOCB_UNCACHED,	"UNCACHED" }, \
 	{ IOCB_EVENTFD,		"EVENTFD"}, \
 	{ IOCB_DIRECT,		"DIRECT" }, \
 	{ IOCB_WRITE,		"WRITE" }, \
@@ -2116,6 +2118,8 @@ struct file_operations {
 #define FOP_HUGE_PAGES		((__force fop_flags_t)(1 << 4))
 /* Treat loff_t as unsigned (e.g., /dev/mem) */
 #define FOP_UNSIGNED_OFFSET	((__force fop_flags_t)(1 << 5))
+/* File system supports uncached read/write buffered IO */
+#define FOP_UNCACHED		((__force fop_flags_t)(1 << 6))
 
 /* Wrap a directory iterator that needs exclusive inode access */
 int wrap_directory_iterator(struct file *, struct dir_context *,
@@ -3532,6 +3536,10 @@ static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags,
 		if (!(ki->ki_filp->f_mode & FMODE_CAN_ATOMIC_WRITE))
 			return -EOPNOTSUPP;
 	}
+	if (flags & RWF_UNCACHED) {
+		if (!(ki->ki_filp->f_op->fop_flags & FOP_UNCACHED))
+			return -EOPNOTSUPP;
+	}
 	kiocb_flags |= (__force int) (flags & RWF_SUPPORTED);
 	if (flags & RWF_SYNC)
 		kiocb_flags |= IOCB_DSYNC;
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 753971770733..dc77cd8ae1a3 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -332,9 +332,13 @@ typedef int __bitwise __kernel_rwf_t;
 /* Atomic Write */
 #define RWF_ATOMIC	((__force __kernel_rwf_t)0x00000040)
 
+/* buffered IO that drops the cache after reading or writing data */
+#define RWF_UNCACHED	((__force __kernel_rwf_t)0x00000080)
+
 /* mask of flags supported by the kernel */
 #define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\
-			 RWF_APPEND | RWF_NOAPPEND | RWF_ATOMIC)
+			 RWF_APPEND | RWF_NOAPPEND | RWF_ATOMIC |\
+			 RWF_UNCACHED)
 
 #define PROCFS_IOCTL_MAGIC 'f'
 
-- 
2.45.2

[PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

Add RWF_UNCACHED as a read operation flag, which means that any data
read wil be removed from the page cache upon completion. Uses the page
cache to synchronize, and simply prunes folios that were instantiated
when the operation completes. While it would be possible to use private
pages for this, using the page cache as synchronization is handy for a
variety of reasons:

1) No special truncate magic is needed
2) Async buffered reads need some place to serialize, using the page
   cache is a lot easier than writing extra code for this
3) The pruning cost is pretty reasonable

and the code to support this is much simpler as a result.

You can think of uncached buffered IO as being the much more attractive
cousing of O_DIRECT - it has none of the restrictions of O_DIRECT. Yes,
it will copy the data, but unlike regular buffered IO, it doesn't run
into the unpredictability of the page cache in terms of reclaim. As an
example, on a test box with 32 drives, reading them with buffered IO
looks as follows:

Reading bs 65536, uncached 0
  1s: 145945MB/sec
  2s: 158067MB/sec
  3s: 157007MB/sec
  4s: 148622MB/sec
  5s: 118824MB/sec
  6s: 70494MB/sec
  7s: 41754MB/sec
  8s: 90811MB/sec
  9s: 92204MB/sec
 10s: 95178MB/sec
 11s: 95488MB/sec
 12s: 95552MB/sec
 13s: 96275MB/sec

where it's quite easy to see where the page cache filled up, and
performance went from good to erratic, and finally settles at a much
lower rate. Looking at top while this is ongoing, we see:

 PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
7535 root      20   0  267004      0      0 S  3199   0.0   8:40.65 uncached
3326 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd4
3327 root      20   0       0      0      0 R 100.0   0.0   0:17.22 kswapd5
3328 root      20   0       0      0      0 R 100.0   0.0   0:13.29 kswapd6
3332 root      20   0       0      0      0 R 100.0   0.0   0:11.11 kswapd10
3339 root      20   0       0      0      0 R 100.0   0.0   0:16.25 kswapd17
3348 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd26
3343 root      20   0       0      0      0 R 100.0   0.0   0:16.30 kswapd21
3344 root      20   0       0      0      0 R 100.0   0.0   0:11.92 kswapd22
3349 root      20   0       0      0      0 R 100.0   0.0   0:16.28 kswapd27
3352 root      20   0       0      0      0 R  99.7   0.0   0:11.89 kswapd30
3353 root      20   0       0      0      0 R  96.7   0.0   0:16.04 kswapd31
3329 root      20   0       0      0      0 R  96.4   0.0   0:11.41 kswapd7
3345 root      20   0       0      0      0 R  96.4   0.0   0:13.40 kswapd23
3330 root      20   0       0      0      0 S  91.1   0.0   0:08.28 kswapd8
3350 root      20   0       0      0      0 S  86.8   0.0   0:11.13 kswapd28
3325 root      20   0       0      0      0 S  76.3   0.0   0:07.43 kswapd3
3341 root      20   0       0      0      0 S  74.7   0.0   0:08.85 kswapd19
3334 root      20   0       0      0      0 S  71.7   0.0   0:10.04 kswapd12
3351 root      20   0       0      0      0 R  60.5   0.0   0:09.59 kswapd29
3323 root      20   0       0      0      0 R  57.6   0.0   0:11.50 kswapd1
[...]

which is just showing a partial list of the 32 kswapd threads that are
running mostly full tilt, burning ~28 full CPU cores.

If the same test case is run with RWF_UNCACHED set for the buffered read,
the output looks as follows:

Reading bs 65536, uncached 0
  1s: 153144MB/sec
  2s: 156760MB/sec
  3s: 158110MB/sec
  4s: 158009MB/sec
  5s: 158043MB/sec
  6s: 157638MB/sec
  7s: 157999MB/sec
  8s: 158024MB/sec
  9s: 157764MB/sec
 10s: 157477MB/sec
 11s: 157417MB/sec
 12s: 157455MB/sec
 13s: 157233MB/sec
 14s: 156692MB/sec

which is just chugging along at ~155GB/sec of read performance. Looking
at top, we see:

 PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
7961 root      20   0  267004      0      0 S  3180   0.0   5:37.95 uncached
8024 axboe     20   0   14292   4096      0 R   1.0   0.0   0:00.13 top

where just the test app is using CPU, no reclaim is taking place outside
of the main thread. Not only is performance 65% better, it's also using
half the CPU to do it.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 mm/filemap.c | 18 ++++++++++++++++--
 mm/swap.c    |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 38dc94b761b7..bd698340ef24 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2474,6 +2474,8 @@ static int filemap_create_folio(struct kiocb *iocb,
 	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), min_order);
 	if (!folio)
 		return -ENOMEM;
+	if (iocb->ki_flags & IOCB_UNCACHED)
+		__folio_set_uncached(folio);
 
 	/*
 	 * Protect against truncate / hole punch. Grabbing invalidate_lock
@@ -2519,6 +2521,8 @@ static int filemap_readahead(struct kiocb *iocb, struct file *file,
 
 	if (iocb->ki_flags & IOCB_NOIO)
 		return -EAGAIN;
+	if (iocb->ki_flags & IOCB_UNCACHED)
+		ractl.uncached = 1;
 	page_cache_async_ra(&ractl, folio, last_index - folio->index);
 	return 0;
 }
@@ -2548,6 +2552,8 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 			return -EAGAIN;
 		if (iocb->ki_flags & IOCB_NOWAIT)
 			flags = memalloc_noio_save();
+		if (iocb->ki_flags & IOCB_UNCACHED)
+			ractl.uncached = 1;
 		page_cache_sync_ra(&ractl, last_index - index);
 		if (iocb->ki_flags & IOCB_NOWAIT)
 			memalloc_noio_restore(flags);
@@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 			}
 		}
 put_folios:
-		for (i = 0; i < folio_batch_count(&fbatch); i++)
-			folio_put(fbatch.folios[i]);
+		for (i = 0; i < folio_batch_count(&fbatch); i++) {
+			struct folio *folio = fbatch.folios[i];
+
+			if (folio_test_uncached(folio)) {
+				folio_lock(folio);
+				invalidate_complete_folio2(mapping, folio, 0);
+				folio_unlock(folio);
+			}
+			folio_put(folio);
+		}
 		folio_batch_init(&fbatch);
 	} while (iov_iter_count(iter) && iocb->ki_pos < isize && !error);
 
diff --git a/mm/swap.c b/mm/swap.c
index 835bdf324b76..f2457acae383 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
  */
 void folio_mark_accessed(struct folio *folio)
 {
+	if (folio_test_uncached(folio))
+		return;
 	if (lru_gen_enabled()) {
 		folio_inc_refs(folio);
 		return;
-- 
2.45.2

[PATCH 09/15] mm/filemap: drop uncached pages when writeback completes

[Jens Axboe]

If the folio is marked as uncached, drop pages when writeback completes.
Intended to be used with RWF_UNCACHED, to avoid needing sync writes for
uncached IO.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 mm/filemap.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index bd698340ef24..efd02b047541 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1600,6 +1600,23 @@ int folio_wait_private_2_killable(struct folio *folio)
 }
 EXPORT_SYMBOL(folio_wait_private_2_killable);
 
+/*
+ * If folio was marked as uncached, then pages should be dropped when writeback
+ * completes. Do that now. If we fail, it's likely because of a big folio -
+ * just reset uncached for that case and latter completions should invalidate.
+ */
+static void folio_end_uncached(struct folio *folio)
+{
+	bool reset = true;
+
+	if (folio_trylock(folio)) {
+		reset = !invalidate_complete_folio2(folio->mapping, folio, 0);
+		folio_unlock(folio);
+	}
+	if (reset)
+		folio_set_uncached(folio);
+}
+
 /**
  * folio_end_writeback - End writeback against a folio.
  * @folio: The folio.
@@ -1610,6 +1627,8 @@ EXPORT_SYMBOL(folio_wait_private_2_killable);
  */
 void folio_end_writeback(struct folio *folio)
 {
+	bool folio_uncached;
+
 	VM_BUG_ON_FOLIO(!folio_test_writeback(folio), folio);
 
 	/*
@@ -1631,9 +1650,13 @@ void folio_end_writeback(struct folio *folio)
 	 * reused before the folio_wake_bit().
 	 */
 	folio_get(folio);
+	folio_uncached = folio_test_clear_uncached(folio);
 	if (__folio_end_writeback(folio))
 		folio_wake_bit(folio, PG_writeback);
 	acct_reclaim_writeback(folio);
+
+	if (folio_uncached)
+		folio_end_uncached(folio);
 	folio_put(folio);
 }
 EXPORT_SYMBOL(folio_end_writeback);
-- 
2.45.2

[PATCH 10/15] mm/filemap: make buffered writes work with RWF_UNCACHED

[Jens Axboe]

If RWF_UNCACHED is set for a write, mark new folios being written with
uncached. This is done by passing in the fact that it's an uncached write
through the folio pointer. We can only get there when IOCB_UNCACHED was
allowed, which can only happen if the file system opts in. Opting in means
they need to check for the LSB in the folio pointer to know if it's an
uncached write or not. If it is, then FGP_UNCACHED should be used if
creating new folios is necessary.

Uncached writes will drop any folios they create upon writeback
completion, but leave folios that may exist in that range alone. Since
->write_begin() doesn't currently take any flags, and to avoid needing
to change the callback kernel wide, use the foliop being passed in to
->write_begin() to signal if this is an uncached write or not. File
systems can then use that to mark newly created folios as uncached.

Add a helper, generic_uncached_write(), that generic_file_write_iter()
calls upon successful completion of an uncached write.

This provides similar benefits to using RWF_UNCACHED with reads. Testing
buffered writes on 32 files:

writing bs 65536, uncached 0
  1s: 196035MB/sec, MB=196035
  2s: 132308MB/sec, MB=328147
  3s: 132438MB/sec, MB=460586
  4s: 116528MB/sec, MB=577115
  5s: 103898MB/sec, MB=681014
  6s: 108893MB/sec, MB=789907
  7s: 99678MB/sec, MB=889586
  8s: 106545MB/sec, MB=996132
  9s: 106826MB/sec, MB=1102958
 10s: 101544MB/sec, MB=1204503
 11s: 111044MB/sec, MB=1315548
 12s: 124257MB/sec, MB=1441121
 13s: 116031MB/sec, MB=1557153
 14s: 114540MB/sec, MB=1671694
 15s: 115011MB/sec, MB=1786705
 16s: 115260MB/sec, MB=1901966
 17s: 116068MB/sec, MB=2018034
 18s: 116096MB/sec, MB=2134131

where it's quite obvious where the page cache filled, and performance
dropped from to about half of where it started, settling in at around
115GB/sec. Meanwhile, 32 kswapds were running full steam trying to
reclaim pages.

Running the same test with uncached buffered writes:

writing bs 65536, uncached 1
  1s: 198974MB/sec
  2s: 189618MB/sec
  3s: 193601MB/sec
  4s: 188582MB/sec
  5s: 193487MB/sec
  6s: 188341MB/sec
  7s: 194325MB/sec
  8s: 188114MB/sec
  9s: 192740MB/sec
 10s: 189206MB/sec
 11s: 193442MB/sec
 12s: 189659MB/sec
 13s: 191732MB/sec
 14s: 190701MB/sec
 15s: 191789MB/sec
 16s: 191259MB/sec
 17s: 190613MB/sec
 18s: 191951MB/sec

and the behavior is fully predictable, performing the same throughout
even after the page cache would otherwise have fully filled with dirty
data. It's also about 65% faster, and using half the CPU of the system
compared to the normal buffered write.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/pagemap.h | 29 +++++++++++++++++++++++++++++
 mm/filemap.c            | 26 +++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 0122b3fbe2ac..5469664f66c3 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -14,6 +14,7 @@
 #include <linux/gfp.h>
 #include <linux/bitops.h>
 #include <linux/hardirq.h> /* for in_interrupt() */
+#include <linux/writeback.h>
 #include <linux/hugetlb_inline.h>
 
 struct folio_batch;
@@ -70,6 +71,34 @@ static inline int filemap_write_and_wait(struct address_space *mapping)
 	return filemap_write_and_wait_range(mapping, 0, LLONG_MAX);
 }
 
+/*
+ * generic_uncached_write - start uncached writeback
+ * @iocb: the iocb that was written
+ * @written: the amount of bytes written
+ *
+ * When writeback has been handled by write_iter, this helper should be called
+ * if the file system supports uncached writes. If %IOCB_UNCACHED is set, it
+ * will kick off writeback for the specified range.
+ */
+static inline void generic_uncached_write(struct kiocb *iocb, ssize_t written)
+{
+	if (iocb->ki_flags & IOCB_UNCACHED) {
+		struct address_space *mapping = iocb->ki_filp->f_mapping;
+
+		/* kick off uncached writeback */
+		__filemap_fdatawrite_range(mapping, iocb->ki_pos,
+					   iocb->ki_pos + written, WB_SYNC_NONE);
+	}
+}
+
+/*
+ * Value passed in to ->write_begin() if IOCB_UNCACHED is set for the write,
+ * and the ->write_begin() handler on a file system supporting FOP_UNCACHED
+ * must check for this and pass FGP_UNCACHED for folio creation.
+ */
+#define foliop_uncached			((struct folio *) 0xfee1c001)
+#define foliop_is_uncached(foliop)	(*(foliop) == foliop_uncached)
+
 /**
  * filemap_set_wb_err - set a writeback error on an address_space
  * @mapping: mapping in which to set writeback error
diff --git a/mm/filemap.c b/mm/filemap.c
index efd02b047541..cfbfc8b14b1f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -430,6 +430,7 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start,
 
 	return filemap_fdatawrite_wbc(mapping, &wbc);
 }
+EXPORT_SYMBOL_GPL(__filemap_fdatawrite_range);
 
 static inline int __filemap_fdatawrite(struct address_space *mapping,
 	int sync_mode)
@@ -1609,7 +1610,14 @@ static void folio_end_uncached(struct folio *folio)
 {
 	bool reset = true;
 
-	if (folio_trylock(folio)) {
+	/*
+	 * Hitting !in_task() should not happen off RWF_UNCACHED writeback, but
+	 * can happen if normal writeback just happens to find dirty folios
+	 * that were created as part of uncached writeback, and that writeback
+	 * would otherwise not need non-IRQ handling. Just skip the
+	 * invalidation in that case.
+	 */
+	if (in_task() && folio_trylock(folio)) {
 		reset = !invalidate_complete_folio2(folio->mapping, folio, 0);
 		folio_unlock(folio);
 	}
@@ -4061,7 +4069,7 @@ ssize_t generic_perform_write(struct kiocb *iocb, struct iov_iter *i)
 	ssize_t written = 0;
 
 	do {
-		struct folio *folio;
+		struct folio *folio = NULL;
 		size_t offset;		/* Offset into folio */
 		size_t bytes;		/* Bytes to write to folio */
 		size_t copied;		/* Bytes copied from user */
@@ -4089,6 +4097,16 @@ ssize_t generic_perform_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
+		/*
+		 * If IOCB_UNCACHED is set here, we now the file system
+		 * supports it. And hence it'll know to check folip for being
+		 * set to this magic value. If so, it's an uncached write.
+		 * Whenever ->write_begin() changes prototypes again, this
+		 * can go away and just pass iocb or iocb flags.
+		 */
+		if (iocb->ki_flags & IOCB_UNCACHED)
+			folio = foliop_uncached;
+
 		status = a_ops->write_begin(file, mapping, pos, bytes,
 						&folio, &fsdata);
 		if (unlikely(status < 0))
@@ -4219,8 +4237,10 @@ ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		ret = __generic_file_write_iter(iocb, from);
 	inode_unlock(inode);
 
-	if (ret > 0)
+	if (ret > 0) {
+		generic_uncached_write(iocb, ret);
 		ret = generic_write_sync(iocb, ret);
+	}
 	return ret;
 }
 EXPORT_SYMBOL(generic_file_write_iter);
-- 
2.45.2

[PATCH 11/15] mm: add FGP_UNCACHED folio creation flag

[Jens Axboe]

Callers can pass this in for uncached folio creation, in which case if
a folio is newly created it gets marked as uncached. If a folio exists
for this index and lookup succeeds, then it will not get marked as
uncached.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/pagemap.h | 2 ++
 mm/filemap.c            | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 5469664f66c3..de0ed906cd2d 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -741,6 +741,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping,
  * * %FGP_NOFS - __GFP_FS will get cleared in gfp.
  * * %FGP_NOWAIT - Don't block on the folio lock.
  * * %FGP_STABLE - Wait for the folio to be stable (finished writeback)
+ * * %FGP_UNCACHED - Uncached buffered IO
  * * %FGP_WRITEBEGIN - The flags to use in a filesystem write_begin()
  *   implementation.
  */
@@ -754,6 +755,7 @@ typedef unsigned int __bitwise fgf_t;
 #define FGP_NOWAIT		((__force fgf_t)0x00000020)
 #define FGP_FOR_MMAP		((__force fgf_t)0x00000040)
 #define FGP_STABLE		((__force fgf_t)0x00000080)
+#define FGP_UNCACHED		((__force fgf_t)0x00000100)
 #define FGF_GET_ORDER(fgf)	(((__force unsigned)fgf) >> 26)	/* top 6 bits */
 
 #define FGP_WRITEBEGIN		(FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE)
diff --git a/mm/filemap.c b/mm/filemap.c
index cfbfc8b14b1f..4fdf3c4ae00f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1987,6 +1987,8 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 			/* Init accessed so avoid atomic mark_page_accessed later */
 			if (fgp_flags & FGP_ACCESSED)
 				__folio_set_referenced(folio);
+			if (fgp_flags & FGP_UNCACHED)
+				__folio_set_uncached(folio);
 
 			err = filemap_add_folio(mapping, folio, index, gfp);
 			if (!err)
-- 
2.45.2

[PATCH 12/15] ext4: add RWF_UNCACHED write support

[Jens Axboe]

IOCB_UNCACHED IO needs to prune writeback regions on IO completion,
and hence need the worker punt that ext4 also does for unwritten
extents. Add an io_end flag to manage that.

If foliop is set to foliop_uncached in ext4_write_begin(), then set
FGP_UNCACHED so that __filemap_get_folio() will mark newly created
folios as uncached. That in turn will make writeback completion drop
these ranges from the page cache.

Now that ext4 supports both uncached reads and writes, add the fop_flag
FOP_UNCACHED to enable it.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/ext4/ext4.h    |  1 +
 fs/ext4/file.c    |  2 +-
 fs/ext4/inline.c  |  7 ++++++-
 fs/ext4/inode.c   | 18 ++++++++++++++++--
 fs/ext4/page-io.c | 28 ++++++++++++++++------------
 5 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 44b0d418143c..60dc9ffae076 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -279,6 +279,7 @@ struct ext4_system_blocks {
  * Flags for ext4_io_end->flags
  */
 #define	EXT4_IO_END_UNWRITTEN	0x0001
+#define EXT4_IO_UNCACHED	0x0002
 
 struct ext4_io_end_vec {
 	struct list_head list;		/* list of io_end_vec */
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index f14aed14b9cf..0ef39d738598 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -944,7 +944,7 @@ const struct file_operations ext4_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ext4_fallocate,
 	.fop_flags	= FOP_MMAP_SYNC | FOP_BUFFER_RASYNC |
-			  FOP_DIO_PARALLEL_WRITE,
+			  FOP_DIO_PARALLEL_WRITE | FOP_UNCACHED,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 3536ca7e4fcc..4089d0744164 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -667,6 +667,7 @@ int ext4_try_to_write_inline_data(struct address_space *mapping,
 	handle_t *handle;
 	struct folio *folio;
 	struct ext4_iloc iloc;
+	fgf_t fgp_flags;
 
 	if (pos + len > ext4_get_max_inline_size(inode))
 		goto convert;
@@ -702,7 +703,11 @@ int ext4_try_to_write_inline_data(struct address_space *mapping,
 	if (ret)
 		goto out;
 
-	folio = __filemap_get_folio(mapping, 0, FGP_WRITEBEGIN | FGP_NOFS,
+	fgp_flags = FGP_WRITEBEGIN | FGP_NOFS;
+	if (*foliop == foliop_uncached)
+		fgp_flags |= FGP_UNCACHED;
+
+	folio = __filemap_get_folio(mapping, 0, fgp_flags,
 					mapping_gfp_mask(mapping));
 	if (IS_ERR(folio)) {
 		ret = PTR_ERR(folio);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 54bdd4884fe6..afae3ab64c9e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1138,6 +1138,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	int ret, needed_blocks;
 	handle_t *handle;
 	int retries = 0;
+	fgf_t fgp_flags;
 	struct folio *folio;
 	pgoff_t index;
 	unsigned from, to;
@@ -1164,6 +1165,15 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 			return 0;
 	}
 
+	/*
+	 * Set FGP_WRITEBEGIN, and FGP_UNCACHED if foliop contains
+	 * foliop_uncached. That's how generic_perform_write() informs us
+	 * that this is an uncached write.
+	 */
+	fgp_flags = FGP_WRITEBEGIN;
+	if (*foliop == foliop_uncached)
+		fgp_flags |= FGP_UNCACHED;
+
 	/*
 	 * __filemap_get_folio() can take a long time if the
 	 * system is thrashing due to memory pressure, or if the folio
@@ -1172,7 +1182,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	 * the folio (if needed) without using GFP_NOFS.
 	 */
 retry_grab:
-	folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
+	folio = __filemap_get_folio(mapping, index, fgp_flags,
 					mapping_gfp_mask(mapping));
 	if (IS_ERR(folio))
 		return PTR_ERR(folio);
@@ -2903,6 +2913,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 	struct folio *folio;
 	pgoff_t index;
 	struct inode *inode = mapping->host;
+	fgf_t fgp_flags;
 
 	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
@@ -2926,8 +2937,11 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 			return 0;
 	}
 
+	fgp_flags = FGP_WRITEBEGIN;
+	if (*foliop == foliop_uncached)
+		fgp_flags |= FGP_UNCACHED;
 retry:
-	folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
+	folio = __filemap_get_folio(mapping, index, fgp_flags,
 			mapping_gfp_mask(mapping));
 	if (IS_ERR(folio))
 		return PTR_ERR(folio);
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index ad5543866d21..10447c3c4ff1 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -226,8 +226,6 @@ static void ext4_add_complete_io(ext4_io_end_t *io_end)
 	unsigned long flags;
 
 	/* Only reserved conversions from writeback should enter here */
-	WARN_ON(!(io_end->flag & EXT4_IO_END_UNWRITTEN));
-	WARN_ON(!io_end->handle && sbi->s_journal);
 	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	wq = sbi->rsv_conversion_wq;
 	if (list_empty(&ei->i_rsv_conversion_list))
@@ -252,7 +250,7 @@ static int ext4_do_flush_completed_IO(struct inode *inode,
 
 	while (!list_empty(&unwritten)) {
 		io_end = list_entry(unwritten.next, ext4_io_end_t, list);
-		BUG_ON(!(io_end->flag & EXT4_IO_END_UNWRITTEN));
+		BUG_ON(!(io_end->flag & (EXT4_IO_END_UNWRITTEN|EXT4_IO_UNCACHED)));
 		list_del_init(&io_end->list);
 
 		err = ext4_end_io_end(io_end);
@@ -287,14 +285,15 @@ ext4_io_end_t *ext4_init_io_end(struct inode *inode, gfp_t flags)
 
 void ext4_put_io_end_defer(ext4_io_end_t *io_end)
 {
-	if (refcount_dec_and_test(&io_end->count)) {
-		if (!(io_end->flag & EXT4_IO_END_UNWRITTEN) ||
-				list_empty(&io_end->list_vec)) {
-			ext4_release_io_end(io_end);
-			return;
-		}
-		ext4_add_complete_io(io_end);
+	if (!refcount_dec_and_test(&io_end->count))
+		return;
+	if ((!(io_end->flag & EXT4_IO_END_UNWRITTEN) ||
+	    list_empty(&io_end->list_vec)) &&
+	    !(io_end->flag & EXT4_IO_UNCACHED)) {
+		ext4_release_io_end(io_end);
+		return;
 	}
+	ext4_add_complete_io(io_end);
 }
 
 int ext4_put_io_end(ext4_io_end_t *io_end)
@@ -348,7 +347,7 @@ static void ext4_end_bio(struct bio *bio)
 				blk_status_to_errno(bio->bi_status));
 	}
 
-	if (io_end->flag & EXT4_IO_END_UNWRITTEN) {
+	if (io_end->flag & (EXT4_IO_END_UNWRITTEN|EXT4_IO_UNCACHED)) {
 		/*
 		 * Link bio into list hanging from io_end. We have to do it
 		 * atomically as bio completions can be racing against each
@@ -417,8 +416,13 @@ static void io_submit_add_bh(struct ext4_io_submit *io,
 submit_and_retry:
 		ext4_io_submit(io);
 	}
-	if (io->io_bio == NULL)
+	if (io->io_bio == NULL) {
 		io_submit_init_bio(io, bh);
+		if (folio_test_uncached(folio)) {
+			ext4_io_end_t *io_end = io->io_bio->bi_private;
+			io_end->flag |= EXT4_IO_UNCACHED;
+		}
+	}
 	if (!bio_add_folio(io->io_bio, io_folio, bh->b_size, bh_offset(bh)))
 		goto submit_and_retry;
 	wbc_account_cgroup_owner(io->io_wbc, &folio->page, bh->b_size);
-- 
2.45.2

[PATCH 13/15] iomap: make buffered writes work with RWF_UNCACHED

[Jens Axboe]

Add iomap buffered write support for RWF_UNCACHED. If RWF_UNCACHED is
set for a write, mark the folios being written with drop_writeback. Then
writeback completion will drop the pages. The write_iter handler simply
kicks off writeback for the pages, and writeback completion will take
care of the rest.

This still needs the user of the iomap buffered write helpers to call
iocb_uncached_write() upon successful issue of the writes.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index ef0b68bccbb6..2f2a5db04a68 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -603,6 +603,8 @@ struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos, size_t len)
 
 	if (iter->flags & IOMAP_NOWAIT)
 		fgp |= FGP_NOWAIT;
+	if (iter->flags & IOMAP_UNCACHED)
+		fgp |= FGP_UNCACHED;
 	fgp |= fgf_set_order(len);
 
 	return __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
@@ -1023,8 +1025,9 @@ ssize_t
 iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 		const struct iomap_ops *ops, void *private)
 {
+	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct iomap_iter iter = {
-		.inode		= iocb->ki_filp->f_mapping->host,
+		.inode		= mapping->host,
 		.pos		= iocb->ki_pos,
 		.len		= iov_iter_count(i),
 		.flags		= IOMAP_WRITE,
@@ -1034,9 +1037,14 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		iter.flags |= IOMAP_NOWAIT;
+	if (iocb->ki_flags & IOCB_UNCACHED)
+		iter.flags |= IOMAP_UNCACHED;
 
-	while ((ret = iomap_iter(&iter, ops)) > 0)
+	while ((ret = iomap_iter(&iter, ops)) > 0) {
+		if (iocb->ki_flags & IOCB_UNCACHED)
+			iter.iomap.flags |= IOMAP_F_UNCACHED;
 		iter.processed = iomap_write_iter(&iter, i);
+	}
 
 	if (unlikely(iter.pos == iocb->ki_pos))
 		return ret;
@@ -1770,6 +1778,9 @@ static int iomap_add_to_ioend(struct iomap_writepage_ctx *wpc,
 	size_t poff = offset_in_folio(folio, pos);
 	int error;
 
+	if (folio_test_uncached(folio))
+		wpc->iomap.flags |= IOMAP_F_UNCACHED;
+
 	if (!wpc->ioend || !iomap_can_add_to_ioend(wpc, pos)) {
 new_ioend:
 		error = iomap_submit_ioend(wpc, 0);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index f61407e3b121..2efc72df19a2 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -64,6 +64,7 @@ struct vm_fault;
 #define IOMAP_F_BUFFER_HEAD	0
 #endif /* CONFIG_BUFFER_HEAD */
 #define IOMAP_F_XATTR		(1U << 5)
+#define IOMAP_F_UNCACHED	(1U << 6)
 
 /*
  * Flags set by the core iomap code during operations:
@@ -173,8 +174,9 @@ struct iomap_folio_ops {
 #define IOMAP_NOWAIT		(1 << 5) /* do not block */
 #define IOMAP_OVERWRITE_ONLY	(1 << 6) /* only pure overwrites allowed */
 #define IOMAP_UNSHARE		(1 << 7) /* unshare_file_range */
+#define IOMAP_UNCACHED		(1 << 8) /* uncached IO */
 #ifdef CONFIG_FS_DAX
-#define IOMAP_DAX		(1 << 8) /* DAX mapping */
+#define IOMAP_DAX		(1 << 9) /* DAX mapping */
 #else
 #define IOMAP_DAX		0
 #endif /* CONFIG_FS_DAX */
-- 
2.45.2

[PATCH 14/15] xfs: punt uncached write completions to the completion wq

[Jens Axboe]

They need non-irq context guaranteed, to be able to prune ranges from
the page cache. Treat them like unwritten extents and punt them to the
completion workqueue.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/xfs/xfs_aops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 559a3a577097..c86fc2b8f344 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -416,9 +416,12 @@ xfs_prepare_ioend(
 
 	memalloc_nofs_restore(nofs_flag);
 
-	/* send ioends that might require a transaction to the completion wq */
+	/*
+	 * Send ioends that might require a transaction or need blocking
+	 * context to the completion wq
+	 */
 	if (xfs_ioend_is_append(ioend) || ioend->io_type == IOMAP_UNWRITTEN ||
-	    (ioend->io_flags & IOMAP_F_SHARED))
+	    (ioend->io_flags & (IOMAP_F_SHARED|IOMAP_F_UNCACHED)))
 		ioend->io_bio.bi_end_io = xfs_end_bio;
 	return status;
 }
-- 
2.45.2

[PATCH 15/15] xfs: flag as supporting FOP_UNCACHED

[Jens Axboe]

Read side was already fully supported, for the write side all that's
needed now is calling generic_uncached_write() when uncached writes
have been submitted. With that, enable the use of RWF_UNCACHED with XFS
by flagging support with FOP_UNCACHED.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/xfs/xfs_file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b19916b11fd5..1a7f46e13464 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -825,6 +825,7 @@ xfs_file_buffered_write(
 
 	if (ret > 0) {
 		XFS_STATS_ADD(ip->i_mount, xs_write_bytes, ret);
+		generic_uncached_write(iocb, ret);
 		/* Handle various SYNC-type writes */
 		ret = generic_write_sync(iocb, ret);
 	}
@@ -1595,7 +1596,8 @@ const struct file_operations xfs_file_operations = {
 	.fadvise	= xfs_file_fadvise,
 	.remap_file_range = xfs_file_remap_range,
 	.fop_flags	= FOP_MMAP_SYNC | FOP_BUFFER_RASYNC |
-			  FOP_BUFFER_WASYNC | FOP_DIO_PARALLEL_WRITE,
+			  FOP_BUFFER_WASYNC | FOP_DIO_PARALLEL_WRITE |
+			  FOP_UNCACHED,
 };
 
 const struct file_operations xfs_dir_file_operations = {
-- 
2.45.2

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Kirill A. Shutemov]

On Sun, Nov 10, 2024 at 08:28:00AM -0700, Jens Axboe wrote:
> Add RWF_UNCACHED as a read operation flag, which means that any data
> read wil be removed from the page cache upon completion. Uses the page
> cache to synchronize, and simply prunes folios that were instantiated
> when the operation completes. While it would be possible to use private
> pages for this, using the page cache as synchronization is handy for a
> variety of reasons:
> 
> 1) No special truncate magic is needed
> 2) Async buffered reads need some place to serialize, using the page
>    cache is a lot easier than writing extra code for this
> 3) The pruning cost is pretty reasonable
> 
> and the code to support this is much simpler as a result.
> 
> You can think of uncached buffered IO as being the much more attractive
> cousing of O_DIRECT - it has none of the restrictions of O_DIRECT. Yes,
> it will copy the data, but unlike regular buffered IO, it doesn't run
> into the unpredictability of the page cache in terms of reclaim. As an
> example, on a test box with 32 drives, reading them with buffered IO
> looks as follows:
> 
> Reading bs 65536, uncached 0
>   1s: 145945MB/sec
>   2s: 158067MB/sec
>   3s: 157007MB/sec
>   4s: 148622MB/sec
>   5s: 118824MB/sec
>   6s: 70494MB/sec
>   7s: 41754MB/sec
>   8s: 90811MB/sec
>   9s: 92204MB/sec
>  10s: 95178MB/sec
>  11s: 95488MB/sec
>  12s: 95552MB/sec
>  13s: 96275MB/sec
> 
> where it's quite easy to see where the page cache filled up, and
> performance went from good to erratic, and finally settles at a much
> lower rate. Looking at top while this is ongoing, we see:
> 
>  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
> 7535 root      20   0  267004      0      0 S  3199   0.0   8:40.65 uncached
> 3326 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd4
> 3327 root      20   0       0      0      0 R 100.0   0.0   0:17.22 kswapd5
> 3328 root      20   0       0      0      0 R 100.0   0.0   0:13.29 kswapd6
> 3332 root      20   0       0      0      0 R 100.0   0.0   0:11.11 kswapd10
> 3339 root      20   0       0      0      0 R 100.0   0.0   0:16.25 kswapd17
> 3348 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd26
> 3343 root      20   0       0      0      0 R 100.0   0.0   0:16.30 kswapd21
> 3344 root      20   0       0      0      0 R 100.0   0.0   0:11.92 kswapd22
> 3349 root      20   0       0      0      0 R 100.0   0.0   0:16.28 kswapd27
> 3352 root      20   0       0      0      0 R  99.7   0.0   0:11.89 kswapd30
> 3353 root      20   0       0      0      0 R  96.7   0.0   0:16.04 kswapd31
> 3329 root      20   0       0      0      0 R  96.4   0.0   0:11.41 kswapd7
> 3345 root      20   0       0      0      0 R  96.4   0.0   0:13.40 kswapd23
> 3330 root      20   0       0      0      0 S  91.1   0.0   0:08.28 kswapd8
> 3350 root      20   0       0      0      0 S  86.8   0.0   0:11.13 kswapd28
> 3325 root      20   0       0      0      0 S  76.3   0.0   0:07.43 kswapd3
> 3341 root      20   0       0      0      0 S  74.7   0.0   0:08.85 kswapd19
> 3334 root      20   0       0      0      0 S  71.7   0.0   0:10.04 kswapd12
> 3351 root      20   0       0      0      0 R  60.5   0.0   0:09.59 kswapd29
> 3323 root      20   0       0      0      0 R  57.6   0.0   0:11.50 kswapd1
> [...]
> 
> which is just showing a partial list of the 32 kswapd threads that are
> running mostly full tilt, burning ~28 full CPU cores.
> 
> If the same test case is run with RWF_UNCACHED set for the buffered read,
> the output looks as follows:
> 
> Reading bs 65536, uncached 0
>   1s: 153144MB/sec
>   2s: 156760MB/sec
>   3s: 158110MB/sec
>   4s: 158009MB/sec
>   5s: 158043MB/sec
>   6s: 157638MB/sec
>   7s: 157999MB/sec
>   8s: 158024MB/sec
>   9s: 157764MB/sec
>  10s: 157477MB/sec
>  11s: 157417MB/sec
>  12s: 157455MB/sec
>  13s: 157233MB/sec
>  14s: 156692MB/sec
> 
> which is just chugging along at ~155GB/sec of read performance. Looking
> at top, we see:
> 
>  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
> 7961 root      20   0  267004      0      0 S  3180   0.0   5:37.95 uncached
> 8024 axboe     20   0   14292   4096      0 R   1.0   0.0   0:00.13 top
> 
> where just the test app is using CPU, no reclaim is taking place outside
> of the main thread. Not only is performance 65% better, it's also using
> half the CPU to do it.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  mm/filemap.c | 18 ++++++++++++++++--
>  mm/swap.c    |  2 ++
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 38dc94b761b7..bd698340ef24 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2474,6 +2474,8 @@ static int filemap_create_folio(struct kiocb *iocb,
>  	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), min_order);
>  	if (!folio)
>  		return -ENOMEM;
> +	if (iocb->ki_flags & IOCB_UNCACHED)
> +		__folio_set_uncached(folio);
>  
>  	/*
>  	 * Protect against truncate / hole punch. Grabbing invalidate_lock
> @@ -2519,6 +2521,8 @@ static int filemap_readahead(struct kiocb *iocb, struct file *file,
>  
>  	if (iocb->ki_flags & IOCB_NOIO)
>  		return -EAGAIN;
> +	if (iocb->ki_flags & IOCB_UNCACHED)
> +		ractl.uncached = 1;
>  	page_cache_async_ra(&ractl, folio, last_index - folio->index);
>  	return 0;
>  }
> @@ -2548,6 +2552,8 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
>  			return -EAGAIN;
>  		if (iocb->ki_flags & IOCB_NOWAIT)
>  			flags = memalloc_noio_save();
> +		if (iocb->ki_flags & IOCB_UNCACHED)
> +			ractl.uncached = 1;
>  		page_cache_sync_ra(&ractl, last_index - index);
>  		if (iocb->ki_flags & IOCB_NOWAIT)
>  			memalloc_noio_restore(flags);
> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>  			}
>  		}
>  put_folios:
> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> -			folio_put(fbatch.folios[i]);
> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> +			struct folio *folio = fbatch.folios[i];
> +
> +			if (folio_test_uncached(folio)) {
> +				folio_lock(folio);
> +				invalidate_complete_folio2(mapping, folio, 0);
> +				folio_unlock(folio);

I am not sure it is safe. What happens if it races with page fault?

The only current caller of invalidate_complete_folio2() unmaps the folio
explicitly before calling it. And folio lock prevents re-faulting.

I think we need to give up PG_uncached if we see folio_mapped(). And maybe
also mark the page accessed.

> +			}
> +			folio_put(folio);
> +		}
>  		folio_batch_init(&fbatch);
>  	} while (iov_iter_count(iter) && iocb->ki_pos < isize && !error);
>  
> diff --git a/mm/swap.c b/mm/swap.c
> index 835bdf324b76..f2457acae383 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
>   */
>  void folio_mark_accessed(struct folio *folio)
>  {
> +	if (folio_test_uncached(folio))
> +		return;

	if (folio_test_uncached(folio)) {
		if (folio_mapped(folio))
			folio_clear_uncached(folio);
		else
			return;
	}

>  	if (lru_gen_enabled()) {
>  		folio_inc_refs(folio);
>  		return;
> -- 
> 2.45.2
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 09/15] mm/filemap: drop uncached pages when writeback completes

[Kirill A. Shutemov]

On Sun, Nov 10, 2024 at 08:28:01AM -0700, Jens Axboe wrote:
> If the folio is marked as uncached, drop pages when writeback completes.
> Intended to be used with RWF_UNCACHED, to avoid needing sync writes for
> uncached IO.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  mm/filemap.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index bd698340ef24..efd02b047541 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1600,6 +1600,23 @@ int folio_wait_private_2_killable(struct folio *folio)
>  }
>  EXPORT_SYMBOL(folio_wait_private_2_killable);
>  
> +/*
> + * If folio was marked as uncached, then pages should be dropped when writeback
> + * completes. Do that now. If we fail, it's likely because of a big folio -
> + * just reset uncached for that case and latter completions should invalidate.
> + */
> +static void folio_end_uncached(struct folio *folio)
> +{
> +	bool reset = true;
> +
> +	if (folio_trylock(folio)) {
> +		reset = !invalidate_complete_folio2(folio->mapping, folio, 0);
> +		folio_unlock(folio);

The same problem with folio_mapped() here.

> +	}
> +	if (reset)
> +		folio_set_uncached(folio);
> +}
> +
>  /**
>   * folio_end_writeback - End writeback against a folio.
>   * @folio: The folio.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>  			}
>>  		}
>>  put_folios:
>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>> -			folio_put(fbatch.folios[i]);
>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>> +			struct folio *folio = fbatch.folios[i];
>> +
>> +			if (folio_test_uncached(folio)) {
>> +				folio_lock(folio);
>> +				invalidate_complete_folio2(mapping, folio, 0);
>> +				folio_unlock(folio);
> 
> I am not sure it is safe. What happens if it races with page fault?
> 
> The only current caller of invalidate_complete_folio2() unmaps the folio
> explicitly before calling it. And folio lock prevents re-faulting.
> 
> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> also mark the page accessed.

Ok thanks, let me take a look at that and create a test case that
exercises that explicitly.

>> diff --git a/mm/swap.c b/mm/swap.c
>> index 835bdf324b76..f2457acae383 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
>>   */
>>  void folio_mark_accessed(struct folio *folio)
>>  {
>> +	if (folio_test_uncached(folio))
>> +		return;
> 
> 	if (folio_test_uncached(folio)) {
> 		if (folio_mapped(folio))
> 			folio_clear_uncached(folio);
> 		else
> 			return;
> 	}

Noted, thanks!

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Christoph Hellwig]

On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> Ok thanks, let me take a look at that and create a test case that
> exercises that explicitly.

Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
exerciser for concurrent issuing of different I/O types to hit these
kinds of corner cases.

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 8:16 AM, Christoph Hellwig wrote:
> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>> Ok thanks, let me take a look at that and create a test case that
>> exercises that explicitly.
> 
> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
> exerciser for concurrent issuing of different I/O types to hit these
> kinds of corner cases.

Sure, can do.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Kirill A. Shutemov]

On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>  			}
> >>  		}
> >>  put_folios:
> >> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >> -			folio_put(fbatch.folios[i]);
> >> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >> +			struct folio *folio = fbatch.folios[i];
> >> +
> >> +			if (folio_test_uncached(folio)) {
> >> +				folio_lock(folio);
> >> +				invalidate_complete_folio2(mapping, folio, 0);
> >> +				folio_unlock(folio);
> > 
> > I am not sure it is safe. What happens if it races with page fault?
> > 
> > The only current caller of invalidate_complete_folio2() unmaps the folio
> > explicitly before calling it. And folio lock prevents re-faulting.
> > 
> > I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> > also mark the page accessed.
> 
> Ok thanks, let me take a look at that and create a test case that
> exercises that explicitly.

It might be worth generalizing it to clearing PG_uncached for any page cache
lookups that don't come from RWF_UNCACHED.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 15/15] xfs: flag as supporting FOP_UNCACHED

[Christoph Hellwig]

On Sun, Nov 10, 2024 at 08:28:07AM -0700, Jens Axboe wrote:
> Read side was already fully supported, for the write side all that's
> needed now is calling generic_uncached_write() when uncached writes
> have been submitted. With that, enable the use of RWF_UNCACHED with XFS
> by flagging support with FOP_UNCACHED.

It also might make sense to default to RWF_UNCACHED for the direct to
buffered I/O fallback for sub-block size writes to reflink files.

Also for the next round you probably want to add the xfs and ext4
lists.

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>>>  			}
>>>>  		}
>>>>  put_folios:
>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>>>> -			folio_put(fbatch.folios[i]);
>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>>>> +			struct folio *folio = fbatch.folios[i];
>>>> +
>>>> +			if (folio_test_uncached(folio)) {
>>>> +				folio_lock(folio);
>>>> +				invalidate_complete_folio2(mapping, folio, 0);
>>>> +				folio_unlock(folio);
>>>
>>> I am not sure it is safe. What happens if it races with page fault?
>>>
>>> The only current caller of invalidate_complete_folio2() unmaps the folio
>>> explicitly before calling it. And folio lock prevents re-faulting.
>>>
>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
>>> also mark the page accessed.
>>
>> Ok thanks, let me take a look at that and create a test case that
>> exercises that explicitly.
> 
> It might be worth generalizing it to clearing PG_uncached for any page cache
> lookups that don't come from RWF_UNCACHED.

We can do that - you mean at lookup time? Eg have __filemap_get_folio()
do:

if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
	folio_clear_uncached(folio);

or do you want this logic just in filemap_read()? Arguably it should
already clear it in the quoted code above, regardless, eg:

	if (folio_test_uncached(folio)) {
		folio_lock(folio);
		invalidate_complete_folio2(mapping, folio, 0);
		folio_clear_uncached(folio);
		folio_unlock(folio);
	}

in case invalidation fails.

-- 
Jens Axboe

Re: [PATCH 15/15] xfs: flag as supporting FOP_UNCACHED

[Jens Axboe]

On 11/11/24 8:27 AM, Christoph Hellwig wrote:
> On Sun, Nov 10, 2024 at 08:28:07AM -0700, Jens Axboe wrote:
>> Read side was already fully supported, for the write side all that's
>> needed now is calling generic_uncached_write() when uncached writes
>> have been submitted. With that, enable the use of RWF_UNCACHED with XFS
>> by flagging support with FOP_UNCACHED.
> 
> It also might make sense to default to RWF_UNCACHED for the direct to
> buffered I/O fallback for sub-block size writes to reflink files.

It very well may make sense, but that's probably something the fs folks
should add after the fact.

> Also for the next round you probably want to add the xfs and ext4
> lists.

Oh for sure, I deliberately didn't do the ext4/xfs folks just yet as I
want to iron out the main bits first. And hopefully get those sorted and
acked first, then move on to a separate xfs and ext4 set of patches. I
also did btrfs in the most recent version, but that'll be a separate
thing too. Just kept it as one big series for now with more limited
scope, so folks can actually test this if they want to, without needing
multiple series of patches.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Kirill A. Shutemov]

On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> > On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> >> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>>>  			}
> >>>>  		}
> >>>>  put_folios:
> >>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >>>> -			folio_put(fbatch.folios[i]);
> >>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >>>> +			struct folio *folio = fbatch.folios[i];
> >>>> +
> >>>> +			if (folio_test_uncached(folio)) {
> >>>> +				folio_lock(folio);
> >>>> +				invalidate_complete_folio2(mapping, folio, 0);
> >>>> +				folio_unlock(folio);
> >>>
> >>> I am not sure it is safe. What happens if it races with page fault?
> >>>
> >>> The only current caller of invalidate_complete_folio2() unmaps the folio
> >>> explicitly before calling it. And folio lock prevents re-faulting.
> >>>
> >>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> >>> also mark the page accessed.
> >>
> >> Ok thanks, let me take a look at that and create a test case that
> >> exercises that explicitly.
> > 
> > It might be worth generalizing it to clearing PG_uncached for any page cache
> > lookups that don't come from RWF_UNCACHED.
> 
> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
> do:
> 
> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
> 	folio_clear_uncached(folio);
> 
> or do you want this logic just in filemap_read()? Arguably it should
> already clear it in the quoted code above, regardless, eg:
> 
> 	if (folio_test_uncached(folio)) {
> 		folio_lock(folio);
> 		invalidate_complete_folio2(mapping, folio, 0);
> 		folio_clear_uncached(folio);
> 		folio_unlock(folio);
> 	}
> 
> in case invalidation fails.

The point is to leave the folio in page cache if there's a
non-RWF_UNCACHED user of it.

Putting the check in __filemap_get_folio() sounds reasonable.

But I am not 100% sure it would be enough to never get PG_uncached mapped.
Will think about it more.

Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
invalidate_complete_folio2().

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 8:51 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
>> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
>>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>>>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>>>>>  			}
>>>>>>  		}
>>>>>>  put_folios:
>>>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>>>>>> -			folio_put(fbatch.folios[i]);
>>>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>>>>>> +			struct folio *folio = fbatch.folios[i];
>>>>>> +
>>>>>> +			if (folio_test_uncached(folio)) {
>>>>>> +				folio_lock(folio);
>>>>>> +				invalidate_complete_folio2(mapping, folio, 0);
>>>>>> +				folio_unlock(folio);
>>>>>
>>>>> I am not sure it is safe. What happens if it races with page fault?
>>>>>
>>>>> The only current caller of invalidate_complete_folio2() unmaps the folio
>>>>> explicitly before calling it. And folio lock prevents re-faulting.
>>>>>
>>>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
>>>>> also mark the page accessed.
>>>>
>>>> Ok thanks, let me take a look at that and create a test case that
>>>> exercises that explicitly.
>>>
>>> It might be worth generalizing it to clearing PG_uncached for any page cache
>>> lookups that don't come from RWF_UNCACHED.
>>
>> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
>> do:
>>
>> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
>> 	folio_clear_uncached(folio);
>>
>> or do you want this logic just in filemap_read()? Arguably it should
>> already clear it in the quoted code above, regardless, eg:
>>
>> 	if (folio_test_uncached(folio)) {
>> 		folio_lock(folio);
>> 		invalidate_complete_folio2(mapping, folio, 0);
>> 		folio_clear_uncached(folio);
>> 		folio_unlock(folio);
>> 	}
>>
>> in case invalidation fails.
> 
> The point is to leave the folio in page cache if there's a
> non-RWF_UNCACHED user of it.

Right. The uncached flag should be ephemeral, hitting it should be
relatively rare. But if it does happen, yeah we should leave the page in
cache.

> Putting the check in __filemap_get_folio() sounds reasonable.

OK will do.

> But I am not 100% sure it would be enough to never get PG_uncached mapped.
> Will think about it more.

Thanks!

> Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
> invalidate_complete_folio2().

Isn't that a bit rough? Maybe just a:

if (WARN_ON_ONCE(folio_mapped(folio)))
	return;

would do? I'm happy to do either one, let me know what you prefer.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Kirill A. Shutemov]

On Mon, Nov 11, 2024 at 08:57:17AM -0700, Jens Axboe wrote:
> On 11/11/24 8:51 AM, Kirill A. Shutemov wrote:
> > On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
> >> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> >>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> >>>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >>>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>>>>>  			}
> >>>>>>  		}
> >>>>>>  put_folios:
> >>>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >>>>>> -			folio_put(fbatch.folios[i]);
> >>>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >>>>>> +			struct folio *folio = fbatch.folios[i];
> >>>>>> +
> >>>>>> +			if (folio_test_uncached(folio)) {
> >>>>>> +				folio_lock(folio);
> >>>>>> +				invalidate_complete_folio2(mapping, folio, 0);
> >>>>>> +				folio_unlock(folio);
> >>>>>
> >>>>> I am not sure it is safe. What happens if it races with page fault?
> >>>>>
> >>>>> The only current caller of invalidate_complete_folio2() unmaps the folio
> >>>>> explicitly before calling it. And folio lock prevents re-faulting.
> >>>>>
> >>>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> >>>>> also mark the page accessed.
> >>>>
> >>>> Ok thanks, let me take a look at that and create a test case that
> >>>> exercises that explicitly.
> >>>
> >>> It might be worth generalizing it to clearing PG_uncached for any page cache
> >>> lookups that don't come from RWF_UNCACHED.
> >>
> >> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
> >> do:
> >>
> >> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
> >> 	folio_clear_uncached(folio);
> >>
> >> or do you want this logic just in filemap_read()? Arguably it should
> >> already clear it in the quoted code above, regardless, eg:
> >>
> >> 	if (folio_test_uncached(folio)) {
> >> 		folio_lock(folio);
> >> 		invalidate_complete_folio2(mapping, folio, 0);
> >> 		folio_clear_uncached(folio);
> >> 		folio_unlock(folio);
> >> 	}
> >>
> >> in case invalidation fails.
> > 
> > The point is to leave the folio in page cache if there's a
> > non-RWF_UNCACHED user of it.
> 
> Right. The uncached flag should be ephemeral, hitting it should be
> relatively rare. But if it does happen, yeah we should leave the page in
> cache.
> 
> > Putting the check in __filemap_get_folio() sounds reasonable.
> 
> OK will do.
> 
> > But I am not 100% sure it would be enough to never get PG_uncached mapped.
> > Will think about it more.
> 
> Thanks!
> 
> > Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
> > invalidate_complete_folio2().
> 
> Isn't that a bit rough? Maybe just a:
> 
> if (WARN_ON_ONCE(folio_mapped(folio)))
> 	return;
> 
> would do? I'm happy to do either one, let me know what you prefer.

I suggested BUG_ON() because current caller has it. But, yeah, WARN() is
good enough.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 8:17 AM, Jens Axboe wrote:
> On 11/11/24 8:16 AM, Christoph Hellwig wrote:
>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>> Ok thanks, let me take a look at that and create a test case that
>>> exercises that explicitly.
>>
>> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
>> exerciser for concurrent issuing of different I/O types to hit these
>> kinds of corner cases.
> 
> Sure, can do.

Not familiar with fsstress at all, but something like the below? Will
use it if available, if it gets EOPNOTSUPP it'll just fallback to
using writev_f()/readv_f() instead.

Did give it a quick test spin and I see uncached reads and writes on the
kernel that supports it.

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 3d248ee25791..6430f10efbc7 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -82,6 +82,12 @@ static int renameat2(int dfd1, const char *path1,
 #define RENAME_WHITEOUT		(1 << 2)	/* Whiteout source */
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED		0x80
+#endif
+
+static int have_rwf_uncached = 1;
+
 #define FILELEN_MAX		(32*4096)
 
 typedef enum {
@@ -117,6 +123,7 @@ typedef enum {
 	OP_COLLAPSE,
 	OP_INSERT,
 	OP_READ,
+	OP_READ_UNCACHED,
 	OP_READLINK,
 	OP_READV,
 	OP_REMOVEFATTR,
@@ -143,6 +150,7 @@ typedef enum {
 	OP_URING_READ,
 	OP_URING_WRITE,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_WRITEV,
 	OP_EXCHANGE_RANGE,
 	OP_LAST
@@ -248,6 +256,7 @@ void	zero_f(opnum_t, long);
 void	collapse_f(opnum_t, long);
 void	insert_f(opnum_t, long);
 void	unshare_f(opnum_t, long);
+void	read_uncached_f(opnum_t, long);
 void	read_f(opnum_t, long);
 void	readlink_f(opnum_t, long);
 void	readv_f(opnum_t, long);
@@ -273,6 +282,7 @@ void	unlink_f(opnum_t, long);
 void	unresvsp_f(opnum_t, long);
 void	uring_read_f(opnum_t, long);
 void	uring_write_f(opnum_t, long);
+void	write_uncached_f(opnum_t, long);
 void	write_f(opnum_t, long);
 void	writev_f(opnum_t, long);
 void	exchangerange_f(opnum_t, long);
@@ -315,6 +325,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_COLLAPSE]	   = {"collapse",      collapse_f,	1, 1 },
 	[OP_INSERT]	   = {"insert",	       insert_f,	1, 1 },
 	[OP_READ]	   = {"read",	       read_f,		1, 0 },
+	[OP_READ_UNCACHED] = {"read_uncached", read_uncached_f,	1, 0 },
 	[OP_READLINK]	   = {"readlink",      readlink_f,	1, 0 },
 	[OP_READV]	   = {"readv",	       readv_f,		1, 0 },
 	/* remove (delete) extended attribute */
@@ -346,6 +357,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_URING_WRITE]   = {"uring_write",   uring_write_f,	1, 1 },
 	[OP_WRITE]	   = {"write",	       write_f,		4, 1 },
 	[OP_WRITEV]	   = {"writev",	       writev_f,	4, 1 },
+	[OP_WRITE_UNCACHED]= {"write_uncaced", write_uncached_f,4, 1 },
 	[OP_EXCHANGE_RANGE]= {"exchangerange", exchangerange_f,	2, 1 },
 }, *ops_end;
 
@@ -4635,6 +4647,76 @@ readv_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+read_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+
+	if (!have_rwf_uncached) {
+		readv_f(opno, r);
+		return;
+	}
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: read - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_RDONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: read - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: read - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	if (stb.st_size == 0) {
+		if (v)
+			printf("%d/%lld: read - %s%s zero size\n", procid, opno,
+			       f.path, st);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % stb.st_size);
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	e = preadv2(fd, &iov, 1, off, RWF_UNCACHED) < 0 ? errno : 0;
+	if (e == EOPNOTSUPP) {
+		have_rwf_uncached = 0;
+		e = 0;
+	}
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: read uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 void
 removefattr_f(opnum_t opno, long r)
 {
@@ -5509,6 +5591,70 @@ writev_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+write_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+
+	if (!have_rwf_uncached) {
+		writev_f(opno, r);
+		return;
+	}
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGm, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: write - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_WRONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: write - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: write - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
+	off %= maxfsize;
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	memset(iov.iov_base, nameseq & 0xff, iov.iov_len);
+	e = pwritev2(fd, &iov, 1, off, RWF_UNCACHED) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: write uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+	if (e == EOPNOTSUPP) {
+		writev_f(opno, r);
+		have_rwf_uncached = 0;
+	}
+}
+
 char *
 xattr_flag_to_string(int flag)
 {

-- 
Jens Axboe

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Matthew Wilcox]

On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
> 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
> to do buffered IO that isn't page cache persistent. The approach back
> then was to have private pages for IO, and then get rid of them once IO
> was done. But that then runs into all the issues that O_DIRECT has, in
> terms of synchronizing with the page cache.

Today's a holiday, and I suspect you're going to do a v3 before I have
a chance to do a proper review of this version of the series.

I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
so that userspace is indicating that this is a streaming I/O and the
kernel gets to choose what to do with that information.

Also, do we want to fail I/Os to filesystems which don't support
it?  I suppose really sophisticated userspace might fall back to
madvise(DONTNEED), but isn't most userspace going to just clear the flag
and retry the I/O?

Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
which is currently a noop.  But would we be better off honouring
POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
some more while I'm offline.

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Jens Axboe]

On 11/11/24 10:25 AM, Matthew Wilcox wrote:
> On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
>> 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
>> to do buffered IO that isn't page cache persistent. The approach back
>> then was to have private pages for IO, and then get rid of them once IO
>> was done. But that then runs into all the issues that O_DIRECT has, in
>> terms of synchronizing with the page cache.
> 
> Today's a holiday, and I suspect you're going to do a v3 before I have
> a chance to do a proper review of this version of the series.

Probably, since I've done some fixes since v2 :-). So you can wait for
v3, I'll post it later today anyway.

> I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
> so that userspace is indicating that this is a streaming I/O and the
> kernel gets to choose what to do with that information.

Yeah not sure, it's the one I used back in the day, and I still haven't
found a more descriptive word for it. That doesn't mean one doesn't
exist, certainly taking suggestions. I don't think STREAMING is the
right one however, you could most certainly be doing random uncached IO.

> Also, do we want to fail I/Os to filesystems which don't support
> it?  I suppose really sophisticated userspace might fall back to
> madvise(DONTNEED), but isn't most userspace going to just clear the flag
> and retry the I/O?

Also something that's a bit undecided, you can make arguments for both
ways. For just ignoring the flag if not support, the argument would be
that the application just wants to do IO, uncached if available. For the
other argument, maybe you have an application that wants to fallback to
O_DIRECT if uncached isn't available. That application certainly wants
to know if it works or not.

Which is why I defaulted to return -EOPNOTSUPP if it's not available.
An applicaton may probe this upfront if it so desires, and just not set
the flag for IO. That'd keep it out of the hot path.

Seems to me that returning whether it's supported or not is the path of
least surprises for applications, which is why I went that way.

> Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
> which is currently a noop.  But would we be better off honouring
> POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
> some more while I'm offline.

That would certainly work too, for synchronous IO. But per-file hints
are a bad idea for async IO, for obvious reasons. We really want per-IO
hints for that, we have a long history of messing that up. That doesn't
mean that FMODE_NOREUSE couldn't just set RWF_UNCACHED, if it's set.
That'd be trivial.

Then the next question is if setting POSIX_FADV_NOREUSE should fail of
file->f_op->fop_flags & FOP_UNCACHED isn't true. Probably not, since
it'd potentially break applications. So probably best to just set
f_iocb_flags IFF FOP_UNCACHED is true for that file.

And the bigger question is why on earth do we have this thing in the
kernel that doesn't do anything... But yeah, now we could make it do
something.

-- 
Jens Axboe

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Yu Zhao]

On Mon, Nov 11, 2024 at 10:25 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
> > 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
> > to do buffered IO that isn't page cache persistent. The approach back
> > then was to have private pages for IO, and then get rid of them once IO
> > was done. But that then runs into all the issues that O_DIRECT has, in
> > terms of synchronizing with the page cache.
>
> Today's a holiday, and I suspect you're going to do a v3 before I have
> a chance to do a proper review of this version of the series.
>
> I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
> so that userspace is indicating that this is a streaming I/O and the
> kernel gets to choose what to do with that information.
>
> Also, do we want to fail I/Os to filesystems which don't support
> it?  I suppose really sophisticated userspace might fall back to
> madvise(DONTNEED), but isn't most userspace going to just clear the flag
> and retry the I/O?
>
> Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
> which is currently a noop.

Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:
support POSIX_FADV_NOREUSE"). And it had at least one user (we made
the userpspace change after the kernel supported it): SVT-AV1 [1]; it
was also added to FIO for testing purposes.

[1] https://gitlab.com/AOMediaCodec/SVT-AV1

> But would we be better off honouring
> POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
> some more while I'm offline.

But I guess the flag isn't honored the way UNCACHED works?

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Matthew Wilcox]

On Mon, Nov 11, 2024 at 02:24:54PM -0700, Yu Zhao wrote:
> Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:

maybe you should send a patch to the manpage?

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Yu Zhao]

On Mon, Nov 11, 2024 at 2:48 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 11, 2024 at 02:24:54PM -0700, Yu Zhao wrote:
> > Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:
>
> maybe you should send a patch to the manpage?

I was under the impression that our engineers took care of that. But
apparently it's still pending:
https://lore.kernel.org/linux-man/20230320222057.1976956-1-talumbau@google.com/

Will find someone else to follow up on that.

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 10:09 AM, Jens Axboe wrote:
> On 11/11/24 8:17 AM, Jens Axboe wrote:
>> On 11/11/24 8:16 AM, Christoph Hellwig wrote:
>>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>>> Ok thanks, let me take a look at that and create a test case that
>>>> exercises that explicitly.
>>>
>>> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
>>> exerciser for concurrent issuing of different I/O types to hit these
>>> kinds of corner cases.
>>
>> Sure, can do.
> 
> Not familiar with fsstress at all, but something like the below? Will
> use it if available, if it gets EOPNOTSUPP it'll just fallback to
> using writev_f()/readv_f() instead.
> 
> Did give it a quick test spin and I see uncached reads and writes on the
> kernel that supports it.

Here's the slightly cleaned up version, this is the one I ran testing
with.

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 3d248ee25791..a06ba300a1d7 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -82,6 +82,12 @@ static int renameat2(int dfd1, const char *path1,
 #define RENAME_WHITEOUT		(1 << 2)	/* Whiteout source */
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED		0x80
+#endif
+
+static int have_rwf_uncached = 1;
+
 #define FILELEN_MAX		(32*4096)
 
 typedef enum {
@@ -117,6 +123,7 @@ typedef enum {
 	OP_COLLAPSE,
 	OP_INSERT,
 	OP_READ,
+	OP_READ_UNCACHED,
 	OP_READLINK,
 	OP_READV,
 	OP_REMOVEFATTR,
@@ -143,6 +150,7 @@ typedef enum {
 	OP_URING_READ,
 	OP_URING_WRITE,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_WRITEV,
 	OP_EXCHANGE_RANGE,
 	OP_LAST
@@ -248,6 +256,7 @@ void	zero_f(opnum_t, long);
 void	collapse_f(opnum_t, long);
 void	insert_f(opnum_t, long);
 void	unshare_f(opnum_t, long);
+void	read_uncached_f(opnum_t, long);
 void	read_f(opnum_t, long);
 void	readlink_f(opnum_t, long);
 void	readv_f(opnum_t, long);
@@ -273,6 +282,7 @@ void	unlink_f(opnum_t, long);
 void	unresvsp_f(opnum_t, long);
 void	uring_read_f(opnum_t, long);
 void	uring_write_f(opnum_t, long);
+void	write_uncached_f(opnum_t, long);
 void	write_f(opnum_t, long);
 void	writev_f(opnum_t, long);
 void	exchangerange_f(opnum_t, long);
@@ -315,6 +325,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_COLLAPSE]	   = {"collapse",      collapse_f,	1, 1 },
 	[OP_INSERT]	   = {"insert",	       insert_f,	1, 1 },
 	[OP_READ]	   = {"read",	       read_f,		1, 0 },
+	[OP_READ_UNCACHED] = {"read_uncached", read_uncached_f,	1, 0 },
 	[OP_READLINK]	   = {"readlink",      readlink_f,	1, 0 },
 	[OP_READV]	   = {"readv",	       readv_f,		1, 0 },
 	/* remove (delete) extended attribute */
@@ -346,6 +357,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_URING_WRITE]   = {"uring_write",   uring_write_f,	1, 1 },
 	[OP_WRITE]	   = {"write",	       write_f,		4, 1 },
 	[OP_WRITEV]	   = {"writev",	       writev_f,	4, 1 },
+	[OP_WRITE_UNCACHED]= {"write_uncaced", write_uncached_f,4, 1 },
 	[OP_EXCHANGE_RANGE]= {"exchangerange", exchangerange_f,	2, 1 },
 }, *ops_end;
 
@@ -4635,6 +4647,71 @@ readv_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+read_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+	int flags;
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: read - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_RDONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: read - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: read - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	if (stb.st_size == 0) {
+		if (v)
+			printf("%d/%lld: read - %s%s zero size\n", procid, opno,
+			       f.path, st);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % stb.st_size);
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	flags = have_rwf_uncached ? RWF_UNCACHED : 0;
+	e = preadv2(fd, &iov, 1, off, flags) < 0 ? errno : 0;
+	if (have_rwf_uncached && e == EOPNOTSUPP)
+		e = preadv2(fd, &iov, 1, off, 0) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: read uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 void
 removefattr_f(opnum_t opno, long r)
 {
@@ -5509,6 +5586,65 @@ writev_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+write_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+	int flags;
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGm, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: write - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_WRONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: write - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: write - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
+	off %= maxfsize;
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	memset(iov.iov_base, nameseq & 0xff, iov.iov_len);
+	flags = have_rwf_uncached ? RWF_UNCACHED : 0;
+	e = pwritev2(fd, &iov, 1, off, flags) < 0 ? errno : 0;
+	if (have_rwf_uncached && e == EOPNOTSUPP)
+		e = pwritev2(fd, &iov, 1, off, 0) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: write uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 char *
 xattr_flag_to_string(int flag)
 {

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Christoph Hellwig]

On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> Here's the slightly cleaned up version, this is the one I ran testing
> with.

Looks reasonable to me, but you probably get better reviews on the
fstests lists.

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>> Here's the slightly cleaned up version, this is the one I ran testing
>> with.
> 
> Looks reasonable to me, but you probably get better reviews on the
> fstests lists.

I'll send it out once this patchset is a bit closer to integration,
there's the usual chicken and egg situation with it. For now, it's quite
handy for my testing, found a few issues with this version. So thanks
for the suggestion, sure beats writing more of your own test cases :-)

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Brian Foster]

On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> > On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >> Here's the slightly cleaned up version, this is the one I ran testing
> >> with.
> > 
> > Looks reasonable to me, but you probably get better reviews on the
> > fstests lists.
> 
> I'll send it out once this patchset is a bit closer to integration,
> there's the usual chicken and egg situation with it. For now, it's quite
> handy for my testing, found a few issues with this version. So thanks
> for the suggestion, sure beats writing more of your own test cases :-)
> 

fsx support is probably a good idea as well. It's similar in idea to
fsstress, but bashes the same file with mixed operations and includes
data integrity validation checks as well. It's pretty useful for
uncovering subtle corner case issues or bad interactions..

Brian

> -- 
> Jens Axboe
> 

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 9:39 AM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>> with.
>>>
>>> Looks reasonable to me, but you probably get better reviews on the
>>> fstests lists.
>>
>> I'll send it out once this patchset is a bit closer to integration,
>> there's the usual chicken and egg situation with it. For now, it's quite
>> handy for my testing, found a few issues with this version. So thanks
>> for the suggestion, sure beats writing more of your own test cases :-)
>>
> 
> fsx support is probably a good idea as well. It's similar in idea to
> fsstress, but bashes the same file with mixed operations and includes
> data integrity validation checks as well. It's pretty useful for
> uncovering subtle corner case issues or bad interactions..

Indeed, I did that too. Re-running xfstests right now with that too.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 10:06 AM, Jens Axboe wrote:
> On 11/12/24 9:39 AM, Brian Foster wrote:
>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>> with.
>>>>
>>>> Looks reasonable to me, but you probably get better reviews on the
>>>> fstests lists.
>>>
>>> I'll send it out once this patchset is a bit closer to integration,
>>> there's the usual chicken and egg situation with it. For now, it's quite
>>> handy for my testing, found a few issues with this version. So thanks
>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>
>>
>> fsx support is probably a good idea as well. It's similar in idea to
>> fsstress, but bashes the same file with mixed operations and includes
>> data integrity validation checks as well. It's pretty useful for
>> uncovering subtle corner case issues or bad interactions..
> 
> Indeed, I did that too. Re-running xfstests right now with that too.

Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
for both the sync read/write and io_uring paths.


diff --git a/ltp/fsx.c b/ltp/fsx.c
index 41933354..104910ff 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -43,6 +43,10 @@
 # define MAP_FILE 0
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED	0x80
+#endif
+
 #define NUMPRINTCOLUMNS 32	/* # columns of data to print on each line */
 
 /* Operation flags (bitmask) */
@@ -101,7 +105,9 @@ int			logcount = 0;	/* total ops */
 enum {
 	/* common operations */
 	OP_READ = 0,
+	OP_READ_UNCACHED,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_MAPREAD,
 	OP_MAPWRITE,
 	OP_MAX_LITE,
@@ -190,15 +196,16 @@ int	o_direct;			/* -Z */
 int	aio = 0;
 int	uring = 0;
 int	mark_nr = 0;
+int	rwf_uncached = 1;
 
 int page_size;
 int page_mask;
 int mmap_mask;
-int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset);
+int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags);
 #define READ 0
 #define WRITE 1
-#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
-#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
+#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
+#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
 
 struct timespec deadline;
 
@@ -266,7 +273,9 @@ prterr(const char *prefix)
 
 static const char *op_names[] = {
 	[OP_READ] = "read",
+	[OP_READ_UNCACHED] = "read_uncached",
 	[OP_WRITE] = "write",
+	[OP_WRITE_UNCACHED] = "write_uncached",
 	[OP_MAPREAD] = "mapread",
 	[OP_MAPWRITE] = "mapwrite",
 	[OP_TRUNCATE] = "truncate",
@@ -393,12 +402,14 @@ logdump(void)
 				prt("\t******WWWW");
 			break;
 		case OP_READ:
+		case OP_READ_UNCACHED:
 			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (overlap)
 				prt("\t***RRRR***");
 			break;
+		case OP_WRITE_UNCACHED:
 		case OP_WRITE:
 			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
@@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
 }
 
 void
-doread(unsigned offset, unsigned size)
+__doread(unsigned offset, unsigned size, int flags)
 {
-	off_t ret;
 	unsigned iret;
 
 	offset -= offset % readbdy;
@@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
 			(monitorend == -1 || offset <= monitorend))))))
 		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
 		    offset, offset + size - 1, size);
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("doread: lseek");
-		report_failure(140);
-	}
-	iret = fsxread(fd, temp_buf, size, offset);
+	iret = fsxread(fd, temp_buf, size, offset, flags);
 	if (iret != size) {
-		if (iret == -1)
-			prterr("doread: read");
-		else
+		if (iret == -1) {
+			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
+				rwf_uncached = 1;
+				return;
+			}
+			prterr("dowrite: read");
+		} else {
 			prt("short read: 0x%x bytes instead of 0x%x\n",
 			    iret, size);
+		}
 		report_failure(141);
 	}
 	check_buffers(temp_buf, offset, size);
 }
+void
+doread(unsigned offset, unsigned size)
+{
+	__doread(offset, size, 0);
+}
 
+void
+doread_uncached(unsigned offset, unsigned size)
+{
+	if (rwf_uncached) {
+		__doread(offset, size, RWF_UNCACHED);
+		if (rwf_uncached)
+			return;
+	}
+	__doread(offset, size, 0);
+}
+	
 void
 check_eofpage(char *s, unsigned offset, char *p, int size)
 {
@@ -870,7 +896,6 @@ check_contents(void)
 	unsigned map_offset;
 	unsigned map_size;
 	char *p;
-	off_t ret;
 	unsigned iret;
 
 	if (!check_buf) {
@@ -885,13 +910,7 @@ check_contents(void)
 	if (size == 0)
 		return;
 
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("doread: lseek");
-		report_failure(140);
-	}
-
-	iret = fsxread(fd, check_buf, size, offset);
+	iret = fsxread(fd, check_buf, size, offset, 0);
 	if (iret != size) {
 		if (iret == -1)
 			prterr("check_contents: read");
@@ -1064,9 +1083,8 @@ update_file_size(unsigned offset, unsigned size)
 }
 
 void
-dowrite(unsigned offset, unsigned size)
+__dowrite(unsigned offset, unsigned size, int flags)
 {
-	off_t ret;
 	unsigned iret;
 
 	offset -= offset % writebdy;
@@ -1101,18 +1119,18 @@ dowrite(unsigned offset, unsigned size)
 			(monitorend == -1 || offset <= monitorend))))))
 		prt("%lld write\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
 		    offset, offset + size - 1, size);
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("dowrite: lseek");
-		report_failure(150);
-	}
-	iret = fsxwrite(fd, good_buf + offset, size, offset);
+	iret = fsxwrite(fd, good_buf + offset, size, offset, flags);
 	if (iret != size) {
-		if (iret == -1)
+		if (iret == -1) {
+			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
+				rwf_uncached = 0;
+				return;
+			}
 			prterr("dowrite: write");
-		else
+		} else {
 			prt("short write: 0x%x bytes instead of 0x%x\n",
 			    iret, size);
+		}
 		report_failure(151);
 	}
 	if (do_fsync) {
@@ -1126,6 +1144,22 @@ dowrite(unsigned offset, unsigned size)
 	}
 }
 
+void
+dowrite(unsigned offset, unsigned size)
+{
+	__dowrite(offset, size, 0);
+}
+
+void
+dowrite_uncached(unsigned offset, unsigned size)
+{
+	if (rwf_uncached) {
+		__dowrite(offset, size, RWF_UNCACHED);
+		if (rwf_uncached)
+			return;
+	}
+	__dowrite(offset, size, 0);
+}
 
 void
 domapwrite(unsigned offset, unsigned size)
@@ -2340,11 +2374,21 @@ have_op:
 		doread(offset, size);
 		break;
 
+	case OP_READ_UNCACHED:
+		TRIM_OFF_LEN(offset, size, file_size);
+		doread_uncached(offset, size);
+		break;
+
 	case OP_WRITE:
 		TRIM_OFF_LEN(offset, size, maxfilelen);
 		dowrite(offset, size);
 		break;
 
+	case OP_WRITE_UNCACHED:
+		TRIM_OFF_LEN(offset, size, maxfilelen);
+		dowrite_uncached(offset, size);
+		break;
+
 	case OP_MAPREAD:
 		TRIM_OFF_LEN(offset, size, file_size);
 		domapread(offset, size);
@@ -2702,7 +2746,7 @@ uring_setup()
 }
 
 int
-uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	struct io_uring_sqe     *sqe;
 	struct io_uring_cqe     *cqe;
@@ -2733,6 +2777,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 		} else {
 			io_uring_prep_writev(sqe, fd, &iovec, 1, o);
 		}
+		sqe->rw_flags = flags;
 
 		ret = io_uring_submit_and_wait(&ring, 1);
 		if (ret != 1) {
@@ -2781,7 +2826,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 }
 #else
 int
-uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	fprintf(stderr, "io_rw: need IO_URING support!\n");
 	exit(111);
@@ -2789,19 +2834,21 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 #endif
 
 int
-fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	int ret;
 
 	if (aio) {
 		ret = aio_rw(rw, fd, buf, len, offset);
 	} else if (uring) {
-		ret = uring_rw(rw, fd, buf, len, offset);
+		ret = uring_rw(rw, fd, buf, len, offset, flags);
 	} else {
+		struct iovec iov = { .iov_base = buf, .iov_len = len };
+
 		if (rw == READ)
-			ret = read(fd, buf, len);
+			ret = preadv2(fd, &iov, 1, offset, flags);
 		else
-			ret = write(fd, buf, len);
+			ret = pwritev2(fd, &iov, 1, offset, flags);
 	}
 	return ret;
 }


-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Brian Foster]

On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> On 11/12/24 10:06 AM, Jens Axboe wrote:
> > On 11/12/24 9:39 AM, Brian Foster wrote:
> >> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>> with.
> >>>>
> >>>> Looks reasonable to me, but you probably get better reviews on the
> >>>> fstests lists.
> >>>
> >>> I'll send it out once this patchset is a bit closer to integration,
> >>> there's the usual chicken and egg situation with it. For now, it's quite
> >>> handy for my testing, found a few issues with this version. So thanks
> >>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>
> >>
> >> fsx support is probably a good idea as well. It's similar in idea to
> >> fsstress, but bashes the same file with mixed operations and includes
> >> data integrity validation checks as well. It's pretty useful for
> >> uncovering subtle corner case issues or bad interactions..
> > 
> > Indeed, I did that too. Re-running xfstests right now with that too.
> 
> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> for both the sync read/write and io_uring paths.
> 

Nice, thanks. Looks reasonable to me at first glance. A few randomish
comments inlined below.

BTW, I should have also mentioned that fsx is also useful for longer
soak testing. I.e., fstests will provide a decent amount of coverage as
is via the various preexisting tests, but I'll occasionally run fsx
directly and let it run overnight or something to get the op count at
least up in the 100 millions or so to have a little more confidence
there isn't some rare/subtle bug lurking. That might be helpful with
something like this. JFYI.

> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 41933354..104910ff 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -43,6 +43,10 @@
>  # define MAP_FILE 0
>  #endif
>  
> +#ifndef RWF_UNCACHED
> +#define RWF_UNCACHED	0x80
> +#endif
> +
>  #define NUMPRINTCOLUMNS 32	/* # columns of data to print on each line */
>  
>  /* Operation flags (bitmask) */
> @@ -101,7 +105,9 @@ int			logcount = 0;	/* total ops */
>  enum {
>  	/* common operations */
>  	OP_READ = 0,
> +	OP_READ_UNCACHED,
>  	OP_WRITE,
> +	OP_WRITE_UNCACHED,
>  	OP_MAPREAD,
>  	OP_MAPWRITE,
>  	OP_MAX_LITE,
> @@ -190,15 +196,16 @@ int	o_direct;			/* -Z */
>  int	aio = 0;
>  int	uring = 0;
>  int	mark_nr = 0;
> +int	rwf_uncached = 1;
>  
>  int page_size;
>  int page_mask;
>  int mmap_mask;
> -int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset);
> +int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags);
>  #define READ 0
>  #define WRITE 1
> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
>  

My pattern recognition brain wants to see an 'e' here. ;)

>  struct timespec deadline;
>  
> @@ -266,7 +273,9 @@ prterr(const char *prefix)
>  
>  static const char *op_names[] = {
>  	[OP_READ] = "read",
> +	[OP_READ_UNCACHED] = "read_uncached",
>  	[OP_WRITE] = "write",
> +	[OP_WRITE_UNCACHED] = "write_uncached",
>  	[OP_MAPREAD] = "mapread",
>  	[OP_MAPWRITE] = "mapwrite",
>  	[OP_TRUNCATE] = "truncate",
> @@ -393,12 +402,14 @@ logdump(void)
>  				prt("\t******WWWW");
>  			break;
>  		case OP_READ:
> +		case OP_READ_UNCACHED:
>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>  			    lp->args[1]);
>  			if (overlap)
>  				prt("\t***RRRR***");
>  			break;
> +		case OP_WRITE_UNCACHED:
>  		case OP_WRITE:
>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
>  }
>  
>  void
> -doread(unsigned offset, unsigned size)
> +__doread(unsigned offset, unsigned size, int flags)
>  {
> -	off_t ret;
>  	unsigned iret;
>  
>  	offset -= offset % readbdy;
> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
>  			(monitorend == -1 || offset <= monitorend))))))
>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>  		    offset, offset + size - 1, size);
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("doread: lseek");
> -		report_failure(140);
> -	}
> -	iret = fsxread(fd, temp_buf, size, offset);
> +	iret = fsxread(fd, temp_buf, size, offset, flags);
>  	if (iret != size) {
> -		if (iret == -1)
> -			prterr("doread: read");
> -		else
> +		if (iret == -1) {
> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> +				rwf_uncached = 1;

I assume you meant rwf_uncached = 0 here?

If so, check out test_fallocate() and friends to see how various
operations are tested for support before the test starts. Following that
might clean things up a bit.

Also it's useful to have a CLI option to enable/disable individual
features. That tends to be helpful to narrow things down when it does
happen to explode and you want to narrow down the cause.

Brian

> +				return;
> +			}
> +			prterr("dowrite: read");
> +		} else {
>  			prt("short read: 0x%x bytes instead of 0x%x\n",
>  			    iret, size);
> +		}
>  		report_failure(141);
>  	}
>  	check_buffers(temp_buf, offset, size);
>  }
> +void
> +doread(unsigned offset, unsigned size)
> +{
> +	__doread(offset, size, 0);
> +}
>  
> +void
> +doread_uncached(unsigned offset, unsigned size)
> +{
> +	if (rwf_uncached) {
> +		__doread(offset, size, RWF_UNCACHED);
> +		if (rwf_uncached)
> +			return;
> +	}
> +	__doread(offset, size, 0);
> +}
> +	
>  void
>  check_eofpage(char *s, unsigned offset, char *p, int size)
>  {
> @@ -870,7 +896,6 @@ check_contents(void)
>  	unsigned map_offset;
>  	unsigned map_size;
>  	char *p;
> -	off_t ret;
>  	unsigned iret;
>  
>  	if (!check_buf) {
> @@ -885,13 +910,7 @@ check_contents(void)
>  	if (size == 0)
>  		return;
>  
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("doread: lseek");
> -		report_failure(140);
> -	}
> -
> -	iret = fsxread(fd, check_buf, size, offset);
> +	iret = fsxread(fd, check_buf, size, offset, 0);
>  	if (iret != size) {
>  		if (iret == -1)
>  			prterr("check_contents: read");
> @@ -1064,9 +1083,8 @@ update_file_size(unsigned offset, unsigned size)
>  }
>  
>  void
> -dowrite(unsigned offset, unsigned size)
> +__dowrite(unsigned offset, unsigned size, int flags)
>  {
> -	off_t ret;
>  	unsigned iret;
>  
>  	offset -= offset % writebdy;
> @@ -1101,18 +1119,18 @@ dowrite(unsigned offset, unsigned size)
>  			(monitorend == -1 || offset <= monitorend))))))
>  		prt("%lld write\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>  		    offset, offset + size - 1, size);
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("dowrite: lseek");
> -		report_failure(150);
> -	}
> -	iret = fsxwrite(fd, good_buf + offset, size, offset);
> +	iret = fsxwrite(fd, good_buf + offset, size, offset, flags);
>  	if (iret != size) {
> -		if (iret == -1)
> +		if (iret == -1) {
> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> +				rwf_uncached = 0;
> +				return;
> +			}
>  			prterr("dowrite: write");
> -		else
> +		} else {
>  			prt("short write: 0x%x bytes instead of 0x%x\n",
>  			    iret, size);
> +		}
>  		report_failure(151);
>  	}
>  	if (do_fsync) {
> @@ -1126,6 +1144,22 @@ dowrite(unsigned offset, unsigned size)
>  	}
>  }
>  
> +void
> +dowrite(unsigned offset, unsigned size)
> +{
> +	__dowrite(offset, size, 0);
> +}
> +
> +void
> +dowrite_uncached(unsigned offset, unsigned size)
> +{
> +	if (rwf_uncached) {
> +		__dowrite(offset, size, RWF_UNCACHED);
> +		if (rwf_uncached)
> +			return;
> +	}
> +	__dowrite(offset, size, 0);
> +}
>  
>  void
>  domapwrite(unsigned offset, unsigned size)
> @@ -2340,11 +2374,21 @@ have_op:
>  		doread(offset, size);
>  		break;
>  
> +	case OP_READ_UNCACHED:
> +		TRIM_OFF_LEN(offset, size, file_size);
> +		doread_uncached(offset, size);
> +		break;
> +
>  	case OP_WRITE:
>  		TRIM_OFF_LEN(offset, size, maxfilelen);
>  		dowrite(offset, size);
>  		break;
>  
> +	case OP_WRITE_UNCACHED:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		dowrite_uncached(offset, size);
> +		break;
> +
>  	case OP_MAPREAD:
>  		TRIM_OFF_LEN(offset, size, file_size);
>  		domapread(offset, size);
> @@ -2702,7 +2746,7 @@ uring_setup()
>  }
>  
>  int
> -uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	struct io_uring_sqe     *sqe;
>  	struct io_uring_cqe     *cqe;
> @@ -2733,6 +2777,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  		} else {
>  			io_uring_prep_writev(sqe, fd, &iovec, 1, o);
>  		}
> +		sqe->rw_flags = flags;
>  
>  		ret = io_uring_submit_and_wait(&ring, 1);
>  		if (ret != 1) {
> @@ -2781,7 +2826,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  }
>  #else
>  int
> -uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	fprintf(stderr, "io_rw: need IO_URING support!\n");
>  	exit(111);
> @@ -2789,19 +2834,21 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  #endif
>  
>  int
> -fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	int ret;
>  
>  	if (aio) {
>  		ret = aio_rw(rw, fd, buf, len, offset);
>  	} else if (uring) {
> -		ret = uring_rw(rw, fd, buf, len, offset);
> +		ret = uring_rw(rw, fd, buf, len, offset, flags);
>  	} else {
> +		struct iovec iov = { .iov_base = buf, .iov_len = len };
> +
>  		if (rw == READ)
> -			ret = read(fd, buf, len);
> +			ret = preadv2(fd, &iov, 1, offset, flags);
>  		else
> -			ret = write(fd, buf, len);
> +			ret = pwritev2(fd, &iov, 1, offset, flags);
>  	}
>  	return ret;
>  }
> 
> 
> -- 
> Jens Axboe
> 

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 11:44 AM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>> with.
>>>>>>
>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>> fstests lists.
>>>>>
>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>
>>>>
>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>> fsstress, but bashes the same file with mixed operations and includes
>>>> data integrity validation checks as well. It's pretty useful for
>>>> uncovering subtle corner case issues or bad interactions..
>>>
>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>
>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>> for both the sync read/write and io_uring paths.
>>
> 
> Nice, thanks. Looks reasonable to me at first glance. A few randomish
> comments inlined below.
> 
> BTW, I should have also mentioned that fsx is also useful for longer
> soak testing. I.e., fstests will provide a decent amount of coverage as
> is via the various preexisting tests, but I'll occasionally run fsx
> directly and let it run overnight or something to get the op count at
> least up in the 100 millions or so to have a little more confidence
> there isn't some rare/subtle bug lurking. That might be helpful with
> something like this. JFYI.

Good suggestion, I can leave it running overnight here as well. Since
I'm not super familiar with it, what would be a good set of parameters
to run it with?

>>  #define READ 0
>>  #define WRITE 1
>> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
>> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
>> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
>> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
>>  
> 
> My pattern recognition brain wants to see an 'e' here. ;)

This is a "check if reviewer has actually looked at it" check ;-)

>> @@ -266,7 +273,9 @@ prterr(const char *prefix)
>>  
>>  static const char *op_names[] = {
>>  	[OP_READ] = "read",
>> +	[OP_READ_UNCACHED] = "read_uncached",
>>  	[OP_WRITE] = "write",
>> +	[OP_WRITE_UNCACHED] = "write_uncached",
>>  	[OP_MAPREAD] = "mapread",
>>  	[OP_MAPWRITE] = "mapwrite",
>>  	[OP_TRUNCATE] = "truncate",
>> @@ -393,12 +402,14 @@ logdump(void)
>>  				prt("\t******WWWW");
>>  			break;
>>  		case OP_READ:
>> +		case OP_READ_UNCACHED:
>>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
>>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>>  			    lp->args[1]);
>>  			if (overlap)
>>  				prt("\t***RRRR***");
>>  			break;
>> +		case OP_WRITE_UNCACHED:
>>  		case OP_WRITE:
>>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
>>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
>>  }
>>  
>>  void
>> -doread(unsigned offset, unsigned size)
>> +__doread(unsigned offset, unsigned size, int flags)
>>  {
>> -	off_t ret;
>>  	unsigned iret;
>>  
>>  	offset -= offset % readbdy;
>> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
>>  			(monitorend == -1 || offset <= monitorend))))))
>>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>>  		    offset, offset + size - 1, size);
>> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
>> -	if (ret == (off_t)-1) {
>> -		prterr("doread: lseek");
>> -		report_failure(140);
>> -	}
>> -	iret = fsxread(fd, temp_buf, size, offset);
>> +	iret = fsxread(fd, temp_buf, size, offset, flags);
>>  	if (iret != size) {
>> -		if (iret == -1)
>> -			prterr("doread: read");
>> -		else
>> +		if (iret == -1) {
>> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
>> +				rwf_uncached = 1;
> 
> I assume you meant rwf_uncached = 0 here?

Indeed, good catch. Haven't tested this on a kernel without RWF_UNCACHED
yet...

> If so, check out test_fallocate() and friends to see how various
> operations are tested for support before the test starts. Following that
> might clean things up a bit.

Sure, I can do something like that instead. fsx looks pretty old school
in its design, was not expecting a static (and single) fd. But since we
have that, we can do the probe and check. Just a basic read would be
enough, with RWF_UNCACHED set.

> Also it's useful to have a CLI option to enable/disable individual
> features. That tends to be helpful to narrow things down when it does
> happen to explode and you want to narrow down the cause.

I can add a -U for "do not use uncached".

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Brian Foster]

On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
> On 11/12/24 11:44 AM, Brian Foster wrote:
> > On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> >> On 11/12/24 10:06 AM, Jens Axboe wrote:
> >>> On 11/12/24 9:39 AM, Brian Foster wrote:
> >>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>>>> with.
> >>>>>>
> >>>>>> Looks reasonable to me, but you probably get better reviews on the
> >>>>>> fstests lists.
> >>>>>
> >>>>> I'll send it out once this patchset is a bit closer to integration,
> >>>>> there's the usual chicken and egg situation with it. For now, it's quite
> >>>>> handy for my testing, found a few issues with this version. So thanks
> >>>>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>>>
> >>>>
> >>>> fsx support is probably a good idea as well. It's similar in idea to
> >>>> fsstress, but bashes the same file with mixed operations and includes
> >>>> data integrity validation checks as well. It's pretty useful for
> >>>> uncovering subtle corner case issues or bad interactions..
> >>>
> >>> Indeed, I did that too. Re-running xfstests right now with that too.
> >>
> >> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> >> for both the sync read/write and io_uring paths.
> >>
> > 
> > Nice, thanks. Looks reasonable to me at first glance. A few randomish
> > comments inlined below.
> > 
> > BTW, I should have also mentioned that fsx is also useful for longer
> > soak testing. I.e., fstests will provide a decent amount of coverage as
> > is via the various preexisting tests, but I'll occasionally run fsx
> > directly and let it run overnight or something to get the op count at
> > least up in the 100 millions or so to have a little more confidence
> > there isn't some rare/subtle bug lurking. That might be helpful with
> > something like this. JFYI.
> 
> Good suggestion, I can leave it running overnight here as well. Since
> I'm not super familiar with it, what would be a good set of parameters
> to run it with?
> 

Most things are on by default, so I'd probably just go with that. -p is
useful to get occasional status output on how many operations have
completed and you could consider increasing the max file size with -l,
but usually I don't use more than a few MB or so if I increase it at
all.

Random other thought: I also wonder if uncached I/O should be an
exclusive mode more similar to like how O_DIRECT or AIO is implemented.
But I dunno, maybe it doesn't matter that much (or maybe others will
have opinions on the fstests list).

Brian

> >>  #define READ 0
> >>  #define WRITE 1
> >> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
> >> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
> >> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
> >> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
> >>  
> > 
> > My pattern recognition brain wants to see an 'e' here. ;)
> 
> This is a "check if reviewer has actually looked at it" check ;-)
> 
> >> @@ -266,7 +273,9 @@ prterr(const char *prefix)
> >>  
> >>  static const char *op_names[] = {
> >>  	[OP_READ] = "read",
> >> +	[OP_READ_UNCACHED] = "read_uncached",
> >>  	[OP_WRITE] = "write",
> >> +	[OP_WRITE_UNCACHED] = "write_uncached",
> >>  	[OP_MAPREAD] = "mapread",
> >>  	[OP_MAPWRITE] = "mapwrite",
> >>  	[OP_TRUNCATE] = "truncate",
> >> @@ -393,12 +402,14 @@ logdump(void)
> >>  				prt("\t******WWWW");
> >>  			break;
> >>  		case OP_READ:
> >> +		case OP_READ_UNCACHED:
> >>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
> >>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> >>  			    lp->args[1]);
> >>  			if (overlap)
> >>  				prt("\t***RRRR***");
> >>  			break;
> >> +		case OP_WRITE_UNCACHED:
> >>  		case OP_WRITE:
> >>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
> >>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> >> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
> >>  }
> >>  
> >>  void
> >> -doread(unsigned offset, unsigned size)
> >> +__doread(unsigned offset, unsigned size, int flags)
> >>  {
> >> -	off_t ret;
> >>  	unsigned iret;
> >>  
> >>  	offset -= offset % readbdy;
> >> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
> >>  			(monitorend == -1 || offset <= monitorend))))))
> >>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
> >>  		    offset, offset + size - 1, size);
> >> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> >> -	if (ret == (off_t)-1) {
> >> -		prterr("doread: lseek");
> >> -		report_failure(140);
> >> -	}
> >> -	iret = fsxread(fd, temp_buf, size, offset);
> >> +	iret = fsxread(fd, temp_buf, size, offset, flags);
> >>  	if (iret != size) {
> >> -		if (iret == -1)
> >> -			prterr("doread: read");
> >> -		else
> >> +		if (iret == -1) {
> >> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> >> +				rwf_uncached = 1;
> > 
> > I assume you meant rwf_uncached = 0 here?
> 
> Indeed, good catch. Haven't tested this on a kernel without RWF_UNCACHED
> yet...
> 
> > If so, check out test_fallocate() and friends to see how various
> > operations are tested for support before the test starts. Following that
> > might clean things up a bit.
> 
> Sure, I can do something like that instead. fsx looks pretty old school
> in its design, was not expecting a static (and single) fd. But since we
> have that, we can do the probe and check. Just a basic read would be
> enough, with RWF_UNCACHED set.
> 
> > Also it's useful to have a CLI option to enable/disable individual
> > features. That tends to be helpful to narrow things down when it does
> > happen to explode and you want to narrow down the cause.
> 
> I can add a -U for "do not use uncached".
> 
> -- 
> Jens Axboe
> 

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 12:39 PM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
>> On 11/12/24 11:44 AM, Brian Foster wrote:
>>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>>>> with.
>>>>>>>>
>>>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>>>> fstests lists.
>>>>>>>
>>>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>>>
>>>>>>
>>>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>>>> fsstress, but bashes the same file with mixed operations and includes
>>>>>> data integrity validation checks as well. It's pretty useful for
>>>>>> uncovering subtle corner case issues or bad interactions..
>>>>>
>>>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>>>
>>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>>>> for both the sync read/write and io_uring paths.
>>>>
>>>
>>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
>>> comments inlined below.
>>>
>>> BTW, I should have also mentioned that fsx is also useful for longer
>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>> is via the various preexisting tests, but I'll occasionally run fsx
>>> directly and let it run overnight or something to get the op count at
>>> least up in the 100 millions or so to have a little more confidence
>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>> something like this. JFYI.
>>
>> Good suggestion, I can leave it running overnight here as well. Since
>> I'm not super familiar with it, what would be a good set of parameters
>> to run it with?
>>
> 
> Most things are on by default, so I'd probably just go with that. -p is
> useful to get occasional status output on how many operations have
> completed and you could consider increasing the max file size with -l,
> but usually I don't use more than a few MB or so if I increase it at
> all.

When you say default, I'd run it without arguments. And then it does
nothing :-)

Not an fs guy, I never run fsx. I run xfstests if I make changes that
may impact the page cache, writeback, or file systems.

IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
specific" ;-)

> Random other thought: I also wonder if uncached I/O should be an
> exclusive mode more similar to like how O_DIRECT or AIO is implemented.
> But I dunno, maybe it doesn't matter that much (or maybe others will
> have opinions on the fstests list).

Should probably exclude it with DIO, as it should not do anything there
anyway. Eg if you ask for DIO, it gets turned off. For some of the other
exclusions, they seem kind of wonky to me. Why can you use libaio and
io_uring at the same time, for example?

io_uring will work just fine with both buffered and direct IO, and it'll
do the right thing with uncached as well. AIO is really a DIO only
thing, not useful for anything else.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Brian Foster]

On Tue, Nov 12, 2024 at 12:45:58PM -0700, Jens Axboe wrote:
> On 11/12/24 12:39 PM, Brian Foster wrote:
> > On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
> >> On 11/12/24 11:44 AM, Brian Foster wrote:
> >>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> >>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
> >>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
> >>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>>>>>> with.
> >>>>>>>>
> >>>>>>>> Looks reasonable to me, but you probably get better reviews on the
> >>>>>>>> fstests lists.
> >>>>>>>
> >>>>>>> I'll send it out once this patchset is a bit closer to integration,
> >>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
> >>>>>>> handy for my testing, found a few issues with this version. So thanks
> >>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>>>>>
> >>>>>>
> >>>>>> fsx support is probably a good idea as well. It's similar in idea to
> >>>>>> fsstress, but bashes the same file with mixed operations and includes
> >>>>>> data integrity validation checks as well. It's pretty useful for
> >>>>>> uncovering subtle corner case issues or bad interactions..
> >>>>>
> >>>>> Indeed, I did that too. Re-running xfstests right now with that too.
> >>>>
> >>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> >>>> for both the sync read/write and io_uring paths.
> >>>>
> >>>
> >>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
> >>> comments inlined below.
> >>>
> >>> BTW, I should have also mentioned that fsx is also useful for longer
> >>> soak testing. I.e., fstests will provide a decent amount of coverage as
> >>> is via the various preexisting tests, but I'll occasionally run fsx
> >>> directly and let it run overnight or something to get the op count at
> >>> least up in the 100 millions or so to have a little more confidence
> >>> there isn't some rare/subtle bug lurking. That might be helpful with
> >>> something like this. JFYI.
> >>
> >> Good suggestion, I can leave it running overnight here as well. Since
> >> I'm not super familiar with it, what would be a good set of parameters
> >> to run it with?
> >>
> > 
> > Most things are on by default, so I'd probably just go with that. -p is
> > useful to get occasional status output on how many operations have
> > completed and you could consider increasing the max file size with -l,
> > but usually I don't use more than a few MB or so if I increase it at
> > all.
> 
> When you say default, I'd run it without arguments. And then it does
> nothing :-)
> 
> Not an fs guy, I never run fsx. I run xfstests if I make changes that
> may impact the page cache, writeback, or file systems.
> 
> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
> specific" ;-)
> 

Heh. In that case I'd just run something like this:

	fsx -p 100000 <file>

... and see how long it survives. It may not necessarily be an uncached
I/O problem if it fails, but depending on how reproducible a failure is,
that's where a cli knob comes in handy.

> > Random other thought: I also wonder if uncached I/O should be an
> > exclusive mode more similar to like how O_DIRECT or AIO is implemented.
> > But I dunno, maybe it doesn't matter that much (or maybe others will
> > have opinions on the fstests list).
> 
> Should probably exclude it with DIO, as it should not do anything there
> anyway. Eg if you ask for DIO, it gets turned off. For some of the other
> exclusions, they seem kind of wonky to me. Why can you use libaio and
> io_uring at the same time, for example?
> 

To your earlier point, if I had to guess it's probably just because it's
grotty test code with sharp edges.

Brian

> io_uring will work just fine with both buffered and direct IO, and it'll
> do the right thing with uncached as well. AIO is really a DIO only
> thing, not useful for anything else.
> 
> -- 
> Jens Axboe
> 

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 1:21 PM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 12:45:58PM -0700, Jens Axboe wrote:
>> On 11/12/24 12:39 PM, Brian Foster wrote:
>>> On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
>>>> On 11/12/24 11:44 AM, Brian Foster wrote:
>>>>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>>>>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>>>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>>>>>> with.
>>>>>>>>>>
>>>>>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>>>>>> fstests lists.
>>>>>>>>>
>>>>>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>>>>>> fsstress, but bashes the same file with mixed operations and includes
>>>>>>>> data integrity validation checks as well. It's pretty useful for
>>>>>>>> uncovering subtle corner case issues or bad interactions..
>>>>>>>
>>>>>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>>>>>
>>>>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>>>>>> for both the sync read/write and io_uring paths.
>>>>>>
>>>>>
>>>>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
>>>>> comments inlined below.
>>>>>
>>>>> BTW, I should have also mentioned that fsx is also useful for longer
>>>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>>>> is via the various preexisting tests, but I'll occasionally run fsx
>>>>> directly and let it run overnight or something to get the op count at
>>>>> least up in the 100 millions or so to have a little more confidence
>>>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>>>> something like this. JFYI.
>>>>
>>>> Good suggestion, I can leave it running overnight here as well. Since
>>>> I'm not super familiar with it, what would be a good set of parameters
>>>> to run it with?
>>>>
>>>
>>> Most things are on by default, so I'd probably just go with that. -p is
>>> useful to get occasional status output on how many operations have
>>> completed and you could consider increasing the max file size with -l,
>>> but usually I don't use more than a few MB or so if I increase it at
>>> all.
>>
>> When you say default, I'd run it without arguments. And then it does
>> nothing :-)
>>
>> Not an fs guy, I never run fsx. I run xfstests if I make changes that
>> may impact the page cache, writeback, or file systems.
>>
>> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
>> specific" ;-)
>>
> 
> Heh. In that case I'd just run something like this:
> 
> 	fsx -p 100000 <file>
> 
> ... and see how long it survives. It may not necessarily be an uncached
> I/O problem if it fails, but depending on how reproducible a failure is,
> that's where a cli knob comes in handy.

OK good, will give that a spin.

>>> Random other thought: I also wonder if uncached I/O should be an
>>> exclusive mode more similar to like how O_DIRECT or AIO is implemented.
>>> But I dunno, maybe it doesn't matter that much (or maybe others will
>>> have opinions on the fstests list).
>>
>> Should probably exclude it with DIO, as it should not do anything there
>> anyway. Eg if you ask for DIO, it gets turned off. For some of the other
>> exclusions, they seem kind of wonky to me. Why can you use libaio and
>> io_uring at the same time, for example?
>>
> 
> To your earlier point, if I had to guess it's probably just because it's
> grotty test code with sharp edges.

Yeah makes sense, unloved.

-- 
Jens Axboe

Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/12/24 1:25 PM, Jens Axboe wrote:
>>>>>> BTW, I should have also mentioned that fsx is also useful for longer
>>>>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>>>>> is via the various preexisting tests, but I'll occasionally run fsx
>>>>>> directly and let it run overnight or something to get the op count at
>>>>>> least up in the 100 millions or so to have a little more confidence
>>>>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>>>>> something like this. JFYI.
>>>>>
>>>>> Good suggestion, I can leave it running overnight here as well. Since
>>>>> I'm not super familiar with it, what would be a good set of parameters
>>>>> to run it with?
>>>>>
>>>>
>>>> Most things are on by default, so I'd probably just go with that. -p is
>>>> useful to get occasional status output on how many operations have
>>>> completed and you could consider increasing the max file size with -l,
>>>> but usually I don't use more than a few MB or so if I increase it at
>>>> all.
>>>
>>> When you say default, I'd run it without arguments. And then it does
>>> nothing :-)
>>>
>>> Not an fs guy, I never run fsx. I run xfstests if I make changes that
>>> may impact the page cache, writeback, or file systems.
>>>
>>> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
>>> specific" ;-)
>>>
>>
>> Heh. In that case I'd just run something like this:
>>
>> 	fsx -p 100000 <file>
>>
>> ... and see how long it survives. It may not necessarily be an uncached
>> I/O problem if it fails, but depending on how reproducible a failure is,
>> that's where a cli knob comes in handy.
> 
> OK good, will give that a spin.

Ran overnight, no issues seen. Just terminated the process. For funsies,
I also added RWF_UNCACHED support to qemu and had the vm booted with
that as well, to get some host side testing too. Everything looks fine.
This is running:

https://git.kernel.dk/cgit/linux/log/?h=buffered-uncached.7

which is the current branch.

-- 
Jens Axboe

Re: [PATCHSET v2 0/15] Uncached buffered IO

[Yuanchu Xie]

On Mon, Nov 11, 2024 at 2:08 PM Yu Zhao <yuzhao@google.com> wrote:
>
> I was under the impression that our engineers took care of that. But
> apparently it's still pending:
> https://lore.kernel.org/linux-man/20230320222057.1976956-1-talumbau@google.com/
>
> Will find someone else to follow up on that.
>

Following up on this thread. Alejandro applied the manpage change for NOREUSE.

https://lore.kernel.org/linux-man/20241120104714.kobevbrggqo7urun@devuan/