NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

The client specifies with the stable parameter the method of how the data is to be processed by the server. If stable is FILE_SYNC4, the server MUST commit the data written plus all file system metadata to stable storage before returning results. This corresponds to the NFSv2 protocol semantics. Any other behavior constitutes a protocol violation. If stable is DATA_SYNC4, then the server MUST commit all of the data to stable storage and enough of the metadata to retrieve the data before returning.

From: Chuck Lever <[email protected]>

Mike noted that when NFSD responds to an NFS_FILE_SYNC WRITE, it does not also persist file time stamps. To wit, Section 18.32.3 of RFC 8881 mandates:

Quoted 8 lines

The client specifies with the stable parameter the method of how the data is to be processed by the server. If stable is FILE_SYNC4, the server MUST commit the data written plus all file system metadata to stable storage before returning results. This corresponds to the NFSv2 protocol semantics. Any other behavior constitutes a protocol violation. If stable is DATA_SYNC4, then the server MUST commit all of the data to stable storage and enough of the metadata to retrieve the data before returning.

For many years, NFSD has used a "data sync only" optimization for FILE_SYNC WRITEs, in violation of the above text (and previous incarnations of the NFS standard). File time stamps haven't been persisted as the mandate above requires.

The purpose of this behavior is that, back in the day, file systems on rotational media were too slow to handle writes with time stamp updates. With the advent of UNSTABLE WRITE, the time stamp update is done by the COMMIT, which amortizes the cost of one time stamp update over possibly many WRITE requests.

The impact of this change will be felt only when a client explicitly requests a FILE_SYNC WRITE on a shared file system backed by slow storage. UNSTABLE and DATA_SYNC WRITEs should not be affected.

Reported-by Mike Snitzer <[email protected]>

Closes https://lore.kernel.org/linux-nfs/[email protected]/T/#t open here

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

---
 fs/nfsd/vfs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/nfsd/vfs.c +1 -2

index f537a7b4ee01..5a9a2a69bc08 100644
@@ -1314,8 +1314,14 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		stable = NFS_UNSTABLE;
 	init_sync_kiocb(&kiocb, file);
 	kiocb.ki_pos = offset;
-	if (stable && !fhp->fh_use_wgather)
-		kiocb.ki_flags |= IOCB_DSYNC;
+	if (stable && !fhp->fh_use_wgather) {

For many years, NFSD has used a "data sync only" optimization for FILE_SYNC WRITEs, in violation of the above text (and previous incarnations of the NFS standard). File time stamps haven't been persisted as the mandate above requires.

The purpose of this behavior is that, back in the day, file systems on rotational media were too slow to handle writes with time stamp updates.

The impact of this change will be felt only when a client explicitly requests a FILE_SYNC WRITE on a shared file system backed by slow storage. UNSTABLE and DATA_SYNC WRITEs should not be affected.

fs/nfsd/vfs.c +0 -0

@@ -1314,8 +1314,14 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		stable = NFS_UNSTABLE;
 	init_sync_kiocb(&kiocb, file);
 	kiocb.ki_pos = offset;

This patch prepares for that change by enabling the modified stable_how value to be passed along to NFSD's WRITE reply encoder. No behavior change is expected.

From: Mike Snitzer <[email protected]>

Extract the common code that is to be used in the buffered and dontcache I/O modes. This common code will also be used as the fallback when direct I/O is requested but cannot be used.

From: Mike Snitzer <[email protected]>

If NFSD_IO_DIRECT is used, split any misaligned WRITE into a start, middle and end as needed. The large middle extent is DIO-aligned and the start and/or end are misaligned. Synchronous buffered IO (with preference towards using DONTCACHE) is used for the misaligned extents and O_DIRECT is used for the middle DIO-aligned extent.

nfsd_issue_write_dio() promotes @stable_how to NFS_FILE_SYNC, which allows the client to drop its dirty data and avoid needing an extra COMMIT operation.

If vfs_iocb_iter_write() returns -ENOTBLK, due to its inability to invalidate the page cache on behalf of the DIO WRITE, then nfsd_issue_write_dio() will fall back to using buffered IO.

These changes served as the original starting point for the NFS client's misaligned O_DIRECT support that landed with commit c817248fc831 ("nfs/localio: add proper O_DIRECT support for READ and WRITE"). But NFSD's support is simpler because it currently doesn't use AIO completion.

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

Reviewed-by Jeff Layton <[email protected]>

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

---
 fs/nfsd/debugfs.c |   1 +
 fs/nfsd/trace.h   |   1 +
 fs/nfsd/vfs.c     | 197 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)

fs/nfsd/debugfs.c +1 -0

index 00eb1ecef6ac..7f44689e0a53 100644
@@ -108,6 +108,7 @@ static int nfsd_io_cache_write_set(void *data, u64 val)
 	switch (val) {
 	case NFSD_IO_BUFFERED:
 	case NFSD_IO_DONTCACHE:
+	case NFSD_IO_DIRECT:
 		nfsd_io_cache_write = val;
 		break;
 	default:

fs/nfsd/trace.h +1 -0

index bfd41236aff2..ad74439d0105 100644
@@ -469,6 +469,7 @@ DEFINE_NFSD_IO_EVENT(read_io_done);
 DEFINE_NFSD_IO_EVENT(read_done);
 DEFINE_NFSD_IO_EVENT(write_start);
 DEFINE_NFSD_IO_EVENT(write_opened);
+DEFINE_NFSD_IO_EVENT(write_direct);
 DEFINE_NFSD_IO_EVENT(write_io_done);
 DEFINE_NFSD_IO_EVENT(write_done);
 DEFINE_NFSD_IO_EVENT(commit_start);

fs/nfsd/vfs.c +123 -0

index 6076821bb541..2832a66cda5b 100644
@@ -1254,6 +1254,109 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio {
+	ssize_t	start_len;	/* Length for misaligned first extent */
+	ssize_t	middle_len;	/* Length for DIO-aligned middle extent */
+	ssize_t	end_len;	/* Length for misaligned last extent */
+};
+
+static bool
+nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
+			   struct nfsd_file *nf,
+			   struct nfsd_write_dio *write_dio)
+{
+	const u32 dio_blocksize = nf->nf_dio_offset_align;
+	loff_t start_end, orig_end, middle_end;
+
+	if (unlikely(!nf->nf_dio_mem_align || !dio_blocksize))
+		return false;
+	if (unlikely(dio_blocksize > PAGE_SIZE))
+		return false;
+	if (unlikely(len < dio_blocksize))
+		return false;
+
+	start_end = round_up(offset, dio_blocksize);
+	orig_end = offset + len;
+	middle_end = round_down(orig_end, dio_blocksize);
+
+	write_dio->start_len = start_end - offset;
+	write_dio->middle_len = middle_end - start_end;
+	write_dio->end_len = orig_end - middle_end;
+
+	return true;
+}
+
+static bool
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
+			   unsigned int len_mask)
+{
+	const struct bio_vec *bvec = i->bvec;
+	size_t skip = i->iov_offset;
+	size_t size = i->count;
+
+	if (size & len_mask)
+		return false;
+	do {
+		size_t len = bvec->bv_len;
+
+		if (len > size)
+			len = size;
+		if ((unsigned long)(bvec->bv_offset + skip) & addr_mask)
+			return false;
+		bvec++;
+		size -= len;
+		skip = 0;
+	} while (size);
+
+	return true;
+}
+
+/*
+ * Setup as many as 3 iov_iter based on extents described by @write_dio.
+ * Returns the number of iov_iter that were setup.
+ */
+static int
+nfsd_setup_write_dio_iters(struct iov_iter **iterp, bool *iter_is_dio_aligned,
+			   struct bio_vec *rq_bvec, unsigned int nvecs,
+			   unsigned long cnt, struct nfsd_write_dio *write_dio,
+			   struct nfsd_file *nf)
+{
+	int n_iters = 0;
+	struct iov_iter *iters = *iterp;
+
+	/* Setup misaligned start? */
+	if (write_dio->start_len) {
+		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
+		iters[n_iters].count = write_dio->start_len;
+		iter_is_dio_aligned[n_iters] = false;
+		++n_iters;
+	}
+
+	/* Setup DIO-aligned middle */
+	iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
+	if (write_dio->start_len)
+		iov_iter_advance(&iters[n_iters], write_dio->start_len);
+	iters[n_iters].count -= write_dio->end_len;
+	iter_is_dio_aligned[n_iters] =
+		nfsd_iov_iter_aligned_bvec(&iters[n_iters],
+					   nf->nf_dio_mem_align - 1,
+					   nf->nf_dio_offset_align - 1);
+	if (unlikely(!iter_is_dio_aligned[n_iters]))
+		return 0; /* no DIO-aligned IO possible */
+	++n_iters;
+
+	/* Setup misaligned end? */
+	if (write_dio->end_len) {
+		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
+		iov_iter_advance(&iters[n_iters],
+				 write_dio->start_len + write_dio->middle_len);
+		iter_is_dio_aligned[n_iters] = false;
+		++n_iters;
+	}
+
+	return n_iters;
+}
+
 static int
 nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs,
 		unsigned long *cnt, struct kiocb *kiocb)

