NFSD direct I/O read

From: Mike Snitzer <[email protected]>

Use STATX_DIOALIGN and STATX_DIO_READ_ALIGN to get DIO alignment attributes from the underlying filesystem and store them in the associated nfsd_file. This is done when the nfsd_file is first opened for each regular file.

Signed-off-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Reviewed-by NeilBrown <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/filecache.c     | 34 ++++++++++++++++++++++++++++++++++
 fs/nfsd/filecache.h     |  4 ++++
 fs/nfsd/nfsfh.c         |  4 ++++
 fs/nfsd/trace.h         | 27 +++++++++++++++++++++++++++
 include/trace/misc/fs.h | 22 ++++++++++++++++++++++
 5 files changed, 91 insertions(+)

fs/nfsd/filecache.c +34 -0

index 75bc48031c07..78cca0d751ac 100644
@@ -231,6 +231,9 @@ nfsd_file_alloc(struct net *net, struct inode *inode, unsigned char need,
 	refcount_set(&nf->nf_ref, 1);
 	nf->nf_may = need;
 	nf->nf_mark = NULL;
+	nf->nf_dio_mem_align = 0;
+	nf->nf_dio_offset_align = 0;
+	nf->nf_dio_read_offset_align = 0;
 	return nf;
 }
 

@@ -1048,6 +1051,35 @@ nfsd_file_is_cached(struct inode *inode)
 	return ret;
 }
 
+static __be32
+nfsd_file_get_dio_attrs(const struct svc_fh *fhp, struct nfsd_file *nf)
+{
+	struct inode *inode = file_inode(nf->nf_file);
+	struct kstat stat;
+	__be32 status;
+
+	/* Currently only need to get DIO alignment info for regular files */
+	if (!S_ISREG(inode->i_mode))
+		return nfs_ok;
+
+	status = fh_getattr(fhp, &stat);
+	if (status != nfs_ok)
+		return status;
+
+	trace_nfsd_file_get_dio_attrs(inode, &stat);
+
+	if (stat.result_mask & STATX_DIOALIGN) {
+		nf->nf_dio_mem_align = stat.dio_mem_align;
+		nf->nf_dio_offset_align = stat.dio_offset_align;
+	}
+	if (stat.result_mask & STATX_DIO_READ_ALIGN)
+		nf->nf_dio_read_offset_align = stat.dio_read_offset_align;
+	else
+		nf->nf_dio_read_offset_align = nf->nf_dio_offset_align;
+
+	return nfs_ok;
+}
+
 static __be32
 nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
 		     struct svc_cred *cred,

@@ -1166,6 +1198,8 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
 			}
 			status = nfserrno(ret);
 			trace_nfsd_file_open(nf, status);
+			if (status == nfs_ok)
+				status = nfsd_file_get_dio_attrs(fhp, nf);
 		}
 	} else
 		status = nfserr_jukebox;

fs/nfsd/filecache.h +4 -0

index 24ddf60e8434..e3d6ca2b6030 100644
@@ -54,6 +54,10 @@ struct nfsd_file {
 	struct list_head	nf_gc;
 	struct rcu_head		nf_rcu;
 	ktime_t			nf_birthtime;
+
+	u32			nf_dio_mem_align;
+	u32			nf_dio_offset_align;
+	u32			nf_dio_read_offset_align;
 };
 
 int nfsd_file_cache_init(void);

fs/nfsd/nfsfh.c +4 -0

index 3edccc38db42..3eb724ec9566 100644
@@ -696,8 +696,12 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
 		.mnt		= fhp->fh_export->ex_path.mnt,
 		.dentry		= fhp->fh_dentry,
 	};
+	struct inode *inode = d_inode(p.dentry);
 	u32 request_mask = STATX_BASIC_STATS;
 
+	if (S_ISREG(inode->i_mode))
+		request_mask |= (STATX_DIOALIGN | STATX_DIO_READ_ALIGN);
+
 	if (fhp->fh_maxsize == NFS4_FHSIZE)
 		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
 

fs/nfsd/trace.h +27 -0

index a664fdf1161e..6e2c8e2aab10 100644
@@ -1133,6 +1133,33 @@ TRACE_EVENT(nfsd_file_alloc,
 	)
 );
 
