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 forced to be persisted as the mandate above requires.

For many in-tree Linux file systems, time stamps, as well as other metadata not needed to find the data on disk, are piggy-backed on fdatasync-mandatory metadata updates. Thus, this change should affect only the case where previously fully-allocated file data is overwritten.

Reported-by Mike Snitzer <[email protected]>

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

Reviewed-by Jeff Layton <[email protected]>

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

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

fs/nfsd/vfs.c +12 -2

index f537a7b4ee01..5333d49910d9 100644
@@ -1314,8 +1314,18 @@ 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 (likely(!fhp->fh_use_wgather)) {
+		switch (stable) {
+		case NFS_FILE_SYNC:
+			/* persist data and timestamps */
+			kiocb.ki_flags |= IOCB_DSYNC | IOCB_SYNC;
+			break;
+		case NFS_DATA_SYNC:
+			/* persist data only */
+			kiocb.ki_flags |= IOCB_DSYNC;
+			break;
+		}
+	}
 
 	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
 	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);

Signature · 1 line

2.51.0

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. 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 record these errnos when they occur.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

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

From: Chuck Lever <[email protected]>

Noted that the NFS client's LOCALIO code still retains this check.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

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

From: Chuck Lever <[email protected]>

Noted that the NFS client's LOCALIO code still retains this check.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

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

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

fs/nfsd/vfs.c +0 -2

index 30094d8f489e..80bc105eb0b6 100644
@@ -1269,8 +1269,6 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 
 	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;
 

Signature · 1 line

2.51.0

Quoted 3 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Noted that the NFS client's LOCALIO code still retains this check.

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

Quoted 3 lines · NeilBrown

Quoted 2 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Quoted 6 lines

Quoted 1 line

Noted that the NFS client's LOCALIO code still retains this check.

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

I'm still trying to understand why it was added in the first place :)

But I'm also completely lost in the maze of fixup patches.

Quoted 11 lines · Christoph Hellwig

Quoted 9 lines · NeilBrown

Quoted 3 lines · Chuck Lever

From: Chuck Lever <[email protected]>

Noted that the NFS client's LOCALIO code still retains this check.

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

Quoted 1 line

I'm still trying to understand why it was added in the first place :)

I'm trying to understand what action you'd like me to take. Should I drop this patch?

Quoted 1 line

But I'm also completely lost in the maze of fixup patches.

Several people have asked me to collapse the fix-ups into a single patch. We would lose some history and attributions doing that. Does anyone have other thoughts?

Quoted 6 lines

Quoted 4 lines

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

I'm still trying to understand why it was added in the first place :)

I'm trying to understand what action you'd like me to take. Should I drop this patch?

Quoted 2 lines

But I'm also completely lost in the maze of fixup patches.

Several people have asked me to collapse the fix-ups into a single patch. We would lose some history and attributions doing that. Does anyone have other thoughts?

Quoted 7 lines · Chuck Lever

Quoted 6 lines

Quoted 4 lines

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

I'm still trying to understand why it was added in the first place :)

Quoted 2 lines

I'm trying to understand what action you'd like me to take. Should I drop this patch?

With "it" I meant the check. І think Mike explain this was due to a PAGE_SIZE bound buffer originally, and in that context it makes sense. Without the explanation I don't understand the rationale for adding the check in the first place.

Quoted 4 lines

Quoted 1 line

But I'm also completely lost in the maze of fixup patches.

Several people have asked me to collapse the fix-ups into a single patch. We would lose some history and attributions doing that. Does anyone have other thoughts?

The action I'd see is to collapse the series into reviewable chunks. I.e., fold the addition of the direct I/O writes into a single patch that has all the policy decisions and documents them, leaving only clearly separate prep patches separate.

Quoted 11 lines · Christoph Hellwig

Quoted 9 lines · Chuck Lever

Quoted 6 lines

Quoted 4 lines