@@ -1270,6 +1373,95 @@ nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs,
 	return 0;
 }
 
+static int
+nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
+		     u32 *stable_how, unsigned int nvecs, unsigned long *cnt,
+		     struct kiocb *kiocb, struct nfsd_write_dio *write_dio)
+{
+	struct file *file = nf->nf_file;
+	bool iter_is_dio_aligned[3];
+	struct iov_iter iter_stack[3];
+	struct iov_iter *iter = iter_stack;
+	unsigned int n_iters = 0;
+	unsigned long in_count = *cnt;
+	loff_t in_offset = kiocb->ki_pos;
+	ssize_t host_err;
+
+	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
+					     rqstp->rq_bvec, nvecs, *cnt,
+					     write_dio, nf);
+	if (unlikely(!n_iters))
+		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
+				       cnt, kiocb);

fs/nfsd/vfs.c +64 -0

+
+	*cnt = 0;
+	for (int i = 0; i < n_iters; i++) {
+		if (iter_is_dio_aligned[i])
+			kiocb->ki_flags |= IOCB_DIRECT;
+		else
+			kiocb->ki_flags &= ~IOCB_DIRECT;
+
+		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
+		if (host_err < 0) {
+			/*
+			 * VFS will return -ENOTBLK if DIO WRITE fails to
+			 * invalidate the page cache. Retry using buffered IO.
+			 */
+			if (unlikely(host_err == -ENOTBLK)) {
+				kiocb->ki_flags &= ~IOCB_DIRECT;
+				*cnt = in_count;
+				kiocb->ki_pos = in_offset;
+				return nfsd_iocb_write(file, rqstp->rq_bvec,
+						       nvecs, cnt, kiocb);
+			} else if (unlikely(host_err == -EINVAL)) {
+				struct inode *inode = d_inode(fhp->fh_dentry);
+
+				pr_info_ratelimited("nfsd: Direct I/O alignment failure on %s/%ld\n",
+						    inode->i_sb->s_id, inode->i_ino);
+				host_err = -ESERVERFAULT;
+			}
+			return host_err;
+		}
+		*cnt += host_err;
+		if (host_err < iter[i].count) /* partial write? */
+			break;
+	}
+
+	return 0;
+}
+
+static noinline_for_stack int
+nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		  struct nfsd_file *nf, u32 *stable_how, unsigned int nvecs,
+		  unsigned long *cnt, struct kiocb *kiocb)
+{
+	struct nfsd_write_dio write_dio;
+
+	/*
+	 * Check if IOCB_DONTCACHE can be used when issuing buffered IO;
+	 * if so, set it to preserve intent of NFSD_IO_DIRECT (it will
+	 * be ignored for any DIO issued here).
+	 */
+	if (nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
+		kiocb->ki_flags |= IOCB_DONTCACHE;
+
+	if (nfsd_is_write_dio_possible(kiocb->ki_pos, *cnt, nf, &write_dio))
+		return nfsd_issue_write_dio(rqstp, fhp, nf, stable_how, nvecs,
+					    cnt, kiocb, &write_dio);
+
+	return nfsd_iocb_write(nf->nf_file, rqstp->rq_bvec, nvecs, cnt, kiocb);
+}
+
 /**
  * nfsd_vfs_write - write data to an already-open file
  * @rqstp: RPC execution context

@@ -1346,6 +1538,11 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		nfsd_copy_write_verifier(verf, nn);
 
 	switch (nfsd_io_cache_write) {
+	case NFSD_IO_DIRECT:
+		host_err = nfsd_direct_write(rqstp, fhp, nf, stable_how,
+					     nvecs, cnt, &kiocb);
+		stable = *stable_how;
+		break;
 	case NFSD_IO_DONTCACHE:
 		if (file->f_op->fop_flags & FOP_DONTCACHE)
 			kiocb.ki_flags |= IOCB_DONTCACHE;

Signature · 1 line

2.51.0

fs/nfsd/vfs.c nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs, · line 1405

Quoted 7 lines

fs/nfsd/vfs.c +7 -0

+	/*
+	 * Any buffered IO issued here will be misaligned, use
+	 * sync IO to ensure it has completed before returning.
+	 * Also update @stable_how to avoid need for COMMIT.
+	 */
+	kiocb->ki_flags |= IOCB_DSYNC;
+	*stable_how = NFS_FILE_SYNC;

Patch 6 really should be folded into this patch, I originally submitted my v3 (and you carried it in v4) with both IOCB_DSYNC|IOCB_SSYNC being set, see:
https://lore.kernel.org/linux-nfs/[email protected]/

If you'd like your comment change and removal of parenthesis factored out (to patch 6 or whatever) that's up to you.

From: Chuck Lever <[email protected]>

Clean up: The helpers in the nfsd_direct_write() code path don't set stable_how to anything else but NFS_FILE_SYNC. All data writes in this code path result in immediately durability.

Instead of passing it through the stack of functions, just set it after the call is done.

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

---
 fs/nfsd/vfs.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

fs/nfsd/vfs.c +7 -8

index 2832a66cda5b..cd2c99e450fb 100644
@@ -1374,9 +1374,10 @@ nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs,
 }
 
 static int
-nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
-		     u32 *stable_how, unsigned int nvecs, unsigned long *cnt,
-		     struct kiocb *kiocb, struct nfsd_write_dio *write_dio)
+nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		     struct nfsd_file *nf, unsigned int nvecs,
+		     unsigned long *cnt, struct kiocb *kiocb,
+		     struct nfsd_write_dio *write_dio)
 {
 	struct file *file = nf->nf_file;
 	bool iter_is_dio_aligned[3];

@@ -1399,10 +1400,8 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_fil
 	/*
 	 * Any buffered IO issued here will be misaligned, use
 	 * sync IO to ensure it has completed before returning.
-	 * Also update @stable_how to avoid need for COMMIT.
 	 */
 	kiocb->ki_flags |= IOCB_DSYNC;
-	*stable_how = NFS_FILE_SYNC;
 
 	*cnt = 0;
 	for (int i = 0; i < n_iters; i++) {

@@ -1442,7 +1441,7 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_fil
 
 static noinline_for_stack int
 nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		  struct nfsd_file *nf, u32 *stable_how, unsigned int nvecs,
+		  struct nfsd_file *nf, unsigned int nvecs,
 		  unsigned long *cnt, struct kiocb *kiocb)
 {
 	struct nfsd_write_dio write_dio;

@@ -1456,8 +1455,8 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		kiocb->ki_flags |= IOCB_DONTCACHE;
 
 	if (nfsd_is_write_dio_possible(kiocb->ki_pos, *cnt, nf, &write_dio))
-		return nfsd_issue_write_dio(rqstp, fhp, nf, stable_how, nvecs,
-					    cnt, kiocb, &write_dio);
+		return nfsd_issue_write_dio(rqstp, fhp, nf, nvecs, cnt, kiocb,
+					    &write_dio);
 
 	return nfsd_iocb_write(nf->nf_file, rqstp->rq_bvec, nvecs, cnt, kiocb);
 }

@@ -1539,9 +1538,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	switch (nfsd_io_cache_write) {

Quoted 65 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Clean up: The helpers in the nfsd_direct_write() code path don't set stable_how to anything else but NFS_FILE_SYNC. All data writes in this code path result in immediately durability.

Instead of passing it through the stack of functions, just set it after the call is done.

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

---
 fs/nfsd/vfs.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

fs/nfsd/vfs.c +7 -8

index 2832a66cda5b..cd2c99e450fb 100644
@@ -1374,9 +1374,10 @@ nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs,
 }
 
 static int
-nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
-		     u32 *stable_how, unsigned int nvecs, unsigned long *cnt,
-		     struct kiocb *kiocb, struct nfsd_write_dio *write_dio)
+nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		     struct nfsd_file *nf, unsigned int nvecs,
+		     unsigned long *cnt, struct kiocb *kiocb,
+		     struct nfsd_write_dio *write_dio)
 {
 	struct file *file = nf->nf_file;
 	bool iter_is_dio_aligned[3];

@@ -1399,10 +1400,8 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_fil
 	/*
 	 * Any buffered IO issued here will be misaligned, use
 	 * sync IO to ensure it has completed before returning.
-	 * Also update @stable_how to avoid need for COMMIT.
 	 */
 	kiocb->ki_flags |= IOCB_DSYNC;
-	*stable_how = NFS_FILE_SYNC;
 
 	*cnt = 0;
 	for (int i = 0; i < n_iters; i++) {

@@ -1442,7 +1441,7 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_fil
 
 static noinline_for_stack int
 nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		  struct nfsd_file *nf, u32 *stable_how, unsigned int nvecs,
+		  struct nfsd_file *nf, unsigned int nvecs,
 		  unsigned long *cnt, struct kiocb *kiocb)
 {
 	struct nfsd_write_dio write_dio;

@@ -1456,8 +1455,8 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		kiocb->ki_flags |= IOCB_DONTCACHE;
 
 	if (nfsd_is_write_dio_possible(kiocb->ki_pos, *cnt, nf, &write_dio))
-		return nfsd_issue_write_dio(rqstp, fhp, nf, stable_how, nvecs,
-					    cnt, kiocb, &write_dio);
+		return nfsd_issue_write_dio(rqstp, fhp, nf, nvecs, cnt, kiocb,
+					    &write_dio);
 
 	return nfsd_iocb_write(nf->nf_file, rqstp->rq_bvec, nvecs, cnt, kiocb);
 }

@@ -1539,9 +1538,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 

fs/nfsd/vfs.c nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1546

Quoted 11 lines

fs/nfsd/vfs.c +3 -3

 	switch (nfsd_io_cache_write) {
 	case NFSD_IO_DIRECT:
-		host_err = nfsd_direct_write(rqstp, fhp, nf, stable_how,
-					     nvecs, cnt, &kiocb);
-		stable = *stable_how;
+		host_err = nfsd_direct_write(rqstp, fhp, nf, nvecs, cnt,
+					     &kiocb);
+		stable = *stable_how = NFS_FILE_SYNC;
 		break;
 	case NFSD_IO_DONTCACHE:
 		if (file->f_op->fop_flags & FOP_DONTCACHE)

I assume you're going to squash some of these changes into the original patches?

Reviewed-by Jeff Layton <[email protected]>

From: Chuck Lever <[email protected]>

Clean up: The helpers in the nfsd_direct_write() code path don't set stable_how to anything else but NFS_FILE_SYNC. All data writes in this code path result in immediately durability.

Quoted 5 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Clean up: The helpers in the nfsd_direct_write() code path don't set stable_how to anything else but NFS_FILE_SYNC. All data writes in this code path result in immediately durability.

No doubting the statement of fact for the current patch set, but this is probably a bad idea. Direct I/O still has to flush caches on devices with a volatile write cache (aka consumer grade SSDs), and it still has to commit a transaction to record metadata changes for most writes. Being able to batch these in a commit is a good idea even for direct I/O.

Promoting all NFSD_IO_DIRECT writes to FILE_SYNC was my idea, based on the assumption that IOCB_DIRECT writes to local file systems left nothing to be done by a later commit. My assumption is based on the behavior of O_DIRECT on NFS files.

If that assumption is not true, then I agree there is no technical reason to promote NFSD_IO_DIRECT writes to FILE_SYNC, and I can remove that built-in assumption for v8 of this series.

Quoted 8 lines · Chuck Lever

Promoting all NFSD_IO_DIRECT writes to FILE_SYNC was my idea, based on the assumption that IOCB_DIRECT writes to local file systems left nothing to be done by a later commit. My assumption is based on the behavior of O_DIRECT on NFS files.

If that assumption is not true, then I agree there is no technical reason to promote NFSD_IO_DIRECT writes to FILE_SYNC, and I can remove that built-in assumption for v8 of this series.

It is not true, or rather only true for a tiny subset of use cases (which NFS can't even query a head of time).

For devices that advertise a volatile write cache, commit has to flush that. High-end enough device won't have one, but a lot of devices that people NFS-export do. For pure overwrites the file system could optimize this way by using the FUA flag, and at least the iomap direct I/O code does implementation that optimization for that particular case.

For any write that is not purely an overwrite, commit has to write out the metadata to track the newly allocated blocks. Applications that do overwrite fully allocated blocks typically do that using the O_DSYNC flag to fully benefit from optimizations for that case.

Quoted 4 lines

that. High-end enough device won't have one, but a lot of devices that people NFS-export do. For pure overwrites the file system could optimize this way by using the FUA flag, and at least the iomap direct I/O code does implementation that optimization for that particular case.

NFSD_IO_DIRECT isn't meant to be uniformly better for all types of storage. Any storage that has a volatile write cache is probably best served by existing NFSD default (NFSD_IO_BUFFERED).

The client doesn't control if/when NFSD would make use of O_DIRECT (other than if it sends misaligned IO and NFSD must do what it can to ensure it safely hits stable storage).

In addition, the use of NFSD_IO_DIRECT is intended to allow for systems large _and_ small to get the advantage of lower memory utilization. Buffered IO is one extreme, but even using a model where NFSD were to not impose NFS_FILE_SYNC would create a situation where more memory needed batch IO and then wait for client to send COMMIT.

The current approach of using IOCB_DSYNC|IOCB_SYNC have performed really well on modern NVMe servers.

Quoted 5 lines · Mike Snitzer

Quoted 4 lines

that. High-end enough device won't have one, but a lot of devices that people NFS-export do. For pure overwrites the file system could optimize this way by using the FUA flag, and at least the iomap direct I/O code does implementation that optimization for that particular case.

Quoted 3 lines

NFSD_IO_DIRECT isn't meant to be uniformly better for all types of storage. Any storage that has a volatile write cache is probably best served by existing NFSD default (NFSD_IO_BUFFERED).

That's a very odd claim. Also what does it have to do with the rest of the discussion here?

Quoted 3 lines

The client doesn't control if/when NFSD would make use of O_DIRECT (other than if it sends misaligned IO and NFSD must do what it can to ensure it safely hits stable storage).

Sure.

Quoted 5 lines

In addition, the use of NFSD_IO_DIRECT is intended to allow for systems large _and_ small to get the advantage of lower memory utilization. Buffered IO is one extreme, but even using a model where NFSD were to not impose NFS_FILE_SYNC would create a situation where more memory needed batch IO and then wait for client to send COMMIT.

Why?

Quoted 2 lines

The current approach of using IOCB_DSYNC|IOCB_SYNC have performed really well on modern NVMe servers.

NVMe does not implement a concept called servers.

Quoted 3 lines

Quoted 2 lines

The current approach of using IOCB_DSYNC|IOCB_SYNC have performed really well on modern NVMe servers.

NVMe does not implement a concept called servers.

But you're aware that servers have NVMe devices in them.. that's all I meant. All of these NFSD_IO_DIRECT changes have been developed and tested in modern servers with 8 NVMe, see:
https://www.youtube.com/watch?v=tpPFDu9Nuuw

(NOTE: results covered in this session did _not_ have the benefit of NFSD responding to client with NFS_FILE_SYNC to avoid COMMIT, the ability to do so was discussed at Bakeathon and was acted on with these latest NFSD Direct patchsets).

Quoted 8 lines · Mike Snitzer

Quoted 3 lines

Quoted 2 lines

The current approach of using IOCB_DSYNC|IOCB_SYNC have performed really well on modern NVMe servers.

NVMe does not implement a concept called servers.

But you're aware that servers have NVMe devices in them.. that's all I meant. All of these NFSD_IO_DIRECT changes have been developed and tested in modern servers with 8 NVMe, see:
https://www.youtube.com/watch?v=tpPFDu9Nuuw

Quoted 4 lines

(NOTE: results covered in this session did _not_ have the benefit of NFSD responding to client with NFS_FILE_SYNC to avoid COMMIT, the ability to do so was discussed at Bakeathon and was acted on with these latest NFSD Direct patchsets).

This is why I suspect that leaving direct writes as UNSTABLE, rather than promoting them to FILE_SYNC, is probably not going to make much difference to Jonathan's benchmark results. That's just a guess though.

IOW the memory costs of sticking with UNSTABLE + COMMIT have already been demonstrated, and it's low.

The v8 of this series will go back to that idea, and if you want, you can benchmark it again. If it regresses, we can stick with FILE_SYNC. It's just ones and zeroes, as they say.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Mike Snitzer

Quoted 4 lines

that. High-end enough device won't have one, but a lot of devices that people NFS-export do. For pure overwrites the file system could optimize this way by using the FUA flag, and at least the iomap direct I/O code does implementation that optimization for that particular case.

NFSD_IO_DIRECT isn't meant to be uniformly better for all types of storage. Any storage that has a volatile write cache is probably best served by existing NFSD default (NFSD_IO_BUFFERED).

Quoted 2 lines

That's a very odd claim. Also what does it have to do with the rest of the discussion here?

What's an odd claim? That shitty storage shouldn't be the bad apple that spoils the bunch? You seem to be advocating for requiring additional NFSD resources as part of the NFSD_IO_DIRECT implementation.

But we could make NFSD's behavior tuneable such that it can signal to NFS client NFS_FILE_SYNC or not.

The goal is to walk before expanding to running with other permutations of how to tune NFSD's IO modes.

Quoted 1 line

Why?

Because NFSD will then need to hold the IO until the COMMIT operation. That requires extra NFSD resurces right?

Quoted 5 lines

Quoted 3 lines

The current approach of using IOCB_DSYNC|IOCB_SYNC have performed really well on modern NVMe servers.

NVMe does not implement a concept called servers.

But you're aware that servers have NVMe devices in them.. that's all I meant. All of these NFSD_IO_DIRECT changes have been developed and tested in modern servers with 8 NVMe, see:
https://www.youtube.com/watch?v=tpPFDu9Nuuw

(NOTE: results covered in this session did _not_ have the benefit of NFSD responding to client with NFS_FILE_SYNC to avoid COMMIT, the ability to do so was discussed at Bakeathon and was acted on with these latest NFSD Direct patchsets).

From: Chuck Lever <[email protected]>

The NFS specs mandate that an NFS_FILE_SYNC write means that file metadata (eg time stamps) are durable before the server sends the response.

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

---
 fs/nfsd/vfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

fs/nfsd/vfs.c +4 -6

index cd2c99e450fb..74fcb12bf19c 100644
@@ -1397,12 +1397,6 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	trace_nfsd_write_direct(rqstp, fhp, in_offset, in_count);
 
-	/*
-	 * Any buffered IO issued here will be misaligned, use
-	 * sync IO to ensure it has completed before returning.
-	 */
-	kiocb->ki_flags |= IOCB_DSYNC;
-
 	*cnt = 0;
 	for (int i = 0; i < n_iters; i++) {
 		if (iter_is_dio_aligned[i])

@@ -1454,6 +1448,17 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
 		kiocb->ki_flags |= IOCB_DONTCACHE;
 
+	/*
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
+	 * both written data and dirty time stamps.
+	 *

fs/nfsd/vfs.c nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1458

Quoted 8 lines · Chuck Lever

fs/nfsd/vfs.c +8 -0

+	/*
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
+	 * both written data and dirty time stamps.
+	 *
+	 * When falling back to buffered I/O or handling the unaligned
+	 * first and last segments, the data and time stamps must be
+	 * durable before nfsd_vfs_write() returns to its caller, matching
+	 * the behavior of direct I/O.

I still haven't understood why we need for for sync writes with direct I/O. The comments suggest it has something to do with the buffered write fallback, but even for that I don't really understand it. But for pure direct I/O writes that are properly aligned there definitively should be no need. If there is a need for the fallback we really need to explain it, as it's non-obvious and a performance issue.

Quoted 6 lines

I still haven't understood why we need for for sync writes with direct I/O. The comments suggest it has something to do with the buffered write fallback, but even for that I don't really understand it. But for pure direct I/O writes that are properly aligned there definitively should be no need. If there is a need for the fallback we really need to explain it, as it's non-obvious and a performance issue.

The current DIO scheme has the server writing the aligned middle part of the range using direct I/O, but any unaligned parts at the start or end of the WRITE will use buffered I/O.

I don't see a need for sync writes with the DIO parts either, but the unaligned buffered ends need to be synchronous so that we know they hit the platter before we reply with NFS_FILE_SYNC.

From: Chuck Lever <[email protected]>

1. Christoph notes that ENOTBLK is not supposed to leak out of file systems, so it's unlikely or impossible to see that error code here.

2. There are several ways to get EINVAL on a write, and the least likely of those is a dio alignment problem. The warning here would be misleading in those more common cases.

It's unlikely that an administrator can do anything about either of these cases, should they appear on a production system.

The trace_nfsd_write_done event will be able to record these errnos.

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

fs/nfsd/vfs.c +0 -14

index 74fcb12bf19c..b50be92343e3 100644
@@ -1405,26 +1405,8 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 
 		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
-		if (host_err < 0) {
-			/*
-			 * VFS will return -ENOTBLK if DIO WRITE fails to
-			 * invalidate the page cache. Retry using buffered IO.
-			 */
-			if (unlikely(host_err == -ENOTBLK)) {
-				kiocb->ki_flags &= ~IOCB_DIRECT;
-				*cnt = in_count;
-				kiocb->ki_pos = in_offset;
-				return nfsd_iocb_write(file, rqstp->rq_bvec,
-						       nvecs, cnt, kiocb);
-			} else if (unlikely(host_err == -EINVAL)) {
-				struct inode *inode = d_inode(fhp->fh_dentry);
-

From: Chuck Lever <[email protected]>

The current set of in-tree file systems do not support alignments larger than a PAGE, so this check is unnecessary.

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 2 --
 1 file changed, 2 deletions(-)

fs/nfsd/vfs.c +0 -0

index b50be92343e3..465d4d091f3d 100644

From: Chuck Lever <[email protected]>

The current set of in-tree file systems do not support alignments larger than a PAGE, so this check is unnecessary.

Quoted 4 lines · Chuck Lever

From: Chuck Lever <[email protected]>

The current set of in-tree file systems do not support alignments larger than a PAGE, so this check is unnecessary.

XFS does, although your won't find production hardware with > 4k blocks as far as I can tell. The reason to drop this check was to not arbitrarily exclude them for no reason.

Quoted 3 lines

XFS does, although your won't find production hardware with > 4k blocks as far as I can tell. The reason to drop this check was to not arbitrarily exclude them for no reason.

IIRC there is a reason: NFSD_IO_DIRECT writes won't be able to align on larger than a page (for the memory buffers).

But perhaps the
alignment constraint that was removed applied only to the file offset.

Quoted 3 lines

Hey Mike, I'm trying to understand when nfsd_is_write_dio_possible() would return true but nfsd_iov_iter_aligned_bvec() on the middle segment would return false.

It is always due to memory alignment (addr_mask check), never due to logical alignment (len_mask check).

So we could remove the len_mask arg and the 'if (size & len_mask)' check from nfsd_iov_iter_aligned_bvec

From: Chuck Lever <[email protected]>

Mike says:

Quoted 9 lines

Quoted 3 lines

Hey Mike, I'm trying to understand when nfsd_is_write_dio_possible() would return true but nfsd_iov_iter_aligned_bvec() on the middle segment would return false.

It is always due to memory alignment (addr_mask check), never due to logical alignment (len_mask check).

So we could remove the len_mask arg and the 'if (size & len_mask)' check from nfsd_iov_iter_aligned_bvec

Suggested-by Mike Snitzer <[email protected]>

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

---
 fs/nfsd/vfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/nfsd/vfs.c +1 -2

index 465d4d091f3d..f6810630bb65 100644
@@ -1285,15 +1285,12 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 }
 
 static bool
-nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
-			   unsigned int len_mask)
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask)

From: Chuck Lever <[email protected]>

Mike says:

Quoted 9 lines

Quoted 3 lines

Hey Mike, I'm trying to understand when nfsd_is_write_dio_possible() would return true but nfsd_iov_iter_aligned_bvec() on the middle segment would return false.

It is always due to memory alignment (addr_mask check), never due to logical alignment (len_mask check).

So we could remove the len_mask arg and the 'if (size & len_mask)' check from nfsd_iov_iter_aligned_bvec

Suggested-by Mike Snitzer <[email protected]>

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

---
 fs/nfsd/vfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/nfsd/vfs.c +1 -4

index 465d4d091f3d..f6810630bb65 100644
@@ -1285,15 +1285,12 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 }
 
 static bool
-nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
-			   unsigned int len_mask)
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask)
 {
 	const struct bio_vec *bvec = i->bvec;
 	size_t skip = i->iov_offset;
 	size_t size = i->count;
 
-	if (size & len_mask)
-		return false;
 	do {
 		size_t len = bvec->bv_len;
 

Signature · 1 line

2.51.0

Quoted 40 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Mike says:

Quoted 9 lines

Quoted 3 lines

Hey Mike, I'm trying to understand when nfsd_is_write_dio_possible() would return true but nfsd_iov_iter_aligned_bvec() on the middle segment would return false.

It is always due to memory alignment (addr_mask check), never due to logical alignment (len_mask check).

So we could remove the len_mask arg and the 'if (size & len_mask)' check from nfsd_iov_iter_aligned_bvec

Suggested-by Mike Snitzer <[email protected]>

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

---
 fs/nfsd/vfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/nfsd/vfs.c +1 -4

index 465d4d091f3d..f6810630bb65 100644
@@ -1285,15 +1285,12 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 }
 
 static bool
-nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
-			   unsigned int len_mask)
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask)
 {
 	const struct bio_vec *bvec = i->bvec;
 	size_t skip = i->iov_offset;
 	size_t size = i->count;
 
-	if (size & len_mask)
-		return false;
 	do {
 		size_t len = bvec->bv_len;
 

Quoted 4 lines

Signature · 1 line

2.51.0

Just a bisect-ability nit, the call to nfsd_iov_iter_aligned_bvec() needs to remove the len_mask arg.

Otherwise:

Reviewed-by Mike Snitzer <[email protected]>

From: Chuck Lever <[email protected]>

Clean up: Keep the specifics of the alignment checking inside of nfsd_iov_iter_aligned_bvec(). Move the calculations of the alignment parameters into the function.

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

---
 fs/nfsd/vfs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

fs/nfsd/vfs.c +2 -1

index f6810630bb65..e7c3458bd178 100644
@@ -1285,8 +1285,9 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 }
 
 static bool
-nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask)
+nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 {
+	unsigned int addr_mask = nf->nf_dio_mem_align - 1;
 	const struct bio_vec *bvec = i->bvec;
 	size_t skip = i->iov_offset;
 	size_t size = i->count;

From: Chuck Lever <[email protected]>

Prepare for moving more common arguments into the shared per-request structure.

First step is to move the target nfsd_file into that structure, as it needs to be available in several functions.

As a clean-up, adopt the common naming of a structure that carries the arguments for a number of functions.

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

---
 fs/nfsd/vfs.c | 61 ++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

fs/nfsd/vfs.c +26 -29

index e7c3458bd178..429f5fc61ead 100644
@@ -1254,21 +1254,22 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
-struct nfsd_write_dio {
+struct nfsd_write_dio_args {
+	struct nfsd_file		*nf;
+
 	ssize_t	start_len;	/* Length for misaligned first extent */
 	ssize_t	middle_len;	/* Length for DIO-aligned middle extent */
 	ssize_t	end_len;	/* Length for misaligned last extent */
 };
 
 static bool
-nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
-			   struct nfsd_file *nf,
-			   struct nfsd_write_dio *write_dio)
+nfsd_is_write_dio_possible(struct nfsd_write_dio_args *args, loff_t offset,
+			   unsigned long len)
 {
-	const u32 dio_blocksize = nf->nf_dio_offset_align;
+	const u32 dio_blocksize = args->nf->nf_dio_offset_align;
 	loff_t start_end, orig_end, middle_end;
 
-	if (unlikely(!nf->nf_dio_mem_align || !dio_blocksize))
+	if (unlikely(!args->nf->nf_dio_mem_align || !dio_blocksize))
 		return false;
 	if (unlikely(len < dio_blocksize))
 		return false;

@@ -1277,9 +1278,9 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 	orig_end = offset + len;
 	middle_end = round_down(orig_end, dio_blocksize);
 
-	write_dio->start_len = start_end - offset;
-	write_dio->middle_len = middle_end - start_end;
-	write_dio->end_len = orig_end - middle_end;
+	args->start_len = start_end - offset;
+	args->middle_len = middle_end - start_end;
+	args->end_len = orig_end - middle_end;
 
 	return true;
 }

@@ -1314,36 +1315,35 @@ nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 static int
 nfsd_setup_write_dio_iters(struct iov_iter **iterp, bool *iter_is_dio_aligned,
 			   struct bio_vec *rq_bvec, unsigned int nvecs,
-			   unsigned long cnt, struct nfsd_write_dio *write_dio,
-			   struct nfsd_file *nf)
+			   unsigned long cnt, struct nfsd_write_dio_args *args)
 {
 	int n_iters = 0;
 	struct iov_iter *iters = *iterp;
 
 	/* Setup misaligned start? */
-	if (write_dio->start_len) {
+	if (args->start_len) {
 		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-		iters[n_iters].count = write_dio->start_len;
+		iters[n_iters].count = args->start_len;
 		iter_is_dio_aligned[n_iters] = false;
 		++n_iters;
 	}
 
 	/* Setup DIO-aligned middle */
 	iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-	if (write_dio->start_len)
-		iov_iter_advance(&iters[n_iters], write_dio->start_len);
-	iters[n_iters].count -= write_dio->end_len;
+	if (args->start_len)
+		iov_iter_advance(&iters[n_iters], args->start_len);
+	iters[n_iters].count -= args->end_len;
 	iter_is_dio_aligned[n_iters] =
-		nfsd_iov_iter_aligned_bvec(nf, &iters[n_iters]);
+		nfsd_iov_iter_aligned_bvec(args->nf, &iters[n_iters]);
 	if (unlikely(!iter_is_dio_aligned[n_iters]))
 		return 0; /* no DIO-aligned IO possible */
 	++n_iters;
 
 	/* Setup misaligned end? */
-	if (write_dio->end_len) {
+	if (args->end_len) {
 		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
 		iov_iter_advance(&iters[n_iters],
-				 write_dio->start_len + write_dio->middle_len);
+				 args->start_len + args->middle_len);
 		iter_is_dio_aligned[n_iters] = false;
 		++n_iters;
 	}

@@ -1369,11 +1369,10 @@ nfsd_iocb_write(struct file *file, struct bio_vec *bvec, unsigned int nvecs,
 
 static int
 nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		     struct nfsd_file *nf, unsigned int nvecs,
-		     unsigned long *cnt, struct kiocb *kiocb,
-		     struct nfsd_write_dio *write_dio)
+		     struct nfsd_write_dio_args *args, struct kiocb *kiocb,
+		     unsigned int nvecs, unsigned long *cnt)
 {
-	struct file *file = nf->nf_file;
+	struct file *file = args->nf->nf_file;
 	bool iter_is_dio_aligned[3];
 	struct iov_iter iter_stack[3];
 	struct iov_iter *iter = iter_stack;

@@ -1384,7 +1383,7 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
 					     rqstp->rq_bvec, nvecs, *cnt,
-					     write_dio, nf);
+					     args);
 	if (unlikely(!n_iters))
 		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
 				       cnt, kiocb);