+TRACE_EVENT(nfsd_file_get_dio_attrs,
+	TP_PROTO(
+		const struct inode *inode,
+		const struct kstat *stat
+	),
+	TP_ARGS(inode, stat),
+	TP_STRUCT__entry(
+		__field(const void *, inode)
+		__field(unsigned long, mask)
+		__field(u32, mem_align)
+		__field(u32, offset_align)
+		__field(u32, read_offset_align)
+	),
+	TP_fast_assign(
+		__entry->inode = inode;
+		__entry->mask = stat->result_mask;
+		__entry->mem_align = stat->dio_mem_align;
+		__entry->offset_align = stat->dio_offset_align;
+		__entry->read_offset_align = stat->dio_read_offset_align;
+	),
+	TP_printk("inode=%p flags=%s mem_align=%u offset_align=%u read_offset_align=%u",
+		__entry->inode, show_statx_mask(__entry->mask),
+		__entry->mem_align, __entry->offset_align,
+		__entry->read_offset_align
+	)
+);
+
 TRACE_EVENT(nfsd_file_acquire,
 	TP_PROTO(
 		const struct svc_rqst *rqstp,

include/trace/misc/fs.h +22 -0

index 0406ebe2a80a..7ead1c61f0cb 100644
@@ -141,3 +141,25 @@
 		{ ATTR_TIMES_SET,	"TIMES_SET" },	\
 		{ ATTR_TOUCH,		"TOUCH"},	\
 		{ ATTR_DELEG,		"DELEG"})
+
+#define show_statx_mask(flags)					\
+	__print_flags(flags, "|",				\
+		{ STATX_TYPE,		"TYPE" },		\
+		{ STATX_MODE,		"MODE" },		\
+		{ STATX_NLINK,		"NLINK" },		\
+		{ STATX_UID,		"UID" },		\
+		{ STATX_GID,		"GID" },		\
+		{ STATX_ATIME,		"ATIME" },		\
+		{ STATX_MTIME,		"MTIME" },		\
+		{ STATX_CTIME,		"CTIME" },		\
+		{ STATX_INO,		"INO" },		\
+		{ STATX_SIZE,		"SIZE" },		\
+		{ STATX_BLOCKS,		"BLOCKS" },		\
+		{ STATX_BASIC_STATS,	"BASIC_STATS" },	\
+		{ STATX_BTIME,		"BTIME" },		\
+		{ STATX_MNT_ID,		"MNT_ID" },		\
+		{ STATX_DIOALIGN,	"DIOALIGN" },		\
+		{ STATX_MNT_ID_UNIQUE,	"MNT_ID_UNIQUE" },	\
+		{ STATX_SUBVOL,		"SUBVOL" },		\
+		{ STATX_WRITE_ATOMIC,	"WRITE_ATOMIC" },	\
+		{ STATX_DIO_READ_ALIGN,	"DIO_READ_ALIGN" })

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 5 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

Quoted 135 lines · NeilBrown

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

Quoted 7 lines

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

It might be updated while the NFSv4 reply encoder is encoding a COMPOUND. If the size of the RPC reply so far is larger than the xdr_buf's .head, the xdr_stream will be positioned somewhere in the xdr_buf's .pages array.

This is precisely why splice READ can be used only for the first NFSv4 READ operation in a COMPOUND. Subsequent READ operations must use nfsd_iter_read().

Quoted 144 lines · Chuck Lever

Quoted 142 lines · NeilBrown

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

Quoted 8 lines

It might be updated while the NFSv4 reply encoder is encoding a COMPOUND. If the size of the RPC reply so far is larger than the xdr_buf's .head, the xdr_stream will be positioned somewhere in the xdr_buf's .pages array.

This is precisely why splice READ can be used only for the first NFSv4 READ operation in a COMPOUND. Subsequent READ operations must use nfsd_iter_read().

Hmmmm...

nfsd4_encode_readv() calls xdr_reserve_space_vec() passing maxcount from the READ request. The maxcount is passed to xdr_reserve_space() repeatedly (one page at a time) where it is added to xdr->buf->page_len (where xdr is ->rq_res_stream and xdr->buf is rq_res).

So the result is often that rq_res.page_len will be maxcount.

Then nfsd4_encode_readv() calls nfsd_iter_read() which, with this patch, will test rq_res.page_len, which will always be non-zero.
So that isn't what we want.

(after the read, nfsd4_encode_readv() will call xdr_truncate_encode()
which will reduce ->page_len based on how much was read).

Then nfsd4_encode_readv() calls nfsd_iter_read().

I don't think we can use ->page_len to determine if it is safe to use nfsd_direct_read(). The conditions where nfsd_direct_read() is safe are similar to the conditions where nfsd_splice_read() is safe. Maybe we should use similar code.

Quoted 145 lines · NeilBrown

Quoted 144 lines · Chuck Lever

Quoted 142 lines · NeilBrown

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

Quoted 8 lines

Quoted 8 lines

It might be updated while the NFSv4 reply encoder is encoding a COMPOUND. If the size of the RPC reply so far is larger than the xdr_buf's .head, the xdr_stream will be positioned somewhere in the xdr_buf's .pages array.

This is precisely why splice READ can be used only for the first NFSv4 READ operation in a COMPOUND. Subsequent READ operations must use nfsd_iter_read().

Hi Neil,

Quoted 7 lines

Hmmmm...

nfsd4_encode_readv() calls xdr_reserve_space_vec() passing maxcount from the READ request. The maxcount is passed to xdr_reserve_space() repeatedly (one page at a time) where it is added to xdr->buf->page_len (where xdr is ->rq_res_stream and xdr->buf is rq_res).

Quoted 5 lines

So the result is often that rq_res.page_len will be maxcount.

Then nfsd4_encode_readv() calls nfsd_iter_read() which, with this patch, will test rq_res.page_len, which will always be non-zero.
So that isn't what we want.

Not true. (And code inspection only review like this, that then makes incorrect yet strong assertions, is pretty disruptive).

That said, I did follow along with your code inspection, I see your concern (but you do gloss over some code in xdr_get_next_encode_buffer that would be cause for avoidng page_len += nbytes).

I'll add some tracing to pin this down.

Quoted 9 lines

(after the read, nfsd4_encode_readv() will call xdr_truncate_encode()
which will reduce ->page_len based on how much was read).

Then nfsd4_encode_readv() calls nfsd_iter_read().

I don't think we can use ->page_len to determine if it is safe to use nfsd_direct_read(). The conditions where nfsd_direct_read() is safe are similar to the conditions where nfsd_splice_read() is safe. Maybe we should use similar code.

Your takeaway from code inspection is clearly missing something because page_len is 0 on entry to nfsd_iter_read for both TCP and RDMA during my READ testing.

nfs_direct_read is being called when io_cache_read is configured to use NFSD_IO_DIRECT. So !page_len && !base isn't too limiting, it works.

FYI, I've been testing with NFSv3. So nfsd_read -> nfsd_iter_read

The last time I tested NFSv4 was with my DIO READ code that checked the memory alignment with precision (albeit adding more work per IO, which Chuck would like to avoid).

-----Original Message-----
From: Mike Snitzer <[email protected]>
Sent: Wednesday, September 24, 2025 12:56 PM
To: NeilBrown <[email protected]>; Chuck Lever <[email protected]>
Cc: Jeff Layton <[email protected]>; Olga Kornievskaia <[email protected]>; Dai Ngo <[email protected]>; Tom Talpey <[email protected]>; [email protected]; Chuck Lever <[email protected]>
Subject: [RFC PATCH v3 4/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ (v3 and older) [Was: "Re: [PATCH v3 3/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ"]

Quoted 6 lines · Mike Snitzer

FYI, I've been testing with NFSv3. So nfsd_read -> nfsd_iter_read

The last time I tested NFSv4 was with my DIO READ code that checked the memory alignment with precision (albeit adding more work per IO, which Chuck would like to avoid).

We could just enable NFSD_IO_DIRECT for NFS v3 and older. And work out adding support for NFSv4 as follow-on work for next cycle?

This could be folded into Patch 3/3, and its header updated to be clear that NFS v4+ isn't supported yet:

fs/nfsd/vfs.c index 96792f8a8fc5a..459a29f377c2c renamed from fs/nfsd/vfs.c +0 -0

100644

fs/nfsd/vfs.c +0 -4

@@ -1182,10 +1182,6 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp,
 struct svc_fh *fhp,
 	case NFSD_IO_BUFFERED:
 		break;
 	case NFSD_IO_DIRECT:
-		if (nf->nf_dio_read_offset_align &&
-		    !rqstp->rq_res.page_len && !base)
-			return nfsd_direct_read(rqstp, fhp, nf, offset,
-						count, eof);
 		fallthrough;
 	case NFSD_IO_DONTCACHE:

fs/nfsd/vfs.c +7 -1

 		if (file->f_op->fop_flags & FOP_DONTCACHE) @@ -1622,8
+1618,14 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	file = nf->nf_file;
 	if (file->f_op->splice_read && nfsd_read_splice_ok(rqstp))
 		err = nfsd_splice_read(rqstp, fhp, file, offset, count, eof);
-	else
+	else {
+		/* Use vectored reads, for NFS v3 and older */
+		if (nfsd_io_cache_read == NFSD_IO_DIRECT &&
+		    nf->nf_dio_read_offset_align &&
+		    !rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset, count,

eof);

-----Original Message-----
From: Jonathan Flynn
Sent: Wednesday, September 24, 2025 8:56 PM
To: 'Mike Snitzer' <[email protected]>; NeilBrown <[email protected]>; Chuck Lever <[email protected]>
Cc: Jeff Layton <[email protected]>; Olga Kornievskaia <[email protected]>; Dai Ngo <[email protected]>; Tom Talpey <[email protected]>; [email protected]; Chuck Lever <[email protected]>
Subject: RE: [RFC PATCH v3 4/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ (v3 and older) [Was: "Re: [PATCH v3 3/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ"]

Quoted 57 lines

-----Original Message-----
From: Mike Snitzer <[email protected]>
Sent: Wednesday, September 24, 2025 12:56 PM
To: NeilBrown <[email protected]>; Chuck Lever <[email protected]>
Cc: Jeff Layton <[email protected]>; Olga Kornievskaia <[email protected]>; Dai Ngo <[email protected]>; Tom Talpey <[email protected]>; [email protected]; Chuck Lever <[email protected]>
Subject: [RFC PATCH v3 4/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ
(v3 and older) [Was: "Re: [PATCH v3 3/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ"]

Quoted 6 lines · Mike Snitzer

FYI, I've been testing with NFSv3. So nfsd_read -> nfsd_iter_read

The last time I tested NFSv4 was with my DIO READ code that checked the memory alignment with precision (albeit adding more work per IO, which Chuck would like to avoid).

We could just enable NFSD_IO_DIRECT for NFS v3 and older. And work out adding support for NFSv4 as follow-on work for next cycle?

This could be folded into Patch 3/3, and its header updated to be clear that NFS v4+ isn't supported yet:

fs/nfsd/vfs.c index renamed from fs/nfsd/vfs.c +0 -0

96792f8a8fc5a..459a29f377c2c
100644

fs/nfsd/vfs.c +0 -4

@@ -1182,10 +1182,6 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp,
 struct svc_fh *fhp,
 	case NFSD_IO_BUFFERED:
 		break;
 	case NFSD_IO_DIRECT:
-		if (nf->nf_dio_read_offset_align &&
-		    !rqstp->rq_res.page_len && !base)
-			return nfsd_direct_read(rqstp, fhp, nf, offset,
-						count, eof);
 		fallthrough;
 	case NFSD_IO_DONTCACHE:

fs/nfsd/vfs.c +8 -1

 		if (file->f_op->fop_flags & FOP_DONTCACHE) @@ -1622,8
+1618,14 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh
+*fhp,
 	file = nf->nf_file;
 	if (file->f_op->splice_read && nfsd_read_splice_ok(rqstp))
 		err = nfsd_splice_read(rqstp, fhp, file, offset, count, eof);
-	else
+	else {
+		/* Use vectored reads, for NFS v3 and older */
+		if (nfsd_io_cache_read == NFSD_IO_DIRECT &&
+		    nf->nf_dio_read_offset_align &&
+		    !rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset, count,

eof);

fs/nfsd/vfs.c __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1631

Quoted 5 lines

fs/nfsd/vfs.c +1 -0

 		err = nfsd_iter_read(rqstp, fhp, nf, offset, count, 0, eof);
+	}
 
 	nfsd_file_put(nf);
 	trace_nfsd_read_done(rqstp, fhp, offset, *count);

Hello,

I wanted to contribute some reproducible data to the discussion on NFSD_IO_DIRECT. With a background in performance benchmarking, I ran fio- based comparisons of buffered I/O (Mode0) and direct I/O (Mode2). The following write-up includes the problem definition, configuration, procedure, and results.

-------------------------------------------------------------------------------
Problem Definition
The primary motivation for NFSD_IO_DIRECT is to remove page cache contention that reduces overall read and write performance. We have consistently seen substantially better throughput with O_DIRECT than with buffered I/O, largely due to avoiding contention in the page cache. While latency improvements are not the sole goal, they serve as a clear indicator of the performance benefits.

Why dataset is much larger than memory (and why 3.125x)
- Goal: ensure the server cannot satisfy requests from page cache, forcing steady-state disk/network behavior rather than warm-cache artifacts.
- Sizing: dataset ~= 3.125x server RAM. With 1 TB RAM and 32 jobs, this yields ~100 GB/job (total ~= 3.2 TB). The 3.125 factor is a convenience that produces even 100 GB files; exact factor isn’t material as long as dataset >> RAM.
- Effect: Buffered mode under memory pressure shows elevated p95?p99.9 latencies due to page-faults, LRU churn, writeback contention, and reclaim activity. The NFSD_IO_DIRECT read path bypasses these cache dynamics, which is why it reduces tail latency and increases sustained bandwidth when the system is under load.
- Concurrency mapping: 32 jobs/files ensures 2 files per NVMe namespace/share (2 * 16 = 32), balancing load across namespaces and preventing any single namespace from becoming a bottleneck. (This is primarily due to my test environment being designed around pNFS/NFSv4.2 for metadata path but uses NFSv3 for data path)

Aside: Dataset fits in memory (~81% of RAM)
As a control case, we also ran tests where the dataset was only ~832 GB, or ~81.25% of the server’s 1 TB RAM. In this in-memory regime, buffered and direct I/O produced nearly identical results. For reads, mean latency was 1.33 ms in both modes with negligible differences across percentiles. For writes, mean latency was 1.76 ms in both modes with only fractional variation well within measurement noise. Bandwidth was likewise ~97?98
GB/s
for reads and ~72?73 GB/s for writes, regardless of mode.

CPU utilization during this in-memory run further illustrates the dynamics:
- Writes Mode0 ~91% total (38% system, 50% iowait, 2.6% IRQ)
- Writes Mode2 ~79% total (9.4% system, 68.4% iowait, 1.8% IRQ)
- Reads Mode0 ~52% total (50% system, 0% iowait, 1.4% IRQ)
- Reads Mode2 ~72% total (11.5% system, 59% iowait, 1.9% IRQ)

Even though throughput and latency metrics converged, the CPU profiles differ substantially between buffered and direct modes, reflecting where the work is absorbed (system vs iowait). These results confirm that NFSD_IO_DIRECT does not hurt performance when the dataset can be fully cached; its advantages only emerge once datasets exceed memory capacity and page cache contention becomes the limiting factor.

-------------------------------------------------------------------------------
Hardware / Software Configuration
- Data Server: Supermicro SYS-121C-TN10R
* CPU: 2x Intel Xeon Gold 6542Y (24 cores/socket, 2.8 GHz)
* Memory: 1 TB RAM
* Storage: 8x ScaleFlux 7.68TB CSD5000 NVMe SSDs
(each exposing 2 namespaces, 16 total)
* Each namespace formatted with XFS, exported via pNFS v4.2
- Metadata Server: Identical platform with 8x ScaleFlux 7.68TB CSD5000 NVMe SSDs, acting as a pNFS Metadata Server (Hammerspace Anvil)
- Client: Identical platform minus local NVMe
- Data Fabric: 2x ConnectX-7 NICs, 400 GbE with RDMA (RoCE)
- OS: Rocky Linux 9.5
- Kernel: 6.12 baseline with NFSD_IO_DIRECT patches applied
- Protocol under test: NFSv4.2 was used to mount all 16 namespaces under
a
single mount point via pNFS (layouts/flexfiles). However, the underlying storage traffic exercised NFSv3 shares, so effectively the test behavior reflects NFSv3 with NFSv4.2 coordinating access.

-------------------------------------------------------------------------------
Test Procedure
The test methodology was automated using a wrapper script (nfsd_mode_compare_fio.sh). Defaults were used without overrides. The procedure was:
- Compute per-job file size so the aggregate dataset is ~3.125x larger than the memory on the NFS server (default: 1 TB → ~100G per job for 32 jobs).
- Build fio commands with:
* --numjobs=32, ensuring two files per NVMe namespace/share (2 files × 16 namespaces = 32 total)
* --bs=1m, --iodepth=2, --ioengine=libaio
* --direct=1, --runtime=600, --ramp_time=30, --time_based
- Files created in ${MOUNT_POINT} using format testfile.$jobnum.$filenum
- Two threads per file: the second thread starts 50% into the file.
- Pass 1 (mode0): Set remote io_cache_read/io_cache_write to 0, run write then read.
- Pause 60s between runs.
- Pass 2 (mode2): Set remote io_cache_read/io_cache_write to 2, drop
remote
NFS server's page cache, rerun write and read.
- Results captured as JSON and summarized into bandwidth, latency percentiles, and comparison tables.

-------------------------------------------------------------------------------
Results (Executive Summaries)

--- READ Executive Summary ---
Key mode0 mode2
------------------- -------------------- --------------------
Bandwidth (GB/s) 18.51 (+/-0.19) 97.86 (+/-0.01)
Mean Latency (ms) 7.21 (+/-15.30) 1.33 (+/-0.25)
Max Latency (ms) 3113.78 37.40

--- WRITE Executive Summary ---
Key mode0 mode2
------------------- -------------------- --------------------
Bandwidth (GB/s) 34.20 (+/-0.25) 72.70 (+/-0.01)
Mean Latency (ms) 3.84 (+/-6.54) 1.76 (+/-0.47)
Max Latency (ms) 199.07 25.68

CPU utilization during 3.125× dataset runs:
- Writes Mode0 ~83% total (79% system, 3.5% iowait, 0.98% IRQ)
- Writes Mode2 ~79% total (8.9% system, 68.7% iowait, 1.8% IRQ)
- Reads Mode0 ~99.5% total (97.1% system, 1.7% iowait, 0.76% IRQ)
- Reads Mode2 ~72% total (11.8% system, 58.1% iowait, 1.9% IRQ)

The improvement on reads is more than 5x in bandwidth with far lower tail latency. Writes also show significant gains, indicating the direct path is valuable for both workloads.

Here is a link to the full results as well as the scripts used to generate this data, with a README describing how to reproduce the tests:
https://drive.google.com/drive/folders/1EbIAmigAv1KbE37dp2gRh_lCKIBxn2R
M?usp=sharing
(I highly recommend reviewing the detailed results and the latency distributions)

These results support enabling NFSD_IO_DIRECT for NFSv3 now, as Mike suggested. The benefits are clear and measurable. I agree that NFSv4 can be addressed in a follow-on cycle once the alignment/validation work is sorted out.

Happy to rerun with different parameters or workloads if that helps.

Thanks,
Jonathan Flynn

-------------------------------------------------------------------------------
Share file list:
3.125x_memory

    grafana_screenshots
        cpu_nfsv3_server.jpg
        memory_nfsv3_server.jpg
        nvme_nfsv3_server.jpg
    logs
        command_used.out
        detailed_results.txt
        fio_20250924_181954.log
        fio_20250924_181954_read_mode0.json
        fio_20250924_181954_read_mode2.json
        fio_20250924_181954_write_mode0.json
        fio_20250924_181954_write_mode2.json
        what-if.out

0.8125x_memory

    grafana_screenshots
        cpu_nfsv3_server.jpg
        memory_nfsv3_server.jpg
        nvme_nfsv3_server.jpg
    logs
        command_used.out
        detailed_results.txt
        fio_20250924_230010.log
        fio_20250924_230010_read_mode0.json
        fio_20250924_230010_read_mode2.json
        fio_20250924_230010_write_mode0.json
        fio_20250924_230010_write_mode2.json
        what-if.out

analyze_fio_json.py
nfsd_mode_compare_fio.sh
README

Quoted 145 lines · NeilBrown

Quoted 144 lines · Chuck Lever

Quoted 142 lines · NeilBrown

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

Quoted 8 lines

Quoted 8 lines

It might be updated while the NFSv4 reply encoder is encoding a COMPOUND. If the size of the RPC reply so far is larger than the xdr_buf's .head, the xdr_stream will be positioned somewhere in the xdr_buf's .pages array.

This is precisely why splice READ can be used only for the first NFSv4 READ operation in a COMPOUND. Subsequent READ operations must use nfsd_iter_read().

Hi Neil,

Quoted 7 lines

Hmmmm...

nfsd4_encode_readv() calls xdr_reserve_space_vec() passing maxcount from the READ request. The maxcount is passed to xdr_reserve_space() repeatedly (one page at a time) where it is added to xdr->buf->page_len (where xdr is ->rq_res_stream and xdr->buf is rq_res).

Quoted 5 lines

So the result is often that rq_res.page_len will be maxcount.

Then nfsd4_encode_readv() calls nfsd_iter_read() which, with this patch, will test rq_res.page_len, which will always be non-zero.
So that isn't what we want.

Not true. (And code inspection only review like this, that then makes incorrect yet strong assertions, is pretty disruptive).

That said, I did follow along with your code inspection, I see your concern (but you do gloss over some code in xdr_get_next_encode_buffer that would be cause for avoidng page_len += nbytes).

I'll add some tracing to pin this down.

Quoted 5 lines

(after the read, nfsd4_encode_readv() will call xdr_truncate_encode()
which will reduce ->page_len based on how much was read).

Then nfsd4_encode_readv() calls nfsd_iter_read().

Quoted 4 lines

I don't think we can use ->page_len to determine if it is safe to use nfsd_direct_read(). The conditions where nfsd_direct_read() is safe are similar to the conditions where nfsd_splice_read() is safe. Maybe we should use similar code.

Your takeaway from code inspection is clearly missing something because page_len is 0 on entry to nfsd_iter_read for both TCP and RDMA during my READ testing.

nfs_direct_read is being called when io_cache_read is configured to use NFSD_IO_DIRECT. So !page_len && !base isn't too limiting, it works.

Mike

Quoted 164 lines · Mike Snitzer

Quoted 145 lines · NeilBrown

Quoted 144 lines · Chuck Lever

Quoted 142 lines · NeilBrown

Quoted 125 lines · Chuck Lever

Quoted 109 lines · Mike Snitzer

Quoted 93 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Add an experimental option that forces NFS READ operations to use direct I/O instead of reading through the NFS server's page cache.

There are already other layers of caching:
- The page cache on NFS clients
- The block device underlying the exported file system

The server's page cache, in many cases, is unlikely to provide additional benefit. Some benchmarks have demonstrated that the server's page cache is actively detrimental for workloads whose working set is larger than the server's available physical memory.

For instance, on small NFS servers, cached NFS file content can squeeze out local memory consumers. For large sequential workloads, an enormous amount of data flows into and out of the page cache and is consumed by NFS clients exactly once -- caching that data is expensive to do and totally valueless.

For now this is a hidden option that can be enabled on test systems for benchmarking. In the longer term, this option might be enabled persistently or per-export. When the exported file system does not support direct I/O, NFSD falls back to using either DONTCACHE or buffered I/O to fulfill NFS READ requests.

Suggested-by Mike Snitzer <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Chuck Lever <[email protected]>

---
 fs/nfsd/debugfs.c |  2 ++
 fs/nfsd/nfsd.h    |  1 +
 fs/nfsd/trace.h   |  1 +
 fs/nfsd/vfs.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+)

fs/nfsd/debugfs.c +2 -0

index ed2b9e066206..00eb1ecef6ac 100644
@@ -44,6 +44,7 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
  * Contents:
  *   %0: NFS READ will use buffered IO
  *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
+ *   %2: NFS READ will use direct IO
  *
  * This setting takes immediate effect for all NFS versions,
  * all exports, and in all NFSD net namespaces.

@@ -64,6 +65,7 @@ static int nfsd_io_cache_read_set(void *data, u64 val)
 		nfsd_io_cache_read = NFSD_IO_BUFFERED;
 		break;
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		/*
 		 * Must disable splice_read when enabling
 		 * NFSD_IO_DONTCACHE.

fs/nfsd/nfsd.h +1 -0

index ea87b42894dd..bdb60ee1f1a4 100644
@@ -157,6 +157,7 @@ enum {
 	/* Any new NFSD_IO enum value must be added at the end */
 	NFSD_IO_BUFFERED,
 	NFSD_IO_DONTCACHE,
+	NFSD_IO_DIRECT,
 };
 
 extern u64 nfsd_io_cache_read __read_mostly;

fs/nfsd/trace.h +1 -0

index 6e2c8e2aab10..bfd41236aff2 100644
@@ -464,6 +464,7 @@ DEFINE_EVENT(nfsd_io_class, nfsd_##name,	\
 DEFINE_NFSD_IO_EVENT(read_start);
 DEFINE_NFSD_IO_EVENT(read_splice);
 DEFINE_NFSD_IO_EVENT(read_vector);
+DEFINE_NFSD_IO_EVENT(read_direct);
 DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);

fs/nfsd/vfs.c +4 -0

index 35880d3f1326..ddcd812f0761 100644
@@ -1074,6 +1074,82 @@ __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
 }
 
+/*
+ * The byte range of the client's READ request is expanded on both
+ * ends until it meets the underlying file system's direct I/O
+ * alignment requirements. After the internal read is complete, the

fs/nfsd/vfs.c __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1090

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+ * byte range of the NFS READ payload is reduced to the byte range
+ * that was originally requested.
+ *
+ * Note that a direct read can be done only when the xdr_buf
+ * containing the NFS READ reply does not already have contents in
+ * its .pages array. This is due to potentially restrictive
+ * alignment requirements on the read buffer. When .page_len and
+ * @base are zero, the .pages array is guaranteed to be page-
+ * aligned.
+ */

So this ^ comment (and the related conversation with Neil in a different thread) says page_len should be 0 on entry to nfsd_direct_read.

Quoted 14 lines

fs/nfsd/vfs.c __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1190

Quoted 10 lines

fs/nfsd/vfs.c +6 -0

@@ -1106,6 +1182,12 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	switch (nfsd_io_cache_read) {
 	case NFSD_IO_BUFFERED:
 		break;
+	case NFSD_IO_DIRECT:
+		if (nf->nf_dio_read_offset_align &&
+		    rqstp->rq_res.page_len && !base)
+			return nfsd_direct_read(rqstp, fhp, nf, offset,
+						count, eof);
+		fallthrough;

Yet the nfsd_iter_read is only calling nfsd_direct_read() if rqstp->rq_res.page_len is not zero, shouldn't it be !rqstp->rq_res.page_len ?

Quoted 8 lines

Oops, yes. I did this work last week, while out of range of my lab.

Quoted 4 lines

(testing confirms it should be !rqstp->rq_res.page_len)

Hopefully with this fix you can have more confidence in staging this in your nfsd-testing?

I'm waiting only for Neil to send an R-b.

After noticing, like Mike, that the page_len test was inverted I went looking to see where page_len was updated, to be sure that a second READ request would not try to use DIRECT IO.
I can see that nfsd_splice_actor() updates page_len, but I cannot see where it is updated when nfsd_iter_read() is used.

What am I missing?

Quoted 8 lines

Quoted 8 lines

It might be updated while the NFSv4 reply encoder is encoding a COMPOUND. If the size of the RPC reply so far is larger than the xdr_buf's .head, the xdr_stream will be positioned somewhere in the xdr_buf's .pages array.

This is precisely why splice READ can be used only for the first NFSv4 READ operation in a COMPOUND. Subsequent READ operations must use nfsd_iter_read().

Hi Neil,

Quoted 7 lines

Hmmmm...

nfsd4_encode_readv() calls xdr_reserve_space_vec() passing maxcount from the READ request. The maxcount is passed to xdr_reserve_space() repeatedly (one page at a time) where it is added to xdr->buf->page_len (where xdr is ->rq_res_stream and xdr->buf is rq_res).

Quoted 8 lines

Quoted 5 lines

So the result is often that rq_res.page_len will be maxcount.

Then nfsd4_encode_readv() calls nfsd_iter_read() which, with this patch, will test rq_res.page_len, which will always be non-zero.
So that isn't what we want.

Not true. (And code inspection only review like this, that then makes incorrect yet strong assertions, is pretty disruptive).

What am I disrupting exactly?

I'm (implicitly?) asked to review the patch. I do that by inspecting the code. I'm not NAKing the code, I'm saying that I cannot see how it could work. I'd be very happy for someone to explain how it does work. We could then put that text in the commit message.

Quoted 16 lines

That said, I did follow along with your code inspection, I see your concern (but you do gloss over some code in xdr_get_next_encode_buffer that would be cause for avoidng page_len += nbytes).

I'll add some tracing to pin this down.

Quoted 9 lines

(after the read, nfsd4_encode_readv() will call xdr_truncate_encode()
which will reduce ->page_len based on how much was read).

Then nfsd4_encode_readv() calls nfsd_iter_read().

I don't think we can use ->page_len to determine if it is safe to use nfsd_direct_read(). The conditions where nfsd_direct_read() is safe are similar to the conditions where nfsd_splice_read() is safe. Maybe we should use similar code.

Quoted 3 lines

Your takeaway from code inspection is clearly missing something because page_len is 0 on entry to nfsd_iter_read for both TCP and RDMA during my READ testing.

Great. I'm glad it works. But without understanding why it works, I cannot give a Reviewed-By. Maybe it just works by accident (I've seen that happen before). And if you or Chuck cannot explain why it works then we clearly have work to do.

nfsd4_encode_readv() calls xdr_reserve_space_vec() passing maxcount from the READ request. The maxcount is passed to xdr_reserve_space() repeatedly (one page at a time) where it is added to xdr->buf->page_len (where xdr is ->rq_res_stream and xdr->buf is rq_res).

So the result is often that rq_res.page_len will be maxcount.

Then nfsd4_encode_readv() calls nfsd_iter_read() which, with this patch, will test rq_res.page_len, which will always be non-zero.
So that isn't what we want.

(after the read, nfsd4_encode_readv() will call xdr_truncate_encode()
which will reduce ->page_len based on how much was read).

Then nfsd4_encode_readv() calls nfsd_iter_read().

I don't think we can use ->page_len to determine if it is safe to use nfsd_direct_read().

The conditions where nfsd_direct_read() is safe are similar to the conditions where nfsd_splice_read() is safe. Maybe we should use similar code.