It might be good to capture here *why* the check is removed. Is it because alignments never exceed PAGE_SIZE, or because the code is quite capable of handling larger alignments
(I haven't been following the conversation closely..)

I'm still trying to understand why it was added in the first place :)

I'm trying to understand what action you'd like me to take. Should I drop this patch?

Quoted 4 lines

With "it" I meant the check. І think Mike explain this was due to a PAGE_SIZE bound buffer originally, and in that context it makes sense. Without the explanation I don't understand the rationale for adding the check in the first place.

Agreed, Mike's original patch has no explanatory comment, and there needs to be one. Mike, can you suggest a one or two sentence comment and I will replace this patch with one that adds the comment.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

But I'm also completely lost in the maze of fixup patches.

Several people have asked me to collapse the fix-ups into a single patch. We would lose some history and attributions doing that. Does anyone have other thoughts?

The action I'd see is to collapse the series into reviewable chunks. I.e., fold the addition of the direct I/O writes into a single patch that has all the policy decisions and documents them, leaving only clearly separate prep patches separate.

Meaning: combine the patches from 3/12 to 12/12 into a single patch.

Quoted 4 lines

The action I'd see is to collapse the series into reviewable chunks. I.e., fold the addition of the direct I/O writes into a single patch that has all the policy decisions and documents them, leaving only clearly separate prep patches separate.

Meaning: combine the patches from 3/12 to 12/12 into a single patch.

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 conventional naming of a structure that carries the same arguments for a number of functions.

Reviewed-by Jeff Layton <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

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

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

fs/nfsd/vfs.c +29 -26

index 80bc105eb0b6..326c60eada65 100644
@@ -1254,7 +1254,9 @@ 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 */

@@ -1262,12 +1264,12 @@ struct nfsd_write_dio {
 
 static bool
 nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
-		       struct nfsd_file *nf, struct nfsd_write_dio *write_dio)
+			   struct nfsd_write_dio_args *args)
 {
-	const u32 dio_blocksize = nf->nf_dio_offset_align;
+	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;

@@ -1276,16 +1278,18 @@ 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;
 }
 
-static bool nfsd_iov_iter_aligned_bvec(const struct iov_iter *i,
-		unsigned int addr_mask, unsigned int len_mask)
+static bool
+nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 {
+	unsigned int len_mask = nf->nf_dio_offset_align - 1;
+	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;

@@ -1314,37 +1318,35 @@ static bool nfsd_iov_iter_aligned_bvec(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(&iters[n_iters],
-				nf->nf_dio_mem_align-1, nf->nf_dio_offset_align-1);
+		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;
 	}

@@ -1370,11 +1372,11 @@ nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
 }
 
 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, u32 *stable_how,
+		     struct kiocb *kiocb, unsigned int nvecs, unsigned long *cnt,
+		     struct nfsd_write_dio_args *args)
 {
-	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 +1386,8 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_fil
 	ssize_t host_err;
 
 	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
-				rqstp->rq_bvec, nvecs, *cnt, write_dio, nf);
+					     rqstp->rq_bvec, nvecs, *cnt,
+					     args);
 	if (unlikely(!n_iters))
 		return nfsd_buffered_write(rqstp, file, nvecs, cnt, kiocb);
 

@@ -1421,21 +1424,23 @@ 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;
+	struct nfsd_write_dio_args args;

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.

Consensus is that allowing the code to build segment arrays that are smaller than 3 is better than the I/O loop unconditionally visiting all 3 segments, skipping the zero-length ones.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

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

---
 fs/nfsd/vfs.c | 120 ++++++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 62 deletions(-)

fs/nfsd/vfs.c +10 -6

index 326c60eada65..5d6efcceb8c9 100644
@@ -1254,12 +1254,16 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio_seg {
+	struct iov_iter			iter;
+	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 */
+	size_t				first, middle, last;
+	unsigned int			nsegs;
+	struct nfsd_write_dio_seg	segment[3];
 };
 
 static bool

@@ -1267,21 +1271,20 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 			   struct nfsd_write_dio_args *args)
 {
 	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);

fs/nfsd/vfs.c +0 -8

 	return true;
 }
 

@@ -1311,47 +1314,47 @@ 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)

fs/nfsd/vfs.c +38 -42

+	iov_iter_truncate(&segment->iter, 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 bio_vec *bvec, unsigned int nvecs,
+			   unsigned long total,
+			   struct nfsd_write_dio_args *args)
+{
+	args->nsegs = 0;
+
+	if (args->first) {
+		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
+					nvecs, total, 0, args->first);
+		++args->nsegs;
 	}
 