@@ -1414,14 +1413,15 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  struct nfsd_file *nf, unsigned int nvecs,
 		  unsigned long *cnt, struct kiocb *kiocb)
 {
-	struct nfsd_write_dio write_dio;
+	struct file *file = nf->nf_file;
+	struct nfsd_write_dio_args args;
 
 	/*
 	 * Check if IOCB_DONTCACHE can be used when issuing buffered IO;
 	 * if so, set it to preserve intent of NFSD_IO_DIRECT (it will
 	 * be ignored for any DIO issued here).
 	 */
-	if (nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
+	if (file->f_op->fop_flags & FOP_DONTCACHE)
 		kiocb->ki_flags |= IOCB_DONTCACHE;
 
 	/*

@@ -1435,11 +1435,12 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	 */
 	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
 
-	if (nfsd_is_write_dio_possible(kiocb->ki_pos, *cnt, nf, &write_dio))
-		return nfsd_issue_write_dio(rqstp, fhp, nf, nvecs, cnt, kiocb,
-					    &write_dio);

From: Chuck Lever <[email protected]>

Prepare for moving more common arguments into the shared per-request structure.

First step is to move the target nfsd_file into that structure, as it needs to be available in several functions.

As a clean-up, adopt the common naming of a structure that carries the arguments for a number of functions.

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

From: Chuck Lever <[email protected]>

Passing iter arrays by reference is a little risky. Instead, pass a struct with a fixed-size array so bounds checking can be done.

Name each item in the array a "segment", as the term "extent" generally refers to a set of blocks on storage, not to a buffer. Each segment is processed via a single vfs_iocb_iter_write() call, and is either IOCB_DIRECT or buffered.

Introduce a segment constructor function so each segment is initialized identically.

Each segment has its own length. The loop that iterates over the segment array can simply skip over the segments of zero length. A count of segments is not needed.

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 121 ++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 64 deletions(-)

fs/nfsd/vfs.c +57 -64

index 429f5fc61ead..b7f217aa4994 100644
@@ -1254,12 +1254,15 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio_seg {
+	struct iov_iter			iter;
+	size_t				len;
+	bool				use_dio;
+};
+
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
-
-	ssize_t	start_len;	/* Length for misaligned first extent */
-	ssize_t	middle_len;	/* Length for DIO-aligned middle extent */
-	ssize_t	end_len;	/* Length for misaligned last extent */
+	struct nfsd_write_dio_seg	segment[3];
 };
 
 static bool

@@ -1267,21 +1270,19 @@ nfsd_is_write_dio_possible(struct nfsd_write_dio_args *args, loff_t offset,
 			   unsigned long len)
 {
 	const u32 dio_blocksize = args->nf->nf_dio_offset_align;
-	loff_t start_end, orig_end, middle_end;
+	loff_t first_end, orig_end, middle_end;
 
 	if (unlikely(!args->nf->nf_dio_mem_align || !dio_blocksize))
 		return false;
 	if (unlikely(len < dio_blocksize))
 		return false;
 
-	start_end = round_up(offset, dio_blocksize);
+	first_end = round_up(offset, dio_blocksize);
 	orig_end = offset + len;
 	middle_end = round_down(orig_end, dio_blocksize);
-
-	args->start_len = start_end - offset;
-	args->middle_len = middle_end - start_end;
-	args->end_len = orig_end - middle_end;
-
+	args->segment[0].len = first_end - offset;	/* first segment */
+	args->segment[1].len = middle_end - first_end;	/* middle segment */
+	args->segment[2].len = orig_end - middle_end;	/* last segment */
 	return true;
 }
 

@@ -1308,47 +1309,42 @@ nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 	return true;
 }
 
