NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

From: Chuck Lever <[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.

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

From: Chuck Lever <[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.

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

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

fs/nfsd/vfs.c +20 -4

index 8b2dc7a88aab..ae9c41f7374e 100644
@@ -1254,6 +1254,22 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
+static int
+nfsd_iocb_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;
+}
+
 /**
  * nfsd_vfs_write - write data to an already-open file
  * @rqstp: RPC execution context

@@ -1282,7 +1298,6 @@ 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;

@@ -1319,25 +1334,24 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		kiocb.ki_flags |= IOCB_DSYNC;
 
 	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
-	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
+
 	since = READ_ONCE(file->f_wb_err);
 	if (verf)
 		nfsd_copy_write_verifier(verf, nn);
 
 	switch (nfsd_io_cache_write) {
-	case NFSD_IO_BUFFERED:
-		break;
 	case NFSD_IO_DONTCACHE:
 		if (file->f_op->fop_flags & FOP_DONTCACHE)
 			kiocb.ki_flags |= IOCB_DONTCACHE;
+		fallthrough;
+	case NFSD_IO_BUFFERED:
+		host_err = nfsd_iocb_write(rqstp, file, nvecs, cnt, &kiocb);
 		break;

From: Chuck Lever <[email protected]>

The upper layer may want to know when the receive buffer's .pages array is guaranteed to contain only an opaque payload. This permits the upper layer to optimize its buffer handling.

NB: Since svc_rdma_recvfrom.c is under net/, we use the comment style that is preferred in the networking layer.

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

---
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 5 +++++
 1 file changed, 5 insertions(+)

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +0 -0

index e7e4a39ca6c6..b1a0c72f73de 100644
@@ -815,6 +815,11 @@ static void svc_rdma_read_complete_one(struct svc_rqst *rqstp,
 	buf->page_len = length;

From: Chuck Lever <[email protected]>

Following on https://lore.kernel.org/linux-nfs/[email protected]/
this series includes all patches needed to make NFSD Direct WRITE work, plus an experimental follow-on patch.

Mike, just send along any responses to review comments as patch snippets and I will apply them as needed.

Changes since v4:
* Split out refactoring nfsd_buffered_write() into a separate patch
* Expand patch description of 1/4
* Don't set IOCB_SYNC flag

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

As a reminder to the thread, Section 3.3.7 of RFC 1813 says:

If stable is FILE_SYNC, the server must commit the data written plus all file system metadata to stable storage before returning results.

The text is a bit blurry about whether "file system metadata" means all of the outstanding metadata changes for every file, or just the metadata changes for the target file handle.

NFSD has historically treated DATA_SYNC and FILE_SYNC identically, as the Linux NFS client does not use DATA_SYNC (IIRC).

Quoted 22 lines · Chuck Lever

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

As a reminder to the thread, Section 3.3.7 of RFC 1813 says:

If stable is FILE_SYNC, the server must commit the data written plus all file system metadata to stable storage before returning results.

The text is a bit blurry about whether "file system metadata" means all of the outstanding metadata changes for every file, or just the metadata changes for the target file handle.

Quoted 3 lines

NFSD has historically treated DATA_SYNC and FILE_SYNC identically, as the Linux NFS client does not use DATA_SYNC (IIRC).

Surely it just meant for the one file.

FILE_SYNC is only applicable to
WRITE/COMMIT operations and those only deal with a single file at a time.

If the client gets back FILE_SYNC on a write, it should _not_ assume that all outstanding dirty data to all files has been sync'ed.

Quoted 23 lines · Jeff Layton

Quoted 22 lines · Chuck Lever

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

As a reminder to the thread, Section 3.3.7 of RFC 1813 says:

If stable is FILE_SYNC, the server must commit the data written plus all file system metadata to stable storage before returning results.

The text is a bit blurry about whether "file system metadata" means all of the outstanding metadata changes for every file, or just the metadata changes for the target file handle.

Quoted 5 lines

Quoted 3 lines

NFSD has historically treated DATA_SYNC and FILE_SYNC identically, as the Linux NFS client does not use DATA_SYNC (IIRC).

Surely it just meant for the one file.

Well yes that is the traditional understanding. I'm merely pointing out that the actual text is not quite as specific as what we've come to understand.

Quoted 3 lines

FILE_SYNC is only applicable to
WRITE/COMMIT operations and those only deal with a single file at a time.

True but you may recall that NFSD's COMMIT used to ignore the range arguments and flush the whole file. Some file systems used to flush all dirty data in this case, IIRC.

There's always been a bit of a mismatch between the spec and what NFSD has implemented.

Quoted 2 lines

If the client gets back FILE_SYNC on a write, it should _not_ assume that all outstanding dirty data to all files has been sync'ed.

Agreed.

But back to Mike's point.

- The spec says NFS_DATA_SYNC means persist file data.

- The spec says NFS_FILE_SYNC means persist file data and file attributes.

- After consulting with the section describing COMMIT, I think that COMMIT is supposed to persist both file data and attributes.

And my reading of the code in fs/nfsd/vfs.c is that NFSD does the equivalent of NFS_DATA_SYNC in all of these cases, and has done for as long as I cared to chase the commit log.

Moveover, commit 3f3503adb332 did not introduce this behavior.

Previous to that commit, nfsd_vfs_write() passed RWF_SYNC to vfs_iter_write(). This API uses kiocb_set_rw_flags() to convert the RWF flag into an IOCB flag. kiocb_set_rw_flags does this:

        kiocb_flags |= (__force int) (flags & RWF_SUPPORTED);
        if (flags & RWF_SYNC)
                kiocb_flags |= IOCB_DSYNC;

And that's where I copied IOCB_DSYNC from. The use of RWF_SYNC was introduced in 2016 by commit 24368aad47dc ("nfsd: use RWF_SYNC").

So we've tacitly agreed to let NFSD fall short of the specs in this regard for some time. However I don't believe this is documented anywhere.

Based on this reasoning, IOCB_DSYNC is historically correct for the DIRECT WRITE path and its fallbacks. I'm guessing that an O_DIRECT WRITE is going to persist the written data but won't persist file attribute changes either.

I'm open to making NFSD adhere more strictly to the spec language, but I bet there will be a performance impact. Maybe that impact will be unnoticeable on modern storage devices.

Quoted 79 lines · Chuck Lever

Quoted 23 lines · Jeff Layton

Quoted 22 lines · Chuck Lever

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

As a reminder to the thread, Section 3.3.7 of RFC 1813 says:

If stable is FILE_SYNC, the server must commit the data written plus all file system metadata to stable storage before returning results.

The text is a bit blurry about whether "file system metadata" means all of the outstanding metadata changes for every file, or just the metadata changes for the target file handle.

Quoted 5 lines

Quoted 3 lines

NFSD has historically treated DATA_SYNC and FILE_SYNC identically, as the Linux NFS client does not use DATA_SYNC (IIRC).

Surely it just meant for the one file.

Well yes that is the traditional understanding. I'm merely pointing out that the actual text is not quite as specific as what we've come to understand.

Quoted 3 lines

FILE_SYNC is only applicable to
WRITE/COMMIT operations and those only deal with a single file at a time.

True but you may recall that NFSD's COMMIT used to ignore the range arguments and flush the whole file. Some file systems used to flush all dirty data in this case, IIRC.

There's always been a bit of a mismatch between the spec and what NFSD has implemented.

Quoted 2 lines

If the client gets back FILE_SYNC on a write, it should _not_ assume that all outstanding dirty data to all files has been sync'ed.

Agreed.

But back to Mike's point.

- The spec says NFS_DATA_SYNC means persist file data.

- The spec says NFS_FILE_SYNC means persist file data and file attributes.

- After consulting with the section describing COMMIT, I think that COMMIT is supposed to persist both file data and attributes.

And my reading of the code in fs/nfsd/vfs.c is that NFSD does the equivalent of NFS_DATA_SYNC in all of these cases, and has done for as long as I cared to chase the commit log.

Moveover, commit 3f3503adb332 did not introduce this behavior.

Previous to that commit, nfsd_vfs_write() passed RWF_SYNC to vfs_iter_write(). This API uses kiocb_set_rw_flags() to convert the RWF flag into an IOCB flag. kiocb_set_rw_flags does this:

        kiocb_flags |= (__force int) (flags & RWF_SUPPORTED);
        if (flags & RWF_SYNC)
                kiocb_flags |= IOCB_DSYNC;

And that's where I copied IOCB_DSYNC from. The use of RWF_SYNC was introduced in 2016 by commit 24368aad47dc ("nfsd: use RWF_SYNC").

Quoted 9 lines

So we've tacitly agreed to let NFSD fall short of the specs in this regard for some time. However I don't believe this is documented anywhere.

Based on this reasoning, IOCB_DSYNC is historically correct for the DIRECT WRITE path and its fallbacks. I'm guessing that an O_DIRECT WRITE is going to persist the written data but won't persist file attribute changes either.

I think that's the case, generally.

Quoted 4 lines

I'm open to making NFSD adhere more strictly to the spec language, but I bet there will be a performance impact. Maybe that impact will be unnoticeable on modern storage devices.

Ok, I had missed the context that we had been doing this all along anyway. In that case, IOCB_DSYNC seems like it's probably acceptable. We likely have bigger cache coherency problems outstanding than potential timestamp rollbacks anyway.

Alternately, we could just return NFS_DATA_SYNC, but then we'd have to deal with follow-on commits.

Quoted 23 lines · Jeff Layton

Quoted 22 lines · Chuck Lever

Quoted 3 lines · Mike Snitzer

On Mon, Oct 20, 2025 at 12:25:42PM -0400, Chuck Lever wrote:
Just a bit concerned about removing IOCB_SYNC in that we're altering stable_how to be NFS_FILE_SYNC.

Commit 3f3503adb332 ("NFSD: Use vfs_iocb_iter_write()") introduces the first use of IOCB_ flags in NFSD's write path, and it uses IOCB_DSYNC. The patch has Reviewed-by's from Christoph, Neil, and Jeff.

Should we be concerned that IOCB_DSYNC does not persist time stamp changes that might be lost during an unplanned server boot?

As a reminder to the thread, Section 3.3.7 of RFC 1813 says:

If stable is FILE_SYNC, the server must commit the data written plus all file system metadata to stable storage before returning results.

The text is a bit blurry about whether "file system metadata" means all of the outstanding metadata changes for every file, or just the metadata changes for the target file handle.

Quoted 5 lines

Quoted 3 lines

NFSD has historically treated DATA_SYNC and FILE_SYNC identically, as the Linux NFS client does not use DATA_SYNC (IIRC).

Surely it just meant for the one file.

Well yes that is the traditional understanding. I'm merely pointing out that the actual text is not quite as specific as what we've come to understand.

Quoted 3 lines

FILE_SYNC is only applicable to
WRITE/COMMIT operations and those only deal with a single file at a time.

True but you may recall that NFSD's COMMIT used to ignore the range arguments and flush the whole file. Some file systems used to flush all dirty data in this case, IIRC.

There's always been a bit of a mismatch between the spec and what NFSD has implemented.

Quoted 2 lines

If the client gets back FILE_SYNC on a write, it should _not_ assume that all outstanding dirty data to all files has been sync'ed.

Agreed.

But back to Mike's point.

- The spec says NFS_DATA_SYNC means persist file data.

- The spec says NFS_FILE_SYNC means persist file data and file attributes.

- After consulting with the section describing COMMIT, I think that COMMIT is supposed to persist both file data and attributes.

And my reading of the code in fs/nfsd/vfs.c is that NFSD does the equivalent of NFS_DATA_SYNC in all of these cases, and has done for as long as I cared to chase the commit log.

Moveover, commit 3f3503adb332 did not introduce this behavior.

Previous to that commit, nfsd_vfs_write() passed RWF_SYNC to vfs_iter_write(). This API uses kiocb_set_rw_flags() to convert the RWF flag into an IOCB flag. kiocb_set_rw_flags does this:

        kiocb_flags |= (__force int) (flags & RWF_SUPPORTED);
        if (flags & RWF_SYNC)
                kiocb_flags |= IOCB_DSYNC;

And that's where I copied IOCB_DSYNC from. The use of RWF_SYNC was introduced in 2016 by commit 24368aad47dc ("nfsd: use RWF_SYNC").

So we've tacitly agreed to let NFSD fall short of the specs in this regard for some time. However I don't believe this is documented anywhere.

Based on this reasoning, IOCB_DSYNC is historically correct for the DIRECT WRITE path and its fallbacks. I'm guessing that an O_DIRECT WRITE is going to persist the written data but won't persist file attribute changes either.

I'm open to making NFSD adhere more strictly to the spec language, but I bet there will be a performance impact. Maybe that impact will be unnoticeable on modern storage devices.