-	/* 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;
+	nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec, nvecs,
+				total, args->first, args->middle);
+	if (!nfsd_iov_iter_aligned_bvec(args->nf,
+					&args->segment[args->nsegs].iter))
+		return false;	/* no DIO-aligned IO possible */
+	args->segment[args->nsegs].use_dio = true;
+	++args->nsegs;
 
-	/* 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;
+	if (args->last) {
+		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
+					nvecs, total, args->first +
+					args->middle, args->last);
+		++args->nsegs;
 	}
 
-	return n_iters;
+	return true;
 }
 
 static int

@@ -1377,22 +1380,12 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 		     struct nfsd_write_dio_args *args)
 {
 	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;
 	ssize_t host_err;
+	unsigned int 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(rqstp->rq_bvec, nvecs, *cnt, args))
 		return nfsd_buffered_write(rqstp, file, nvecs, cnt, kiocb);
 
-	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.

@@ -1402,18 +1395,21 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 	*stable_how = NFS_FILE_SYNC;
 
 	*cnt = 0;
-	for (int i = 0; i < n_iters; i++) {
-		if (iter_is_dio_aligned[i])
+	for (i = 0; i < args->nsegs; i++) {
+		if (args->segment[i].use_dio) {
 			kiocb->ki_flags |= IOCB_DIRECT;
-		else
+			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
+						args->segment[i].iter.count);
+		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 
-		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
+		host_err = vfs_iocb_iter_write(file, kiocb,
+					       &args->segment[i].iter);
 		if (host_err < 0)
 			return host_err;
 		*cnt += host_err;
-		if (host_err < iter[i].count) /* partial write? */
-			break;
+		if (host_err < args->segment[i].iter.count)
+			break;	/* partial write */
 	}
 
 	return 0;

Signature · 1 line

2.51.0

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

Consensus is that allowing the code to build segment arrays that are smaller than 3 is better than the I/O loop unconditionally visiting all 3 segments, skipping the zero-length ones.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

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

---
 fs/nfsd/vfs.c | 120 ++++++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 62 deletions(-)

fs/nfsd/vfs.c +10 -6

index 326c60eada65..5d6efcceb8c9 100644
@@ -1254,12 +1254,16 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+struct nfsd_write_dio_seg {
+	struct iov_iter			iter;
+	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 */
+	size_t				first, middle, last;
+	unsigned int			nsegs;
+	struct nfsd_write_dio_seg	segment[3];
 };
 
 static bool

@@ -1267,21 +1271,20 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 			   struct nfsd_write_dio_args *args)
 {
 	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);
 

fs/nfsd/vfs.c nfsd_is_write_dio_possible(loff_t offset, unsigned long len, · line 1287

Quoted 7 lines

fs/nfsd/vfs.c +3 -4

-	args->start_len = start_end - offset;
-	args->middle_len = middle_end - start_end;
-	args->end_len = orig_end - middle_end;
-
+	args->first = first_end - offset;
+	args->middle = middle_end - first_end;
+	args->last = orig_end - middle_end;

The commit message didn't warn that "start" is being renamed to "first" and that "_len" is being dropped.
I can understand the former (though I'd go for "head", "body", "tail" myself) but I would prefer to keep _len.

Quoted 15 lines

fs/nfsd/vfs.c +0 -8

 	return true;
 }
 

@@ -1311,47 +1314,47 @@ 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)

fs/nfsd/vfs.c nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i) · line 1324

Quoted 10 lines

fs/nfsd/vfs.c +7 -2

+static void
+nfsd_write_dio_seg_init(struct nfsd_write_dio_seg *segment,
+			struct bio_vec *bvec, unsigned int nvecs,
+			unsigned long total, size_t start, size_t len)
 {
-	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);

To me, this would read better as

     if (start_len)
              iov_iter......

But I don't see anything actually wrong with the patch, so

Reviewed-by NeilBrown <[email protected]>

NeilBrown

Quoted 117 lines

fs/nfsd/vfs.c +38 -42

+	iov_iter_truncate(&segment->iter, 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 bio_vec *bvec, unsigned int nvecs,
+			   unsigned long total,
+			   struct nfsd_write_dio_args *args)
+{
+	args->nsegs = 0;
+
+	if (args->first) {
+		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
+					nvecs, total, 0, args->first);
+		++args->nsegs;
 	}
 