-/*
- * Setup as many as 3 iov_iter based on extents described by @write_dio.
- * Returns the number of iov_iter that were setup.
- */
-static int
-nfsd_setup_write_dio_iters(struct iov_iter **iterp, bool *iter_is_dio_aligned,
-			   struct bio_vec *rq_bvec, unsigned int nvecs,
-			   unsigned long cnt, struct nfsd_write_dio_args *args)
+static void
+nfsd_setup_write_dio_seg(struct nfsd_write_dio_seg *segment,
+			 struct bio_vec *bvec, unsigned int nvecs,
+			 unsigned long total, size_t start)
 {
-	int n_iters = 0;
-	struct iov_iter *iters = *iterp;
+	iov_iter_bvec(&segment->iter, ITER_SOURCE, bvec, nvecs, total);
+	if (start)
+		iov_iter_advance(&segment->iter, start);
+	iov_iter_truncate(&segment->iter, segment->len);
+	segment->use_dio = false;
+}
 
-	/* Setup misaligned start? */
-	if (args->start_len) {
-		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-		iters[n_iters].count = args->start_len;
-		iter_is_dio_aligned[n_iters] = false;
-		++n_iters;
-	}
+static bool
+nfsd_setup_write_dio_iters(struct nfsd_write_dio_args *args,
+			   struct bio_vec *bvec, unsigned int nvecs,
+			   unsigned long total)
+{
+	/* first segment */
+	if (args->segment[0].len)
+		nfsd_setup_write_dio_seg(&args->segment[0],
+					 bvec, nvecs, total, 0);
 
-	/* Setup DIO-aligned middle */
-	iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-	if (args->start_len)
-		iov_iter_advance(&iters[n_iters], args->start_len);
-	iters[n_iters].count -= args->end_len;
-	iter_is_dio_aligned[n_iters] =
-		nfsd_iov_iter_aligned_bvec(args->nf, &iters[n_iters]);
-	if (unlikely(!iter_is_dio_aligned[n_iters]))
-		return 0; /* no DIO-aligned IO possible */
-	++n_iters;
+	/* middle segment */
+	nfsd_setup_write_dio_seg(&args->segment[1], bvec, nvecs, total,
+				 args->segment[0].len);
+	if (!nfsd_iov_iter_aligned_bvec(args->nf, &args->segment[1].iter))
+		return false; /* no DIO-aligned IO possible */
+	args->segment[1].use_dio = true;
 
-	/* Setup misaligned end? */
-	if (args->end_len) {
-		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-		iov_iter_advance(&iters[n_iters],
-				 args->start_len + args->middle_len);
-		iter_is_dio_aligned[n_iters] = false;
-		++n_iters;
-	}
+	/* last segment */
+	if (args->segment[2].len)
+		nfsd_setup_write_dio_seg(&args->segment[2], bvec, nvecs,
+					 total, args->segment[0].len +
+					 args->segment[1].len);
 
-	return n_iters;
+	return true;
 }
 
 static int

