Disabling read-ahead makes I/O of large reads small

In porting some application code to Linux, its performance over NFSv3 on Linux is terrible. I'm posting this note to LKML since the problem was actually tracked back to the VFS layer.

The app has a simple database that's accessed over NFS. It always does random I/O, so any read-ahead is a waste. The app uses O_DIRECT which has the side-effect of disabling read-ahead.

On Linux accessing an O_DIRECT opened file over NFS is much akin to disabling its attribute cache causing its attributes to be refetched from the server before each NFS operation. After some thought, given the Linux behavior of O_DIRECT on regular hard disk files to ensure file cache consistency, frustratingly, that's probably the more correct answer to emulate this file system behavior for NFS. At this point, rather than expecting Linux to somehow change to avoid the unnecessary flood of GETATTRs, I thought it best for the app not to just use the O_DIRECT flag on Linux. So I changed the app code and then added a posix_fadvise(2) call to keep read-ahead disabled. When I did that, I ran into an unexpected problem.

Adding the posix_fadvise(..., POSIX_FADV_RANDOM) call sets ra_pages=0. This has a very odd side-effect in the kernel. Once read-ahead is disabled, subsequent calls to read(2) are now done in the kernel via ->readpage() callback doing I/O one page at a time!

Pouring through the code in mm/filemap.c I see that the kernel has commingled read-ahead and plain read implementations. The algorithms have much in common, so I can see why it was done, but it left this anomaly of severely pimping read(2) calls on file descriptors with read-ahead disabled.

For example, with a read(2) of 98K bytes of a file opened with O_DIRECT accessed over NFSv3 with rsize=32768, I see:
=========
V3 ACCESS Call (Reply In 249), FH:0xf3a8e519
V3 ACCESS Reply (Call In 248)
V3 READ Call (Reply In 321), FH:0xf3a8e519 Offset:0 Len:32768 V3 READ Call (Reply In 287), FH:0xf3a8e519 Offset:32768 Len:32768 V3 READ Call (Reply In 356), FH:0xf3a8e519 Offset:65536 Len:32768 V3 READ Reply (Call In 251) Len:32768
V3 READ Reply (Call In 250) Len:32768
V3 READ Reply (Call In 252) Len:32768
=========

I would expect three READs issued of size 32K, and that's exactly what I see.

For the same file without O_DIRECT but with read-ahead disabled (its ra_pages=0), I see:
=========
V3 ACCESS Call (Reply In 167), FH:0xf3a8e519
V3 ACCESS Reply (Call In 166)
V3 READ Call (Reply In 172), FH:0xf3a8e519 Offset:0 Len:4096 V3 READ Reply (Call In 168) Len:4096
V3 READ Call (Reply In 177), FH:0xf3a8e519 Offset:4096 Len:4096 V3 READ Reply (Call In 173) Len:4096
V3 READ Call (Reply In 182), FH:0xf3a8e519 Offset:8192 Len:4096 V3 READ Reply (Call In 178) Len:4096
[... READ Call/Reply pairs repeated another 21 times ...]
=========

Now I see 24 READ calls of 4K each!

A workaround for this kernel problem is to hack the app to do a readahead(2) call prior to the read(2), however, I would think a better approach would be to fix the kernel. I came up with the included patch that once applied restores the expected read(2) behavior. For the latter test case above of a file with read-ahead disabled but now with the patch below applied, I now see:
=========
V3 ACCESS Call (Reply In 1350), FH:0xf3a8e519
V3 ACCESS Reply (Call In 1349)
V3 READ Call (Reply In 1387), FH:0xf3a8e519 Offset:0 Len:32768 V3 READ Call (Reply In 1421), FH:0xf3a8e519 Offset:32768 Len:32768 V3 READ Call (Reply In 1456), FH:0xf3a8e519 Offset:65536 Len:32768 V3 READ Reply (Call In 1351) Len:32768
V3 READ Reply (Call In 1352) Len:32768
V3 READ Reply (Call In 1353) Len:32768
=========