-	/* 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;
+	nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec, nvecs,
+				total, args->first, args->middle);
+	if (!nfsd_iov_iter_aligned_bvec(args->nf,
+					&args->segment[args->nsegs].iter))
+		return false;	/* no DIO-aligned IO possible */
+	args->segment[args->nsegs].use_dio = true;
+	++args->nsegs;
 
-	/* 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;
+	if (args->last) {
+		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
+					nvecs, total, args->first +
+					args->middle, args->last);
+		++args->nsegs;
 	}
 
-	return n_iters;
+	return true;
 }
 
 static int

@@ -1377,22 +1380,12 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 		     struct nfsd_write_dio_args *args)
 {
 	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;
 	ssize_t host_err;
+	unsigned int 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(rqstp->rq_bvec, nvecs, *cnt, args))
 		return nfsd_buffered_write(rqstp, file, nvecs, cnt, kiocb);
 
-	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.

@@ -1402,18 +1395,21 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 	*stable_how = NFS_FILE_SYNC;
 
 	*cnt = 0;
-	for (int i = 0; i < n_iters; i++) {
-		if (iter_is_dio_aligned[i])
+	for (i = 0; i < args->nsegs; i++) {
+		if (args->segment[i].use_dio) {
 			kiocb->ki_flags |= IOCB_DIRECT;
-		else
+			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
+						args->segment[i].iter.count);
+		} else
 			kiocb->ki_flags &= ~IOCB_DIRECT;
 
-		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
+		host_err = vfs_iocb_iter_write(file, kiocb,
+					       &args->segment[i].iter);
 		if (host_err < 0)
 			return host_err;
 		*cnt += host_err;
-		if (host_err < iter[i].count) /* partial write? */
-			break;
+		if (host_err < args->segment[i].iter.count)
+			break;	/* partial write */
 	}
 
 	return 0;

Signature · 1 line

2.51.0

From: Chuck Lever <[email protected]>

Pages in the RPC receive buffer are contiguous. Practically speaking, this means that, after the first bvec, the bv_offset field is always zero.

The loop in nfsd_iov_iter_aligned_bvec() also sets "skip" to zero after the first bvec is checked. Thus, for bvecs following the first one, bv_offset = 0 and skip = 0, and the check becomes:

(0 + 0) & addr_mask = 0

This always passes regardless of the alignment mask.

Since all bvecs after the first one start at bv_offset zero (page- aligned), they are inherently aligned for DIO. Therefore, only the first bvec needs to be checked for DIO alignment.

Note that for RDMA transports, the incoming payload always starts on page alignment, since svcrdma sets up the RDMA Read sink buffers that way. For RDMA, this loop visits every bvec in each payload, and never finds an unaligned bvec.

Reviewed-by Jeff Layton <[email protected]>

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

From: Chuck Lever <[email protected]>

Pages in the RPC receive buffer are contiguous. Practically speaking, this means that, after the first bvec, the bv_offset field is always zero.

The loop in nfsd_iov_iter_aligned_bvec() also sets "skip" to zero after the first bvec is checked. Thus, for bvecs following the first one, bv_offset = 0 and skip = 0, and the check becomes:

(0 + 0) & addr_mask = 0

This always passes regardless of the alignment mask.

Since all bvecs after the first one start at bv_offset zero (page- aligned), they are inherently aligned for DIO. Therefore, only the first bvec needs to be checked for DIO alignment.

Note that for RDMA transports, the incoming payload always starts on page alignment, since svcrdma sets up the RDMA Read sink buffers that way. For RDMA, this loop visits every bvec in each payload, and never finds an unaligned bvec.

Reviewed-by Jeff Layton <[email protected]>

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

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

fs/nfsd/vfs.c +7 -9

index 5d6efcceb8c9..37353fb48d58 100644
@@ -1288,30 +1288,20 @@ nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
 	return true;
 }
 