@@ -1373,36 +1369,33 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		     unsigned int nvecs, unsigned long *cnt)
 {
 	struct file *file = args->nf->nf_file;
-	bool iter_is_dio_aligned[3];
-	struct iov_iter iter_stack[3];
-	struct iov_iter *iter = iter_stack;
-	unsigned int n_iters = 0;
-	unsigned long in_count = *cnt;
-	loff_t in_offset = kiocb->ki_pos;
+	struct nfsd_write_dio_seg *segment;
 	ssize_t host_err;
+	size_t i;
 
-	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
-					     rqstp->rq_bvec, nvecs, *cnt,
-					     args);
-	if (unlikely(!n_iters))
+	if (!nfsd_setup_write_dio_iters(args, rqstp->rq_bvec, nvecs, *cnt))
 		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
 				       cnt, kiocb);
 
-	trace_nfsd_write_direct(rqstp, fhp, in_offset, in_count);
-
 	*cnt = 0;
-	for (int i = 0; i < n_iters; i++) {
-		if (iter_is_dio_aligned[i])
+	segment = args->segment;
+	for (i = 0; i < ARRAY_SIZE(args->segment); i++) {
+		if (segment->len == 0)
+			continue;
+		if (segment->use_dio) {
 			kiocb->ki_flags |= IOCB_DIRECT;
-		else
+			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
+						segment->len);
+		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 
-		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
+		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
 		if (host_err < 0)
 			return host_err;
 		*cnt += host_err;
-		if (host_err < iter[i].count) /* partial write? */
+		if (host_err < segment->iter.count)
 			break;
+		++segment;
 	}
 
 	return 0;

Quoted 13 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Passing iter arrays by reference is a little risky. Instead, pass a struct with a fixed-size array so bounds checking can be done.

Name each item in the array a "segment", as the term "extent" generally refers to a set of blocks on storage, not to a buffer. Each segment is processed via a single vfs_iocb_iter_write() call, and is either IOCB_DIRECT or buffered.

Introduce a segment constructor function so each segment is initialized identically.

Quoted 4 lines

Each segment has its own length. The loop that iterates over the segment array can simply skip over the segments of zero length. A count of segments is not needed.

True, but it's easy to get that sort of accounting wrong, and if we do we're looking at a buffer overrun.

Maybe it'd be reasonable to keep a
number of segments in this struct too, if only for defensive coding reasons, and to enable proper bounds checking?

Quoted 187 lines

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 121 ++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 64 deletions(-)

fs/nfsd/vfs.c +57 -64

index 429f5fc61ead..b7f217aa4994 100644
@@ -1254,12 +1254,15 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio_seg {
+	struct iov_iter			iter;
+	size_t				len;
+	bool				use_dio;
+};
+
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
-
-	ssize_t	start_len;	/* Length for misaligned first extent */
-	ssize_t	middle_len;	/* Length for DIO-aligned middle extent */
-	ssize_t	end_len;	/* Length for misaligned last extent */
+	struct nfsd_write_dio_seg	segment[3];
 };
 
 static bool

@@ -1267,21 +1270,19 @@ nfsd_is_write_dio_possible(struct nfsd_write_dio_args *args, loff_t offset,
 			   unsigned long len)
 {
 	const u32 dio_blocksize = args->nf->nf_dio_offset_align;
-	loff_t start_end, orig_end, middle_end;
+	loff_t first_end, orig_end, middle_end;
 
 	if (unlikely(!args->nf->nf_dio_mem_align || !dio_blocksize))
 		return false;
 	if (unlikely(len < dio_blocksize))
 		return false;
 
-	start_end = round_up(offset, dio_blocksize);
+	first_end = round_up(offset, dio_blocksize);
 	orig_end = offset + len;
 	middle_end = round_down(orig_end, dio_blocksize);
-
-	args->start_len = start_end - offset;
-	args->middle_len = middle_end - start_end;
-	args->end_len = orig_end - middle_end;
-
+	args->segment[0].len = first_end - offset;	/* first segment */
+	args->segment[1].len = middle_end - first_end;	/* middle segment */
+	args->segment[2].len = orig_end - middle_end;	/* last segment */
 	return true;
 }
 