Which is what I would expect -- back to just three 32K READs.

After this change, the overall performance of the application increased by 313%!

I have no idea if my patch is the appropriate fix. I'm well out of my area in this part of the kernel. It solves this one problem, but I have no idea how many boundary cases it doesn't cover or even if it is the right way to go about addressing this issue.

Is this behavior of shorting I/O of read(2) considered a bug? And is this approach for a fix approriate?

Quentin

@@ -1012,9 +1012,13 @@ static void do_generic_file_read(struct 
 find_page:
 		page = find_get_page(mapping, index);
 		if (!page) {
-			page_cache_sync_readahead(mapping,
-					ra, filp,
-					index, last_index - index);
+			if (ra->ra_pages)
+				page_cache_sync_readahead(mapping,
+						ra, filp,
+						index, last_index - index);
+			else
+				force_page_cache_readahead(mapping, filp,
+						index, last_index - index);
 			page = find_get_page(mapping, index);
 			if (unlikely(page == NULL))
 				goto no_cached_page;

My test program used to gather the network traces above:
=========
#define _GNU_SOURCE 1
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

int
main(int argc, char **argv)

{
	char	scratch[32768*3];
	int	lgfd;
	int	cnt;

	//if ( (lgfd = open(argv[1], O_RDWR|O_DIRECT)) == -1 ) {
	if ( (lgfd = open(argv[1], O_RDWR)) == -1 ) {
		fprintf(stderr, "Cannot open '%s'.\n", argv[1]);
		return 1;
	}

	posix_fadvise(lgfd, 0, 0, POSIX_FADV_RANDOM);
	//readahead(lgfd, 0, sizeof(scratch));
	cnt = read(lgfd, scratch, sizeof(scratch));
	printf("Read %d bytes.\n", cnt);
	close(lgfd);

	return 0;
}

=========

Quentin Barnes <[email protected]> writes:

cc fengguang who is Mr.Readahead. The full description+patch is in the archives.

[...]

It sounds like a (performance) bug to me.

-Andi

[email protected] -- Speaking for myself only.

Andi,

Thank you for the CC.

Yes it's a bug. It hit my mind in some early days.. I should be blamed to lose track of it.

Yes, it's reasonable to directly call force_page_cache_readahead() in this case.

However the ra_pages=0 trick in fadvise also asks for fix. We'd better let it set a readahead flag, because ra_pages=0 is used in many other places to really disable the (heuristic|force) readahead. See the second patch's description for more details.

Thanks,
Fengguang

Introduce a readahead flags field and embed the existing mmap_miss in it (to save space).

It will be possible to lose the flags in race conditions, however the impact should be limited.

---
 include/linux/fs.h |   30 +++++++++++++++++++++++++++++-
 mm/filemap.c       |    7 ++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

@@ -889,10 +889,38 @@ struct file_ra_state {
 					   there are only # of pages ahead */
 
 	unsigned int ra_pages;		/* Maximum readahead window */
-	unsigned int mmap_miss;		/* Cache miss stat for mmap accesses */
+	unsigned int flags;
 	loff_t prev_pos;		/* Cache last read() position */
 };
 
+/* low 16 bits: cache miss stat for mmap accesses */
+#define	RA_FLAG_MMAP_MISS		0x0000ffff
+
+/*
+ * Don't do flags++ directly to avoid possible overflow:
+ * the ra fields can be accessed concurrently in a racy way.
+ */
+static inline unsigned int ra_mmap_miss_inc(struct file_ra_state *ra)
+{
+	unsigned int miss = ra->flags & RA_FLAG_MMAP_MISS;
+
+	if (miss < RA_FLAG_MMAP_MISS) {
+		miss++;
+		ra->flags = miss | (ra->flags &~ RA_FLAG_MMAP_MISS);
+	}
+	return miss;
+}
+
+static inline void ra_mmap_miss_dec(struct file_ra_state *ra)
+{
+	unsigned int miss = ra->flags & RA_FLAG_MMAP_MISS;
+
+	if (miss) {
+		miss--;
+		ra->flags = miss | (ra->flags &~ RA_FLAG_MMAP_MISS);
+	}
+}
+
 /*
  * Check if @index falls in the readahead windows.
  */

@@ -1418,14 +1418,12 @@ static void do_sync_mmap_readahead(struc
 		return;
 	}
 
-	if (ra->mmap_miss < INT_MAX)
-		ra->mmap_miss++;
 
 	/*
 	 * Do we miss much more than hit in this file? If so,
 	 * stop bothering with read-ahead. It will only hurt.
 	 */
-	if (ra->mmap_miss > MMAP_LOTSAMISS)
+	if (ra_mmap_miss_inc(ra) > MMAP_LOTSAMISS)
 		return;
 
 	/*

@@ -1455,8 +1453,7 @@ static void do_async_mmap_readahead(stru
 	/* If we don't want any read-ahead, don't bother */
 	if (VM_RandomReadHint(vma))
 		return;
-	if (ra->mmap_miss > 0)
-		ra->mmap_miss--;
+	ra_mmap_miss_dec(ra);
 	if (PageReadahead(page))
 		page_cache_async_readahead(mapping, ra, file,
 					   page, offset, ra->ra_pages);

POSIX_FADV_RANDOM sets ra_pages=0, which leads to poor performance:
a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a readahead flag RA_FLAG_RANDOM for POSIX_FADV_RANDOM.

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

An implementation caveat: the set of RA_FLAG_RANDOM can be lost if the same file descriptor is being concurrent read in another thread. However it's rare event and the possible performance hit is expected to be low.

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

---
 include/linux/fs.h |    2 ++
 mm/fadvise.c       |    4 +++-
 mm/readahead.c     |    6 ++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

@@ -896,6 +896,8 @@ struct file_ra_state {
 /* low 16 bits: cache miss stat for mmap accesses */
 #define	RA_FLAG_MMAP_MISS		0x0000ffff
 
+#define	RA_FLAG_RANDOM			0x00010000	/* POSIX_FADV_RANDOM */
+
 /*
  * Don't do flags++ directly to avoid possible overflow:
  * the ra fields can be accessed concurrently in a racy way.

@@ -77,12 +77,14 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		file->f_ra.flags &= ~RA_FLAG_RANDOM;
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		file->f_ra.flags |= RA_FLAG_RANDOM;
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		file->f_ra.flags &= ~RA_FLAG_RANDOM;
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (ra->flags & RA_FLAG_RANDOM) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

Oh I pretended that the problem don't exist..

To be serious, the race only exist inside a mutithread application, where one single fd is shared between two threads, one is doing fadvise, another doing readahead.

A sane application won't do fadvise(POSIX_FADV_RANDOM) while active reads are going one concurrently: this leads to indeterminate behavior by itself -- from which request the random hint takes effect?

fadvise() shall always be in the same streamline with all reads.

In real workloads, 1% applications may do POSIX_FADV_RANDOM, among which 1% applications may be broken. And if the race does happen, the impact is very small. So I choose to just ignore the race and use non-atomic operations..

Thanks,
Fengguang

OK, when updating the manpages as follows, I feel ashamed to add a sentence like "make sure there are no concurrent reads on the same file descriptor...otherwise your advice will be lost".

So how about add a FMODE_HINT_RANDOM_READ bit to file->f_mode? Modifying it at fadvise() time at least won't disturb the existing open-time-modify-only f_mode bits..

Thanks,
Fengguang

---
 man2/posix_fadvise.2 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

@@ -104,7 +104,8 @@ in POSIX.1-2003 TC1.
 .SH NOTES
 Under Linux, \fBPOSIX_FADV_NORMAL\fP sets the readahead window to the
 default size for the backing device; \fBPOSIX_FADV_SEQUENTIAL\fP doubles
-this size, and \fBPOSIX_FADV_RANDOM\fP disables file readahead entirely.
+this size. \fBPOSIX_FADV_RANDOM\fP ignores the readahead size, and will
+submit IO for the read requests as-is.
 These changes affect the entire file, not just the specified region
 (but other open file handles to the same file are unaffected).
 

Ah it seems file->f_flags (modified under f_lock) is ideal candidate :)

Thanks,
Fengguang

[an updated patch for the previous two patches in this thread]

This fixes inefficient page-by-page reads on POSIX_FADV_RANDOM.

POSIX_FADV_RANDOM used to set ra_pages=0, which leads to poor
performance: a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a flag O_RANDOM_READ for POSIX_FADV_RANDOM. It will be visible to fcntl(F_GETFL).

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

---
 include/asm-generic/fcntl.h |    4 ++++
 mm/fadvise.c                |   10 +++++++++-
 mm/readahead.c              |    6 ++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

@@ -80,6 +80,10 @@
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+#ifndef O_RANDOM_READ
+#define O_RANDOM_READ	010000000
+#endif
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */

@@ -77,12 +77,20 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM_READ;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		spin_lock(&file->f_lock);
+		file->f_flags |= O_RANDOM_READ;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM_READ;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (filp->f_flags & O_RANDOM_READ) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

Just thinking about this again, why don't you put the flag into file->f_mode and the FMODE_* namespace given that we don't want it to be settable from open?

Good idea. To do that without race I would like to add ->f_lock to f_mode modifications at non-open time, like this. What do you think?

---
vfs: take f_lock on modifying f_mode after open time

---
 fs/file_table.c     |    2 ++
 fs/nfsd/nfs4state.c |    2 ++
 2 files changed, 4 insertions(+)

@@ -394,7 +394,9 @@ retry:
 			continue;
 		if (!(f->f_mode & FMODE_WRITE))
 			continue;
+		spin_lock(&f->f_lock);
 		f->f_mode &= ~FMODE_WRITE;
+		spin_unlock(&f->f_lock);
 		if (file_check_writeable(f) != 0)
 			continue;
 		file_release_write(f);

@@ -1998,7 +1998,9 @@ nfs4_file_downgrade(struct file *filp, u
 {
 	if (share_access & NFS4_SHARE_ACCESS_WRITE) {
 		drop_file_write_access(filp);
+		spin_lock(&filp->f_lock);
 		filp->f_mode = (filp->f_mode | FMODE_READ) & ~FMODE_WRITE;
+		spin_unlock(&filp->f_lock);
 	}
 }
 

And the FMODE_RANDOM patch.

Thanks,
Fengguang
---

readahead: introduce FMODE_RANDOM for POSIX_FADV_RANDOM

This fixes inefficient page-by-page reads on POSIX_FADV_RANDOM.

POSIX_FADV_RANDOM used to set ra_pages=0, which leads to poor
performance: a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a flag FMODE_RANDOM for POSIX_FADV_RANDOM.

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

v6: use FMODE_RANDOM (proposed by Christoph Hellwig)
v5: use bit 0200000000; explicitly nuke the O_RANDOM bit in __dentry_open()
(Stephen Rothwell)
v4: resolve bit conflicts with sparc and parisc;
use bit 040000000(=FMODE_NONOTIFY), which will be masked out by __dentry_open(), so that open(O_RANDOM) is disabled (Stephen Rothwell and Christoph Hellwig)
v3: use O_RANDOM to indicate both read/write access pattern as in
posix_fadvise(), although it only takes effect for read() now (proposed by Quentin)
v2: use O_RANDOM_READ to avoid race conditions (pointed out by Andi)

---
 include/linux/fs.h |    3 +++
 mm/fadvise.c       |   10 +++++++++-
 mm/readahead.c     |    6 ++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

@@ -77,12 +77,20 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		spin_lock(&file->f_lock);
+		file->f_flags |= FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~FMODE_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (filp->f_mode & FMODE_RANDOM) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

@@ -90,6 +90,9 @@ struct inodes_stat_t {
 /* File was opened by fanotify and shouldn't generate fanotify events */
 #define FMODE_NONOTIFY		((__force fmode_t)0x1000000)
 
+/* Expect random access pattern */
+#define FMODE_RANDOM		((__force fmode_t)0x1000)
+
 /*
  * The below are the various read and write types that we support. Some of
  * them include behavioral modifiers that send information down to the

This fixes inefficient page-by-page reads on POSIX_FADV_RANDOM.

POSIX_FADV_RANDOM used to set ra_pages=0, which leads to poor
performance: a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a flag O_RANDOM for POSIX_FADV_RANDOM. It will be visible to fcntl(F_GETFL).

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

v3: use O_RANDOM to indicate both read/write access pattern as in
posix_fadvise(), although it only takes effect for read() now (proposed by Quentin)
v2: use O_RANDOM_READ to avoid race conditions (pointed out by Andi)

---
 include/asm-generic/fcntl.h |    4 ++++
 mm/fadvise.c                |   10 +++++++++-
 mm/readahead.c              |    6 ++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

@@ -80,6 +80,10 @@
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+#ifndef O_RANDOM
+#define O_RANDOM	010000000	/* random access pattern hint */
+#endif
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */

@@ -77,12 +77,20 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		spin_lock(&file->f_lock);
+		file->f_flags |= O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (filp->f_flags & O_RANDOM) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

Hi Wu,

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Also when I tried to use this value for O_RSYNC and tested it I could not actually see it getting propagated by the open code.

Eitherway I don't think an O_ value is a good idea for a simple access pattern hint.

Ah thanks!

For now I'll use bit 040000000, which happen to equal to FMODE_NONOTIFY and will be masked out by __dentry_open() in the very beginning.

So there's no way to call open(O_RANDOM).

Hmm, I don't really care adding open(O_RANDOM) support.

O_RANDOM is used in the code mainly because the file_ra_state fields are racy, so the f_lock protected file->f_flags is convenient for saving the random hint bit.

If necessary, we can further mask out O_RANDOM for fcntl(F_GETFL).

Thanks,
Fengguang
---
readahead: introduce O_RANDOM for POSIX_FADV_RANDOM

This fixes inefficient page-by-page reads on POSIX_FADV_RANDOM.

POSIX_FADV_RANDOM used to set ra_pages=0, which leads to poor
performance: a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a flag O_RANDOM for POSIX_FADV_RANDOM. It will be visible to fcntl(F_GETFL), but open(O_RANDOM) is disabled for now.

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

v4: resolve bit conflicts with sparc and parisc;
use bit 040000000(=FMODE_NONOTIFY), which will be masked out by __dentry_open(), so that open(O_RANDOM) is disabled (Stephen Rothwell and Christoph Hellwig)
v3: use O_RANDOM to indicate both read/write access pattern as in
posix_fadvise(), although it only takes effect for read() now (proposed by Quentin)
v2: use O_RANDOM_READ to avoid race conditions (pointed out by Andi)

---
 include/asm-generic/fcntl.h |    8 ++++++++
 mm/fadvise.c                |   10 +++++++++-
 mm/readahead.c              |    6 ++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

@@ -80,6 +80,14 @@
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+/*
+ * don't support open(O_RANDOM) for now:
+ * __dentry_open() will nuke this bit implicitly
+ */
+#ifndef O_RANDOM
+#define O_RANDOM	040000000	/* random access pattern hint */
+#endif
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */

@@ -77,12 +77,20 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		spin_lock(&file->f_lock);
+		file->f_flags |= O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (filp->f_flags & O_RANDOM) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

But also clashes with __O_SYNC on sparc, sorry. I am not sure how we are meant to reasonably choose these values any more ...

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Where is it? I cannot grep find one in arch/. There is one defined in include/asm-generic/fcntl.h:

#ifndef O_SYNC
#define __O_SYNC 04000000
#define O_SYNC (__O_SYNC|O_DSYNC)
#endif

However it has one less '0' :)

Thanks,
Fengguang

Hi Fengguang,

Search for 0x800000 ...

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Thanks, I got it..

Does that mean __O_SYNC would be _accidentally_ masked out in __dentry_open() by the newly introduced fanotify bit?

        f->f_flags = (flags & ~(FMODE_EXEC | FMODE_NONOTIFY));

The above line is added in commit f67cee7b50b004357be383, and can be fixed by the below patch.

Eric, what do you think?

Thanks,
Fengguang
---

 include/asm-generic/fcntl.h |    2 +-
 include/linux/fs.h          |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

@@ -5,7 +5,7 @@
 
 /*
  * FMODE_EXEC is 0x20
- * FMODE_NONOTIFY is 0x800000
+ * FMODE_NONOTIFY is 0x1000000
  * These cannot be used by userspace O_* until internal and external open
  * flags are split.
  * -Eric Paris

@@ -88,7 +88,7 @@ struct inodes_stat_t {
 #define FMODE_NOCMTIME		((__force fmode_t)2048)
 
 /* File was opened by fanotify and shouldn't generate fanotify events */
-#define FMODE_NONOTIFY		((__force fmode_t)8388608)
+#define FMODE_NONOTIFY		((__force fmode_t)0x1000000)
 
 /*
  * The below are the various read and write types that we support. Some of

I'll try to get that patch added tonight/tomorrow (although I'm basically without internet until the 7th).

Al mentioned to me (for a completely unrelated reason) he'd rather see internal and external open flags split, so we don't have that stupidity of masking off FMODE_EXEC and FMODE_NONOTIFY. I'll probably try to work on something like that, after I'm back working. Maybe Al or Christoph would like to actually review this work and tell me everything else they don't like?

-Eric

This fixes inefficient page-by-page reads on POSIX_FADV_RANDOM.

POSIX_FADV_RANDOM used to set ra_pages=0, which leads to poor
performance: a 16K read will be carried out in 4 _sync_ 1-page reads.

In other places, ra_pages==0 means
- it's ramfs/tmpfs/hugetlbfs/sysfs/configfs
- some IO error happened
where multi-page read IO won't help or should be avoided.

POSIX_FADV_RANDOM actually want a different semantics: to disable the *heuristic* readahead algorithm, and to use a dumb one which faithfully submit read IO for whatever application requests.

So introduce a flag O_RANDOM for POSIX_FADV_RANDOM. It will be visible to fcntl(F_GETFL), but open(O_RANDOM) is disabled for now.

Note that the random hint is not likely to help random reads performance noticeably. And it may be too permissive on huge request size (its IO size is not limited by read_ahead_kb).

In Quentin's report (http://lkml.org/lkml/2009/12/24/145), the overall (NFS read) performance of the application increased by 313%!

v5: use bit 0200000000; explicitly nuke the O_RANDOM bit in __dentry_open()
(Stephen Rothwell)
v4: resolve bit conflicts with sparc and parisc;
use bit 040000000(=FMODE_NONOTIFY), which will be masked out by __dentry_open(), so that open(O_RANDOM) is disabled (Stephen Rothwell and Christoph Hellwig)
v3: use O_RANDOM to indicate both read/write access pattern as in
posix_fadvise(), although it only takes effect for read() now (proposed by Quentin)
v2: use O_RANDOM_READ to avoid race conditions (pointed out by Andi)

---
 fs/open.c                   |    2 +-
 include/asm-generic/fcntl.h |    7 +++++++
 mm/fadvise.c                |   10 +++++++++-
 mm/readahead.c              |    6 ++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

@@ -80,6 +80,13 @@
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+/*
+ * usage of open(O_RANDOM) is disabled: __dentry_open() will nuke this bit
+ */
+#ifndef O_RANDOM
+#define O_RANDOM	0200000000	/* random access pattern hint */
+#endif
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */

@@ -77,12 +77,20 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, lof
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
 		file->f_ra.ra_pages = bdi->ra_pages;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_RANDOM:
-		file->f_ra.ra_pages = 0;
+		spin_lock(&file->f_lock);
+		file->f_flags |= O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_SEQUENTIAL:
 		file->f_ra.ra_pages = bdi->ra_pages * 2;
+		spin_lock(&file->f_lock);
+		file->f_flags &= ~O_RANDOM;
+		spin_unlock(&file->f_lock);
 		break;
 	case POSIX_FADV_WILLNEED:
 		if (!mapping->a_ops->readpage) {

@@ -501,6 +501,12 @@ void page_cache_sync_readahead(struct ad
 	if (!ra->ra_pages)
 		return;
 
+	/* be dumb */
+	if (filp->f_flags & O_RANDOM) {
+		force_page_cache_readahead(mapping, filp, offset, req_size);
+		return;
+	}
+
 	/* do read-ahead */
 	ondemand_readahead(mapping, ra, filp, false, offset, req_size);
 }

@@ -862,7 +862,7 @@ static struct file *__dentry_open(struct
 	}
 	ima_counts_get(f);
 
-	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
+	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | O_RANDOM);
 
 	file_ra_state_init(&f->f_ra, f->f_mapping->host->i_mapping);
 

Could you expand on that?

I was surprised by Wu's O_RANDOM approach, but after thinking about it, I liked it. I'm used to seeing (on non-UNIX OSes) a parameter as part of the open syscall that announces to the OS what the app's access strategy through that file descriptor will be for that file. An issue with the current fadvise(2) approach is for random access files it necessitates two syscalls (open plus fadvise) for what could be or should be only one syscall (open).

My guess on your issue is that open(2) should take only flags that are necessary for the open state itself and therefore can't be implemented as a separate and later syscall. I would generally agree with that. There is however already at least two exceptions to that principle, the O_SYNC and O_DIRECT flags. They are access states though. I guess the question is whether to think of the O_RANDOM flag as a "hint" or as an "access strategy".

Quentin

Given that syscall overhead is very minimal, especially since fadvise is only setting some in-memory state and doesn't have to flush cache or anything, I don't see that as a significant reason to consume an O_ flag. Those flags are essentially limited to 32-bit values, or 32-bit applications wouldn't be able to use all of the flags, and we are already into the mid-20's of bits.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Hi, Wu.

Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Minchan,

Yes it will have some problem with shrink_readahead_size_eio(), which want to disable readahead and use ->readpage() when ra_pages==0.

Do you have specific use case in mind? The file systems that set ra_pages=0 seems to don't need readahead, too.

Thanks,
Fengguang

Never mind. It's just out of curiosity. :)

I thought although user disable readahead, we could enhance file I/O with one readpages not multiple readpage if we know the user want to read big contiguous blocks.

But I though it break current readahead off semantics. right?

Thanks for reply about my dumb question, Wu. :)

Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yes, not-break-large-read-into-pages would be good for HD/SSD drives when readahead is disabled.

Currently, ->ra_pages is somehow overloaded in its ==0 case. As you said, it's in fact possible to disable readahead while still limiting read IO size to a non-zero ->ra_pages.

It can be done by applying the ->ra_pages limit to O_RANDOM. This also makes O_RANDOM safer to use:

@@ -497,6 +497,13 @@ void page_cache_sync_readahead(struct ad
struct file_ra_state *ra, struct file *filp, pgoff_t offset, unsigned long req_size)
{
+ /* be dumb */
+ if (filp->f_flags & O_RANDOM) {

+               req_size = clamp_t(unsigned long, req_size, 1, ra->ra_pages);
+               force_page_cache_readahead(mapping, filp, offset, req_size);
+               return;
+       }
+      
        /* no read-ahead */
        if (!ra->ra_pages)
                return;

To make real change, we need an interface for the user to disable whole-partition readahead by setting O_RANDOM instead of ra_pages=0. That would be a hard sell..

You are welcome :)

Thanks,
Fengguang

Okay. I understand.
Thanks, Wu.

Kind regards,
Minchan Kim