+/*
+ * Check if the bvec iterator is aligned for direct I/O.
+ *
+ * bvecs generated from RPC receive buffers are contiguous: After the first
+ * bvec, all subsequent bvecs start at bv_offset zero (page-aligned).
+ * Therefore, only the first bvec is checked.
+ */
 static bool
 nfsd_iov_iter_aligned_bvec(const struct nfsd_file *nf, const struct iov_iter *i)
 {
-	unsigned int len_mask = nf->nf_dio_offset_align - 1;
 	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;
 
-	if (size & len_mask)
-		return false;
-	do {
-		size_t len = bvec->bv_len;
-
-		if (len > size)

fs/nfsd/vfs.c +0 -0

 }
 
 static void

Signature · 1 line

2.51.0

From: Chuck Lever <[email protected]>

When direct I/O is not feasible (due to missing alignment info, too-small writes, or no alignment possible), pack the entire write payload into a single non-DIO segment and follow the usual direct write I/O path.

This simplifies nfsd_direct_write() by eliminating the fallback path and the separate nfsd_buffered_write() call - all writes now go through nfsd_issue_write_dio() which handles both DIO and buffered segments.

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

From: Chuck Lever <[email protected]>

When direct I/O is not feasible (due to missing alignment info, too-small writes, or no alignment possible), pack the entire write payload into a single non-DIO segment and follow the usual direct write I/O path.

This simplifies nfsd_direct_write() by eliminating the fallback path and the separate nfsd_buffered_write() call - all writes now go through nfsd_issue_write_dio() which handles both DIO and buffered segments.

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

---
 fs/nfsd/vfs.c | 93 +++++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 44 deletions(-)

fs/nfsd/vfs.c +16 -28

index 37353fb48d58..1b1173de4f78 100644
@@ -1261,33 +1261,10 @@ struct nfsd_write_dio_seg {
 
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
-	size_t				first, middle, last;
 	unsigned int			nsegs;
 	struct nfsd_write_dio_seg	segment[3];
 };
 
-static bool
-nfsd_is_write_dio_possible(loff_t offset, unsigned long len,
-			   struct nfsd_write_dio_args *args)
-{
-	u32 dio_blocksize = args->nf->nf_dio_offset_align;
-	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;
-
-	first_end = round_up(offset, dio_blocksize);
-	orig_end = offset + len;
-	middle_end = round_down(orig_end, dio_blocksize);
-
-	args->first = first_end - offset;
-	args->middle = middle_end - first_end;
-	args->last = orig_end - middle_end;
-	return true;
-}
-
 /*
  * Check if the bvec iterator is aligned for direct I/O.
  *

@@ -1316,35 +1293,66 @@ nfsd_write_dio_seg_init(struct nfsd_write_dio_seg *segment,
 	segment->use_dio = false;
 }
 
-static bool
-nfsd_setup_write_dio_iters(struct bio_vec *bvec, unsigned int nvecs,
-			   unsigned long total,
-			   struct nfsd_write_dio_args *args)
+static void
+nfsd_write_dio_iters_init(struct bio_vec *bvec, unsigned int nvecs,
+			  loff_t offset, unsigned long total,
+			  struct nfsd_write_dio_args *args)
 {
+	u32 offset_align = args->nf->nf_dio_offset_align;
+	u32 mem_align = args->nf->nf_dio_mem_align;
+	loff_t prefix_end, orig_end, middle_end;
+	size_t prefix, middle, suffix;
+
 	args->nsegs = 0;
 
-	if (args->first) {
+	/*
+	 * Check if direct I/O is feasible for this write request.
+	 * If alignments are not available, the write is too small,
+	 * or no alignment can be found, fall back to buffered I/O.
+	 */
+	if (unlikely(!mem_align || !offset_align) ||
+	    unlikely(total < max(offset_align, mem_align)))

fs/nfsd/vfs.c +23 -16

+
+	if (!middle)
+		goto no_dio;
+
+	if (prefix) {
 		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
-					nvecs, total, 0, args->first);
+					nvecs, total, 0, prefix);
 		++args->nsegs;
 	}
 
 	nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec, nvecs,
-				total, args->first, args->middle);
+				total, prefix, middle);
 	if (!nfsd_iov_iter_aligned_bvec(args->nf,
 					&args->segment[args->nsegs].iter))
-		return false;	/* no DIO-aligned IO possible */
+		goto no_dio;
 	args->segment[args->nsegs].use_dio = true;
 	++args->nsegs;
 
