Uncached buffered IO

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 <[email protected]>

---
 mm/filemap.c | 18 ++++++++++++++++--
 mm/swap.c    |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

mm/filemap.c +6 -0

index 38dc94b761b7..bd698340ef24 100644
@@ -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,
 			}
 		}

mm/filemap.c +3 -0

+			}
+			folio_put(folio);
+		}
 		folio_batch_init(&fbatch);
 	} while (iov_iter_count(iter) && iocb->ki_pos < isize && !error);
 

mm/swap.c +0 -0

index 835bdf324b76..f2457acae383 100644

mm/swap.c +0 -0

 	if (lru_gen_enabled()) {
 		folio_inc_refs(folio);
 		return;

Signature · 1 line

2.45.2

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

mm/filemap.c +7 -2

@@ -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.

Quoted 9 lines

mm/swap.c +1 -0

index 835bdf324b76..f2457acae383 100644
@@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
  */
 void folio_mark_accessed(struct folio *folio)
 {
+	if (folio_test_uncached(folio))

mm/swap.c static void folio_inc_refs(struct folio *folio) · line 476

Quoted 1 line

mm/swap.c +1 -0

+		return;

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

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

Quoted 2 lines · Jens Axboe

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.

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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.

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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.

Quoted 1 line

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.

Quoted 2 lines · Jens Axboe

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.

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 2 lines

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 :-)

Signature · 1 line

Jens Axboe

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 15 lines · Brian Foster

Quoted 13 lines · Jens Axboe

Quoted 6 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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 :-)

Quoted 4 lines

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.

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 1 line

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.

ltp/fsx.c +7 -0

index 41933354..104910ff 100644
@@ -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;

ltp/fsx.c +5 -6

 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);

ltp/fsx.c +66 -24

+				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;
 }

Signature · 2 lines

-- 
Jens Axboe

Quoted 22 lines · Jens Axboe

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 5 lines

Quoted 1 line

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.

ltp/fsx.c int o_direct; /* -Z */ · line 208

Quoted 7 lines

#define READ 0
#define WRITE 1

ltp/fsx.c +2 -2

-#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. ;)

Quoted 48 lines

ltp/fsx.c +6 -8

@@ -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) {

ltp/fsx.c doread(unsigned offset, unsigned size) · line 835

Quoted 6 lines

ltp/fsx.c +3 -3

-		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.

Quoted 32 lines · Brian Foster

Quoted 22 lines · Jens Axboe

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

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?

Quoted 9 lines

ltp/fsx.c int o_direct; /* -Z */ · line 208

Quoted 7 lines

#define READ 0
#define WRITE 1

ltp/fsx.c +2 -2

-#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 ;-)

Quoted 48 lines

Quoted 48 lines

ltp/fsx.c +6 -8

@@ -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) {

Quoted 8 lines

ltp/fsx.c doread(unsigned offset, unsigned size) · line 835

Quoted 6 lines

ltp/fsx.c +3 -3

-		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...

Quoted 3 lines

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.

Quoted 3 lines

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".

Signature · 1 line

Jens Axboe

Quoted 41 lines · Jens Axboe

Quoted 32 lines · Brian Foster

Quoted 22 lines · Jens Axboe

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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).

Quoted 47 lines · Brian Foster

Quoted 41 lines · Jens Axboe

Quoted 32 lines · Brian Foster

Quoted 22 lines · Jens Axboe

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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" ;-)

Quoted 4 lines

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.

Signature · 1 line

Jens Axboe

Quoted 54 lines · Jens Axboe

Quoted 47 lines · Brian Foster

Quoted 41 lines · Jens Axboe

Quoted 32 lines · Brian Foster

Quoted 22 lines · Jens Axboe

Quoted 21 lines · Jens Axboe

Quoted 19 lines · Brian Foster

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Jens Axboe

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

Quoted 8 lines

Quoted 2 lines

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..

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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.

Quoted 9 lines

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.

Quoted 5 lines

Quoted 4 lines

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).

Quoted 5 lines

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.

Quoted 31 lines

Quoted 19 lines

Quoted 13 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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.

Quoted 9 lines

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:

Quoted 5 lines

	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.

Quoted 16 lines · Kirill A. Shutemov

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

mm/filemap.c +7 -2

@@ -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?

Quoted 5 lines

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.

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Kirill A. Shutemov

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

mm/filemap.c +7 -2

@@ -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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 21 lines · Kirill A. Shutemov

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Kirill A. Shutemov

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

mm/filemap.c +7 -2

@@ -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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 47 lines · Jens Axboe

Quoted 21 lines · Kirill A. Shutemov

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Kirill A. Shutemov

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

mm/filemap.c +7 -2

@@ -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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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);
	}

Quoted 1 line

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().

Quoted 48 lines · Kirill A. Shutemov

Quoted 47 lines · Jens Axboe

Quoted 21 lines · Kirill A. Shutemov

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Kirill A. Shutemov

Review ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2721

Quoted 13 lines

(quoted diff) +7 -2

@@ -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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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);
	}

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

Putting the check in __filemap_get_folio() sounds reasonable.

OK will do.

Quoted 2 lines

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

Thanks!

Quoted 2 lines

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.

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 <[email protected]>

---
 mm/filemap.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

mm/filemap.c +2 -0

index bd698340ef24..efd02b047541 100644
@@ -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

mm/filemap.c +5 -0

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

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.

Quoted 4 lines · 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.

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.

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.

Quoted 5 lines · Jens Axboe

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.

Quoted 5 lines · Jens Axboe

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.

Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:

Quoted 1 line · Yu Zhao

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/[email protected]/

Will find someone else to follow up on that.