@@ -1308,47 +1309,42 @@ nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 	return true;
 }
 
-/*
- * Setup as many as 3 iov_iter based on extents described by @write_dio.
- * Returns the number of iov_iter that were setup.
- */
-static int
-nfsd_setup_write_dio_iters(struct iov_iter **iterp, bool *iter_is_dio_aligned,
-			   struct bio_vec *rq_bvec, unsigned int nvecs,
-			   unsigned long cnt, struct nfsd_write_dio_args *args)
+static void
+nfsd_setup_write_dio_seg(struct nfsd_write_dio_seg *segment,
+			 struct bio_vec *bvec, unsigned int nvecs,
+			 unsigned long total, size_t start)
 {
-	int n_iters = 0;
-	struct iov_iter *iters = *iterp;
+	iov_iter_bvec(&segment->iter, ITER_SOURCE, bvec, nvecs, total);
+	if (start)
+		iov_iter_advance(&segment->iter, start);
+	iov_iter_truncate(&segment->iter, segment->len);
+	segment->use_dio = false;
+}
 
-	/* Setup misaligned start? */
-	if (args->start_len) {
-		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-		iters[n_iters].count = args->start_len;
-		iter_is_dio_aligned[n_iters] = false;
-		++n_iters;
-	}
+static bool
+nfsd_setup_write_dio_iters(struct nfsd_write_dio_args *args,
+			   struct bio_vec *bvec, unsigned int nvecs,
+			   unsigned long total)
+{
+	/* first segment */
+	if (args->segment[0].len)
+		nfsd_setup_write_dio_seg(&args->segment[0],
+					 bvec, nvecs, total, 0);
 
-	/* Setup DIO-aligned middle */
-	iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-	if (args->start_len)
-		iov_iter_advance(&iters[n_iters], args->start_len);
-	iters[n_iters].count -= args->end_len;
-	iter_is_dio_aligned[n_iters] =
-		nfsd_iov_iter_aligned_bvec(args->nf, &iters[n_iters]);
-	if (unlikely(!iter_is_dio_aligned[n_iters]))
-		return 0; /* no DIO-aligned IO possible */
-	++n_iters;
+	/* middle segment */
+	nfsd_setup_write_dio_seg(&args->segment[1], bvec, nvecs, total,
+				 args->segment[0].len);
+	if (!nfsd_iov_iter_aligned_bvec(args->nf, &args->segment[1].iter))
+		return false; /* no DIO-aligned IO possible */
+	args->segment[1].use_dio = true;
 
-	/* Setup misaligned end? */
-	if (args->end_len) {
-		iov_iter_bvec(&iters[n_iters], ITER_SOURCE, rq_bvec, nvecs, cnt);
-		iov_iter_advance(&iters[n_iters],
-				 args->start_len + args->middle_len);
-		iter_is_dio_aligned[n_iters] = false;
-		++n_iters;
-	}
+	/* last segment */
+	if (args->segment[2].len)
+		nfsd_setup_write_dio_seg(&args->segment[2], bvec, nvecs,
+					 total, args->segment[0].len +
+					 args->segment[1].len);
 
-	return n_iters;
+	return true;
 }
 
 static int

@@ -1373,36 +1369,33 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		     unsigned int nvecs, unsigned long *cnt)
 {
 	struct file *file = args->nf->nf_file;
-	bool iter_is_dio_aligned[3];
-	struct iov_iter iter_stack[3];
-	struct iov_iter *iter = iter_stack;
-	unsigned int n_iters = 0;
-	unsigned long in_count = *cnt;
-	loff_t in_offset = kiocb->ki_pos;
+	struct nfsd_write_dio_seg *segment;
 	ssize_t host_err;
+	size_t i;
 
-	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
-					     rqstp->rq_bvec, nvecs, *cnt,
-					     args);
-	if (unlikely(!n_iters))
+	if (!nfsd_setup_write_dio_iters(args, rqstp->rq_bvec, nvecs, *cnt))
 		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
 				       cnt, kiocb);
 
-	trace_nfsd_write_direct(rqstp, fhp, in_offset, in_count);
-
 	*cnt = 0;
-	for (int i = 0; i < n_iters; i++) {
-		if (iter_is_dio_aligned[i])
+	segment = args->segment;
+	for (i = 0; i < ARRAY_SIZE(args->segment); i++) {
+		if (segment->len == 0)
+			continue;
+		if (segment->use_dio) {
 			kiocb->ki_flags |= IOCB_DIRECT;
-		else
+			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
+						segment->len);
+		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 
-		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
+		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
 		if (host_err < 0)
 			return host_err;
 		*cnt += host_err;
-		if (host_err < iter[i].count) /* partial write? */
+		if (host_err < segment->iter.count)
 			break;
+		++segment;
 	}
 
 	return 0;

From: Chuck Lever <[email protected]>

Passing iter arrays by reference is a little risky. Instead, pass a struct with a fixed-size array so bounds checking can be done.

Name each item in the array a "segment", as the term "extent" generally refers to a set of blocks on storage, not to a buffer.

Each segment is processed via a single vfs_iocb_iter_write() call, and is either IOCB_DIRECT or buffered.

Introduce a segment constructor function so each segment is initialized identically.

Each segment has its own length. The loop that iterates over the segment array can simply skip over the segments of zero length. A count of segments is not needed.

Suggested-by Christoph Hellwig <[email protected]>

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

fs/nfsd/vfs.c +11 -15

@@ -1373,36 +1369,33 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		     unsigned int nvecs, unsigned long *cnt)
 {
 	struct file *file = args->nf->nf_file;
-	bool iter_is_dio_aligned[3];
-	struct iov_iter iter_stack[3];
-	struct iov_iter *iter = iter_stack;
-	unsigned int n_iters = 0;
-	unsigned long in_count = *cnt;
-	loff_t in_offset = kiocb->ki_pos;
+	struct nfsd_write_dio_seg *segment;
 	ssize_t host_err;
+	size_t i;
 
-	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
-					     rqstp->rq_bvec, nvecs, *cnt,
-					     args);
-	if (unlikely(!n_iters))
+	if (!nfsd_setup_write_dio_iters(args, rqstp->rq_bvec, nvecs, *cnt))
 		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
 				       cnt, kiocb);
 
-	trace_nfsd_write_direct(rqstp, fhp, in_offset, in_count);
-
 	*cnt = 0;