-	if (args->last) {
+	if (suffix) {
 		nfsd_write_dio_seg_init(&args->segment[args->nsegs], bvec,
-					nvecs, total, args->first +
-					args->middle, args->last);
+					nvecs, total, prefix + middle, suffix);
 		++args->nsegs;
 	}
 
-	return true;
+	return;
+
+no_dio:
+	/* No alignment possible - pack into single non-DIO segment */
+	nfsd_write_dio_seg_init(&args->segment[0], bvec, nvecs, total,
+				0, total);
+	args->nsegs = 1;
 }
 
 static int

@@ -1365,7 +1373,7 @@ nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
 }
 
 static int
-nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how,
+nfsd_issue_dio_write(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how,
 		     struct kiocb *kiocb, unsigned int nvecs, unsigned long *cnt,
 		     struct nfsd_write_dio_args *args)
 {

@@ -1373,9 +1381,6 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 	ssize_t host_err;
 	unsigned int i;
 
-	if (!nfsd_setup_write_dio_iters(rqstp->rq_bvec, nvecs, *cnt, args))
-		return nfsd_buffered_write(rqstp, file, nvecs, cnt, kiocb);
-
 	/*
 	 * Any buffered IO issued here will be misaligned, use
 	 * sync IO to ensure it has completed before returning.

@@ -1384,6 +1389,9 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how
 	kiocb->ki_flags |= (IOCB_DSYNC|IOCB_SYNC);
 	*stable_how = NFS_FILE_SYNC;
 
+	nfsd_write_dio_iters_init(rqstp->rq_bvec, nvecs, kiocb->ki_pos,
+				  *cnt, args);
+
 	*cnt = 0;
 	for (i = 0; i < args->nsegs; i++) {
 		if (args->segment[i].use_dio) {

@@ -1422,11 +1430,8 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (args.nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
 		kiocb->ki_flags |= IOCB_DONTCACHE;
 
-	if (nfsd_is_write_dio_possible(kiocb->ki_pos, *cnt, &args))
-		return nfsd_issue_write_dio(rqstp, fhp, stable_how, kiocb,
-					    nvecs, cnt, &args);
-
-	return nfsd_buffered_write(rqstp, args.nf->nf_file, nvecs, cnt, kiocb);
+	return nfsd_issue_dio_write(rqstp, fhp, stable_how, kiocb, nvecs,
+				    cnt, &args);
 }
 
 /**

Signature · 1 line

2.51.0

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.

Quoted 8 lines

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

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.

and

Quoted 11 lines

Quoted 8 lines

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

So, observe the existing setting of ki_flags rather than forcing persistence unconditionally, and ensure that DONTCACHE is not set for IOCB_DIRECT writes.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

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

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.

and

Quoted 9 lines

Quoted 8 lines

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 2 lines

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

So, observe the existing setting of ki_flags rather than forcing persistence unconditionally, and ensure that DONTCACHE is not set for IOCB_DIRECT writes.

Suggested-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

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

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

fs/nfsd/vfs.c +14 -19

index 1b1173de4f78..0e5e82b286f1 100644
@@ -1261,6 +1261,8 @@ struct nfsd_write_dio_seg {
 
 struct nfsd_write_dio_args {
 	struct nfsd_file		*nf;
+	int				flags_buffered;
+	int				flags_direct;
 	unsigned int			nsegs;
 	struct nfsd_write_dio_seg	segment[3];
 };

@@ -1373,33 +1375,25 @@ nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
 }
 
 static int
-nfsd_issue_dio_write(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *stable_how,
-		     struct kiocb *kiocb, unsigned int nvecs, unsigned long *cnt,
-		     struct nfsd_write_dio_args *args)
+nfsd_issue_dio_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		     struct kiocb *kiocb, unsigned int nvecs,
+		     unsigned long *cnt, struct nfsd_write_dio_args *args)
 {
 	struct file *file = args->nf->nf_file;
 	ssize_t host_err;
 	unsigned int i;
 
-	/*
-	 * 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|IOCB_SYNC);
-	*stable_how = NFS_FILE_SYNC;
-
 	nfsd_write_dio_iters_init(rqstp->rq_bvec, nvecs, kiocb->ki_pos,
 				  *cnt, args);
 
 	*cnt = 0;
 	for (i = 0; i < args->nsegs; i++) {
 		if (args->segment[i].use_dio) {
-			kiocb->ki_flags |= IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_direct;
 			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
 						args->segment[i].iter.count);
 		} else
-			kiocb->ki_flags &= ~IOCB_DIRECT;
+			kiocb->ki_flags = args->flags_buffered;
 
 		host_err = vfs_iocb_iter_write(file, kiocb,
 					       &args->segment[i].iter);

@@ -1423,15 +1417,16 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	args.nf = nf;
 
 	/*
-	 * 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;
 	if (args.nf->nf_file->f_op->fop_flags & FOP_DONTCACHE)
-		kiocb->ki_flags |= IOCB_DONTCACHE;
+		args.flags_buffered |= IOCB_DONTCACHE;
 
-	return nfsd_issue_dio_write(rqstp, fhp, stable_how, kiocb, nvecs,
-				    cnt, &args);
+	args.flags_direct = kiocb->ki_flags | IOCB_DIRECT;
+
+	return nfsd_issue_dio_write(rqstp, fhp, kiocb, nvecs, cnt, &args);
 }
 
 /**

Signature · 1 line

2.51.0

Quoted 3 lines

So, observe the existing setting of ki_flags rather than forcing persistence unconditionally, and ensure that DONTCACHE is not set for IOCB_DIRECT writes.

Of the write has no prefix or suffix and so is entirely O_DIRECT, then we get DATASYNC for free do we not?

From: Chuck Lever <[email protected]>

There is now only one caller of nfsd_buffered_write(), so it can be folded back into nfsd_vfs_write().

Reviewed-by Jeff Layton <[email protected]>

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

From: Chuck Lever <[email protected]>

There is now only one caller of nfsd_buffered_write(), so it can be folded back into nfsd_vfs_write().

Reviewed-by Jeff Layton <[email protected]>

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

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

fs/nfsd/vfs.c +7 -20

index 0e5e82b286f1..8518fb619f56 100644
@@ -1357,23 +1357,6 @@ nfsd_write_dio_iters_init(struct bio_vec *bvec, unsigned int nvecs,
 	args->nsegs = 1;
 }
 
-static int
-nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
-		    unsigned int nvecs, unsigned long *cnt,
-		    struct kiocb *kiocb)
-{
-	struct iov_iter iter;
-	int host_err;
-
-	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
-	host_err = vfs_iocb_iter_write(file, kiocb, &iter);
-	if (host_err < 0)
-		return host_err;
-	*cnt = host_err;
-
-	return 0;
-}
-
 static int
 nfsd_issue_dio_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		     struct kiocb *kiocb, unsigned int nvecs,

@@ -1457,6 +1440,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	u32			stable = *stable_how;
 	struct kiocb		kiocb;
 	struct svc_export	*exp;
+	struct iov_iter		iter;
 	errseq_t		since;
 	__be32			nfserr;
 	int			host_err;

@@ -1517,10 +1501,13 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	case NFSD_IO_DONTCACHE:
 		if (file->f_op->fop_flags & FOP_DONTCACHE)
 			kiocb.ki_flags |= IOCB_DONTCACHE;
-		fallthrough; /* must call nfsd_buffered_write */
+		fallthrough;
 	case NFSD_IO_BUFFERED:
-		host_err = nfsd_buffered_write(rqstp, file,
-					       nvecs, cnt, &kiocb);
+		iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
+		host_err = vfs_iocb_iter_write(file, &kiocb, &iter);
+		if (host_err < 0)
+			break;
+		*cnt = host_err;
 		break;
 	}
 	if (host_err < 0) {

Signature · 1 line

2.51.0

From: Mike Snitzer <[email protected]>

This document details the NFSD IO modes that are configurable using NFSD's experimental debugfs interfaces:

/sys/kernel/debug/nfsd/io_cache_read
/sys/kernel/debug/nfsd/io_cache_write

This document will evolve as NFSD's interfaces do (e.g. if/when NFSD's debugfs interfaces are replaced with per-export controls).

Future updates will provide more specific guidance and howto information to help others use and evaluate NFSD's IO modes:
BUFFERED, DONTCACHE and DIRECT.

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

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

---
 .../filesystems/nfs/nfsd-io-modes.rst         | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/filesystems/nfs/nfsd-io-modes.rst