-	for (int i = 0; i < n_iters; i++) {
-		if (iter_is_dio_aligned[i])
+	segment = args->segment;
+	for (i = 0; i < ARRAY_SIZE(args->segment); i++) {
+		if (segment->len == 0)
+			continue;
+		if (segment->use_dio) {
 			kiocb->ki_flags |= IOCB_DIRECT;
-		else
+			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
+						segment->len);
+		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 

From: Chuck Lever <[email protected]>

Clean up: Use the usual error flow form:

	if (uncommon condition) {
		handle it;
		return;
	}
	do common thing;

in nfsd_direct_write(). Now there is a single place where the direct write path falls back to a single cached write.

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

---
 fs/nfsd/vfs.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

fs/nfsd/vfs.c +1 -8

index b7f217aa4994..b0e4105e0075 100644
@@ -1373,10 +1373,6 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	ssize_t host_err;
 	size_t i;
 
-	if (!nfsd_setup_write_dio_iters(args, rqstp->rq_bvec, nvecs, *cnt))
-		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
-				       cnt, kiocb);
-
 	*cnt = 0;
 	segment = args->segment;
 	for (i = 0; i < ARRAY_SIZE(args->segment); i++) {

@@ -1388,7 +1384,6 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 						segment->len);
 		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
-
 		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
 		if (host_err < 0)
 			return host_err;

@@ -1429,11 +1424,13 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
 
 	args.nf = nf;
-	if (nfsd_is_write_dio_possible(&args, kiocb->ki_pos, *cnt))
-		return nfsd_issue_write_dio(rqstp, fhp, &args, kiocb,
-					    nvecs, cnt);
+	if (!nfsd_is_write_dio_possible(&args, kiocb->ki_pos, *cnt) ||

From: Chuck Lever <[email protected]>

Clean up: Use the usual error flow form:

	if (uncommon condition) {
		handle it;
		return;
	}
	do common thing;

in nfsd_direct_write(). Now there is a single place where the direct write path falls back to a single cached write.

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (file->f_op->fop_flags & FOP_DONTCACHE)
+		kiocb->ki_flags |= IOCB_DONTCACHE;

IOCB_DONTCACHE isn't defined for IOCB_DIRECT. So this should move into a branch just for buffered I/O.

From: Chuck Lever <[email protected]>

Christoph says:

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (file->f_op->fop_flags & FOP_DONTCACHE)
+		kiocb->ki_flags |= IOCB_DONTCACHE;

IOCB_DONTCACHE isn't defined for IOCB_DIRECT. So this should move into a branch just for buffered I/O.

Instead, let's set up separate ki_flags for buffered I/O and for direct I/O requests. Then we don't have to set a jumble of flag bits in a single flags field.

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

fs/nfsd/vfs.c +11 -14

index b0e4105e0075..b7b9f8cf0452 100644
@@ -1262,6 +1262,8 @@ struct nfsd_write_dio_seg {
 
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
+	int				flags_buffered;
+	int				flags_direct;
 	struct nfsd_write_dio_seg	segment[3];
 };
 

@@ -1379,11 +1381,11 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (segment->len == 0)
 			continue;
 		if (segment->use_dio) {
-			kiocb->ki_flags |= IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_direct;
 			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
 						segment->len);
 		} else
-			kiocb->ki_flags &= ~IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_buffered;
 		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
 		if (host_err < 0)
 			return host_err;

@@ -1405,30 +1407,27 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct nfsd_write_dio_args args;
 
 	/*
-	 * Check if IOCB_DONTCACHE can be used when issuing buffered IO;
-	 * if so, set it to preserve intent of NFSD_IO_DIRECT (it will
-	 * be ignored for any DIO issued here).
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT when
+	 * writing unaligned segments or handling fallback I/O.
 	 */
+	args.flags_buffered = kiocb->ki_flags | IOCB_SYNC | IOCB_DSYNC;
 	if (file->f_op->fop_flags & FOP_DONTCACHE)
-		kiocb->ki_flags |= IOCB_DONTCACHE;
+		args.flags_buffered |= IOCB_DONTCACHE;
 
 	/*
-	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
-	 * both written data and dirty time stamps.
-	 *
-	 * When falling back to buffered I/O or handling the unaligned
-	 * first and last segments, the data and time stamps must be
-	 * durable before nfsd_vfs_write() returns to its caller, matching
-	 * the behavior of direct I/O.
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should
+	 * persist both written data and dirty time stamps.
 	 */
-	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
+	args.flags_direct = kiocb->ki_flags | IOCB_SYNC | IOCB_DIRECT;
 
 	args.nf = nf;
 	if (!nfsd_is_write_dio_possible(&args, kiocb->ki_pos, *cnt) ||

From: Chuck Lever <[email protected]>

Christoph says:

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (file->f_op->fop_flags & FOP_DONTCACHE)
+		kiocb->ki_flags |= IOCB_DONTCACHE;

IOCB_DONTCACHE isn't defined for IOCB_DIRECT. So this should move into a branch just for buffered I/O.

Instead, let's set up separate ki_flags for buffered I/O and for direct I/O requests. Then we don't have to set a jumble of flag bits in a single flags field.

Suggested-by Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/vfs.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

fs/nfsd/vfs.c +8 -8

index b0e4105e0075..b7b9f8cf0452 100644
@@ -1262,6 +1262,8 @@ struct nfsd_write_dio_seg {
 
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
+	int				flags_buffered;
+	int				flags_direct;
 	struct nfsd_write_dio_seg	segment[3];
 };
 

@@ -1379,11 +1381,11 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (segment->len == 0)
 			continue;
 		if (segment->use_dio) {
-			kiocb->ki_flags |= IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_direct;
 			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
 						segment->len);
 		} else
-			kiocb->ki_flags &= ~IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_buffered;
 		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
 		if (host_err < 0)
 			return host_err;

@@ -1405,30 +1407,27 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct nfsd_write_dio_args args;
 
 	/*
-	 * Check if IOCB_DONTCACHE can be used when issuing buffered IO;
-	 * if so, set it to preserve intent of NFSD_IO_DIRECT (it will
-	 * be ignored for any DIO issued here).
+	 * IOCB_DONTCACHE preserves the intent of NFSD_IO_DIRECT when
+	 * writing unaligned segments or handling fallback I/O.
 	 */
+	args.flags_buffered = kiocb->ki_flags | IOCB_SYNC | IOCB_DSYNC;
 	if (file->f_op->fop_flags & FOP_DONTCACHE)
-		kiocb->ki_flags |= IOCB_DONTCACHE;
+		args.flags_buffered |= IOCB_DONTCACHE;
 
 	/*
-	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
-	 * both written data and dirty time stamps.

fs/nfsd/vfs.c nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1421

Quoted 13 lines

fs/nfsd/vfs.c +3 -8

 	/*
-	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
-	 * both written data and dirty time stamps.
-	 *
-	 * When falling back to buffered I/O or handling the unaligned
-	 * first and last segments, the data and time stamps must be
-	 * durable before nfsd_vfs_write() returns to its caller, matching
-	 * the behavior of direct I/O.
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should
+	 * persist both written data and dirty time stamps.
 	 */
-	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
+	args.flags_direct = kiocb->ki_flags | IOCB_SYNC | IOCB_DIRECT;

AFAIK we still need: IOCB_DIRECT | IOCB_DSYNC | IOCB_SYNC

IOCB_DIRECT | IOCB_DSYNC was recently put under a microscope relative to XFS performance and the resulting improvement was merged for 6.18 with commit c91d38b57f ("xfs: rework datasync tracking and execution")

Quoted 15 lines · Mike Snitzer

fs/nfsd/vfs.c nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1421

Quoted 13 lines

fs/nfsd/vfs.c +3 -8

 	/*
-	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
-	 * both written data and dirty time stamps.
-	 *
-	 * When falling back to buffered I/O or handling the unaligned
-	 * first and last segments, the data and time stamps must be
-	 * durable before nfsd_vfs_write() returns to its caller, matching
-	 * the behavior of direct I/O.
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should
+	 * persist both written data and dirty time stamps.
 	 */
-	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
+	args.flags_direct = kiocb->ki_flags | IOCB_SYNC | IOCB_DIRECT;

AFAIK we still need: IOCB_DIRECT | IOCB_DSYNC | IOCB_SYNC

Quoted 3 lines

IOCB_DIRECT | IOCB_DSYNC was recently put under a microscope relative to XFS performance and the resulting improvement was merged for 6.18 with commit c91d38b57f ("xfs: rework datasync tracking and execution")

This looks like an xfs-specific fix. I'm reluctant to apply a fix for a specific file system implementation in what's supposed to be generic code.

If (IOCB_DIRECT | IOCB_DSYNC | IOCB_SYNC) /is/ correct for all file systems, then it needs an explanatory code comment, which I'm not yet qualified to write. I don't see any textual material in previous incarnations of this code that might help get me started.

Quoted 20 lines · Chuck Lever

Quoted 15 lines · Mike Snitzer

fs/nfsd/vfs.c nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp, · line 1421

Quoted 13 lines

fs/nfsd/vfs.c +3 -8

 	/*
-	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should persist
-	 * both written data and dirty time stamps.
-	 *
-	 * When falling back to buffered I/O or handling the unaligned
-	 * first and last segments, the data and time stamps must be
-	 * durable before nfsd_vfs_write() returns to its caller, matching
-	 * the behavior of direct I/O.
+	 * IOCB_SYNC + IOCB_DIRECT requests that iter_write should
+	 * persist both written data and dirty time stamps.
 	 */
-	kiocb->ki_flags |= IOCB_SYNC | IOCB_DSYNC;
+	args.flags_direct = kiocb->ki_flags | IOCB_SYNC | IOCB_DIRECT;

AFAIK we still need: IOCB_DIRECT | IOCB_DSYNC | IOCB_SYNC

Quoted 3 lines

IOCB_DIRECT | IOCB_DSYNC was recently put under a microscope relative to XFS performance and the resulting improvement was merged for 6.18 with commit c91d38b57f ("xfs: rework datasync tracking and execution")

Quoted 8 lines

This looks like an xfs-specific fix. I'm reluctant to apply a fix for a specific file system implementation in what's supposed to be generic code.

If (IOCB_DIRECT | IOCB_DSYNC | IOCB_SYNC) /is/ correct for all file systems, then it needs an explanatory code comment, which I'm not yet qualified to write. I don't see any textual material in previous incarnations of this code that might help get me started.

The XFS specific performance improvement isn't the point. The point is that applications (like I think DB2 is what started all this with Jan Kara and the XFS filesystem) results in the use of O_DIRECT+O_DSYNC.

It is a clear reality that other filesystems are catering to O_DIRECT+O_DSYNC. And given our findings with Christoph that buffered IO needs O_DSYNC+O_SYNC, I'd rather we not expose ourselves to not having O_DSYNC set.