NFSD: Implement NFSD_IO_DIRECT for NFS WRITE

From: Chuck Lever <[email protected]>

In a subsequent patch, nfsd_vfs_write() will promote an UNSTABLE WRITE to be a FILE_SYNC WRITE. This indicates that the client does

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.

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.

fs/nfsd/vfs.c +5 -0

+static int
+nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
+		    unsigned int nvecs, unsigned long *cnt,
+		    struct kiocb *kiocb)
+{

fs/nfsd/vfs.c +6 -0

+	*stable_how = NFS_FILE_SYNC;
+
+	*cnt = 0;
+	for (int i = 0; i < n_iters; i++) {
+		if (iter_is_dio_aligned[i])
+			kiocb->ki_flags |= IOCB_DIRECT;

Quoted 13 lines · Chuck Lever

From: Mike Snitzer <[email protected]>

fs/nfsd/vfs.c +11 -0

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

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1374

Quoted 4 lines

fs/nfsd/vfs.c +4 -0

+		return host_err;
+	*cnt = host_err;
+
+	return 0;

Nothing really buffered here per se, it's just a small wrapper around vfs_iocb_iter_write.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1431

Quoted 6 lines

fs/nfsd/vfs.c +6 -0

+			} 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;

-EINVAL can be lot more things than alignment failure. And more
importantly alignment failures should not happen with the proper checks in place.

I tend to agree that I expect an alignment failure will rarely if ever happen if we've done our job well.

Quoted 7 lines · Chuck Lever

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.

Can you define synchronous better here? The term is unfortunately overloaded between synchronous syscalls vs aio/io_uring and O_(D)SYNC style I/O. As of now I don't understand which one you mean, especially with the DONTCACHE reference thrown in, but I guess I'll figure it out reading the patch.

Quoted 3 lines

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.

Did you see -ENOTBLK leaking out of the file systems? Because at least for iomap it is supposed to be an indication that the file system ->write_iter handler needs to retry using buffered I/O and never leak to the caller.

Quoted 5 lines

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.

I don't understand this paragraph. What does starting point mean here? How does it matter for the patch description?

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1261

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+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 */
+};

Looking at how the code is structured later on, it seems like it would work much better if each of these sections had it's own object with the len, iov_iter, flag if it's aligned, etc. Otherwise we have this structure and lots of arrays of three items passed around.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1291

Quoted 3 lines

fs/nfsd/vfs.c +3 -0

+static bool
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
+                        unsigned int len_mask)

Wouldn't it make sense to track the alignment when building the bio_vec array instead of doing another walk here touching all cache lines?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (unlikely(dio_blocksize > PAGE_SIZE))
+		return false;

Why does this matter? Can you add a comment explaining it?

Quoted 11 lines

fs/nfsd/vfs.c +11 -0

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

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1374

Quoted 4 lines

fs/nfsd/vfs.c +4 -0

+		return host_err;
+	*cnt = host_err;
+
+	return 0;

Nothing really buffered here per se, it's just a small wrapper around vfs_iocb_iter_write.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 6 lines

fs/nfsd/vfs.c +6 -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 | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

Also IOCB_SYNC is wrong here, as the only thing it does over IOCB_DSYNC is also forcing back of metadata not needed to find data (aka timestamps), which I can't see any need for here.

Quoted 11 lines

fs/nfsd/vfs.c +11 -0

+	*stable_how = NFS_FILE_SYNC;
+
+	*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) {

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1420

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+			/*
+			 * VFS will return -ENOTBLK if DIO WRITE fails to
+			 * invalidate the page cache. Retry using buffered IO.
+			 */
+			if (unlikely(host_err == -ENOTBLK)) {

The VFS certainly does not, and if it leaks out of a specific file system we need to fix that.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1431

Quoted 6 lines

fs/nfsd/vfs.c +6 -0

+			} 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;

-EINVAL can be lot more things than alignment failure. And more
importantly alignment failures should not happen with the proper checks in place.

Quoted 5 lines

Can you define synchronous better here? The term is unfortunately overloaded between synchronous syscalls vs aio/io_uring and O_(D)SYNC style I/O. As of now I don't understand which one you mean, especially with the DONTCACHE reference thrown in, but I guess I'll figure it out reading the patch.

It clearly talks about synchronous IO. DONTCACHE just happens to be the buffered IO that'll be used if supported.

I don't see anything that needs changing here.

Quoted 9 lines

Quoted 4 lines

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.

Did you see -ENOTBLK leaking out of the file systems? Because at least for iomap it is supposed to be an indication that the file system ->write_iter handler needs to retry using buffered I/O and never leak to the caller.

fs/iomap/direct-io.c:__iomap_dio_rw() will return -ENOTBLK on its own.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 2 lines

I don't understand this paragraph. What does starting point mean here? How does it matter for the patch description?

This patch's NFSD changes were starting point for NFS commit c817248fc831

Would prefer that cleanup, if done, to happen with an incremental follow-up patch.

Quoted 7 lines

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1291

Quoted 4 lines

fs/nfsd/vfs.c +3 -0

+static bool
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
+                        unsigned int len_mask)

Wouldn't it make sense to track the alignment when building the bio_vec array instead of doing another walk here touching all cache lines?

Yes, that is the conventional wisdom that justified Keith removing iov_iter_aligned. But for NFSD's WRITE payload the bio_vec is built outside of the fs/nfsd/vfs.c code. Could be there is a natural way to clean this up (to make the sunrpc layer to conditionally care about alignment) but I didn't confront that yet.

Room for follow-on improvement to be sure.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (unlikely(dio_blocksize > PAGE_SIZE))
+		return false;

Why does this matter? Can you add a comment explaining it?

I did/do have concerns that a bug in the storage driver could expose dio_offset_align that is far too large and so bounded dio_blocksize to a conservative size. What is a better constant to check?

Quoted 7 lines

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 3 lines

fs/nfsd/vfs.c +3 -0

+	 * Also update @stable_how to avoid need for COMMIT.
+	 */
+	kiocb->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

The IO needs to have reached stable storage.

Quoted 3 lines

Also IOCB_SYNC is wrong here, as the only thing it does over IOCB_DSYNC is also forcing back of metadata not needed to find data (aka timestamps), which I can't see any need for here.

Well, we're eliding any followup SYNC from the NFS client by setting stable_how to NFS_FILE_SYNC, so I made sure to use SYNC:

Quoted 2 lines

The VFS certainly does not, and if it leaks out of a specific file system we need to fix that.

As I said above, fs/iomap/direct-io.c:__iomap_dio_rw() does.

Regardless, the most likely outcome from issing what should be aligned DIO only to get -EINVAL is exactly what is addressed with this _unlikely_ error handling: misaligned IO..

Quoted 10 lines · Mike Snitzer

Quoted 5 lines

Can you define synchronous better here? The term is unfortunately overloaded between synchronous syscalls vs aio/io_uring and O_(D)SYNC style I/O. As of now I don't understand which one you mean, especially with the DONTCACHE reference thrown in, but I guess I'll figure it out reading the patch.

It clearly talks about synchronous IO. DONTCACHE just happens to be the buffered IO that'll be used if supported.

I don't see anything that needs changing here.

Again, what synchronous? O_(D)SYNC, or !async?

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

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.

Did you see -ENOTBLK leaking out of the file systems? Because at least for iomap it is supposed to be an indication that the file system ->write_iter handler needs to retry using buffered I/O and never leak to the caller.

Quoted 1 line

fs/iomap/direct-io.c:__iomap_dio_rw() will return -ENOTBLK on its own.

Yes, and the callers in the file system methods are supposed to handle that and fall back to buffered I/O. Take a look at xfs_file_write_iter.

If this leaks out of ->write_iter we have a problem that needs fixing in the affected file system.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 2 lines

I don't understand this paragraph. What does starting point mean here? How does it matter for the patch description?

This patch's NFSD changes were starting point for NFS commit c817248fc831

But that commit is already in? This sentence really confuses me.

Quoted 2 lines

Would prefer that cleanup, if done, to happen with an incremental follow-up patch.

Starting out with well structured code is generally a good idea :(

Quoted 7 lines

Quoted 6 lines

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1291

Quoted 3 lines

fs/nfsd/vfs.c +3 -0

+static bool
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
+                        unsigned int len_mask)

Wouldn't it make sense to track the alignment when building the bio_vec array instead of doing another walk here touching all cache lines?

Quoted 5 lines

Yes, that is the conventional wisdom that justified Keith removing iov_iter_aligned. But for NFSD's WRITE payload the bio_vec is built outside of the fs/nfsd/vfs.c code. Could be there is a natural way to clean this up (to make the sunrpc layer to conditionally care about alignment) but I didn't confront that yet.

Well, for the block code it's also build outside the layer consuming it. But Keith showed that you can easily communicate that information and avoid extra loops touching the cache lines.

Quoted 10 lines

Room for follow-on improvement to be sure.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (unlikely(dio_blocksize > PAGE_SIZE))
+		return false;

Why does this matter? Can you add a comment explaining it?

I did/do have concerns that a bug in the storage driver could expose dio_offset_align that is far too large and so bounded dio_blocksize to a conservative size. What is a better constant to check?

I don't think there is a good upper limit, we not do supper LBA sizes larger than PAGE_SIZE, although I don't think there's any shipping devices that do that. But the most important thing is to always add a comment with the rationale for such non-obvious checks so that someone who has to lift it later can understand why it is done.

Quoted 8 lines

Quoted 7 lines

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 3 lines

fs/nfsd/vfs.c +3 -0

+	 * Also update @stable_how to avoid need for COMMIT.
+	 */
+	kiocb->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

Quoted 1 line

The IO needs to have reached stable storage.

Please spell that out. Because that's different from how completed is generally used in file systems and storage protocols.

Quoted 4 lines

Quoted 3 lines

Also IOCB_SYNC is wrong here, as the only thing it does over IOCB_DSYNC is also forcing back of metadata not needed to find data (aka timestamps), which I can't see any need for here.

Quoted 2 lines

Well, we're eliding any followup SYNC from the NFS client by setting stable_how to NFS_FILE_SYNC, so I made sure to use SYNC:

Why do you care about timestamps reaching stable storage? Which is the only practical extra thing IOCB_SYNC will give you. If there is a good reason for that, add a comment because that's a bit unexpected (and in general IOCB_SYNC / O_SYNC is just a sign that someone does not know about SYNC vs DSYNC semantics).

Quoted 4 lines

Quoted 2 lines

The VFS certainly does not, and if it leaks out of a specific file system we need to fix that.

As I said above, fs/iomap/direct-io.c:__iomap_dio_rw() does.

As stated above that's an internal helper and the code is supposed to be handled by the file system and never leak out.

Quoted 3 lines

Regardless, the most likely outcome from issing what should be aligned DIO only to get -EINVAL is exactly what is addressed with this _unlikely_ error handling: misaligned IO..

No. There is no reason why this is any more likely than any of the other -EINVAL cases.

What I'm hearing is that nfsd_vfs_write() should never see -ENOTBLK, so this check is essentially dead code.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

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.

I don't understand this paragraph. What does starting point mean here? How does it matter for the patch description?

Quoted 4 lines

Quoted 2 lines

This patch's NFSD changes were starting point for NFS commit c817248fc831

But that commit is already in? This sentence really confuses me.

I'm not convinced this paragraph in the patch description is needed. Can it be removed?

(Perhaps to get us started, Christoph, do you know of specific code that shows how this code could be reorganized?)

If NFSD responds with NFS_FILE_SYNC here, then timestamps need to be persisted as well as data. As discussed in other threads, currently nfsd_vfs_write() seems to miss that mark.

So either: return NFS_DATA_SYNC (which means we've persisted only the data) or make this path persist the timestamps too.

I haven't seen any existing code that convinces me that setting both IOCB_SYNC and IOCB_DSYNC is necessary.

Quoted 7 lines · Chuck Lever

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.

Can you define synchronous better here? The term is unfortunately overloaded between synchronous syscalls vs aio/io_uring and O_(D)SYNC style I/O. As of now I don't understand which one you mean, especially with the DONTCACHE reference thrown in, but I guess I'll figure it out reading the patch.

Quoted 3 lines

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.

Did you see -ENOTBLK leaking out of the file systems? Because at least for iomap it is supposed to be an indication that the file system ->write_iter handler needs to retry using buffered I/O and never leak to the caller.

Quoted 5 lines

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.

I don't understand this paragraph. What does starting point mean here? How does it matter for the patch description?

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1261

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+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 */
+};

Looking at how the code is structured later on, it seems like it would work much better if each of these sections had it's own object with the len, iov_iter, flag if it's aligned, etc. Otherwise we have this structure and lots of arrays of three items passed around.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1291

Quoted 3 lines

fs/nfsd/vfs.c +3 -0

+static bool
+nfsd_iov_iter_aligned_bvec(const struct iov_iter *i, unsigned int addr_mask,
+                        unsigned int len_mask)

Wouldn't it make sense to track the alignment when building the bio_vec array instead of doing another walk here touching all cache lines?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (unlikely(dio_blocksize > PAGE_SIZE))
+		return false;

Why does this matter? Can you add a comment explaining it?

Quoted 5 lines

fs/nfsd/vfs.c +5 -0

+static int
+nfsd_buffered_write(struct svc_rqst *rqstp, struct file *file,
+		    unsigned int nvecs, unsigned long *cnt,
+		    struct kiocb *kiocb)
+{

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1374

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+	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;

Nothing really buffered here per se, it's just a small wrapper around vfs_iocb_iter_write.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 6 lines

fs/nfsd/vfs.c +6 -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 | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

Also IOCB_SYNC is wrong here, as the only thing it does over IOCB_DSYNC is also forcing back of metadata not needed to find data (aka timestamps), which I can't see any need for here.

Quoted 6 lines

fs/nfsd/vfs.c +6 -0

+	*stable_how = NFS_FILE_SYNC;
+
+	*cnt = 0;
+	for (int i = 0; i < n_iters; i++) {
+		if (iter_is_dio_aligned[i])
+			kiocb->ki_flags |= IOCB_DIRECT;

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1420

Quoted 10 lines

fs/nfsd/vfs.c +10 -0

+		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)) {

The VFS certainly does not, and if it leaks out of a specific file system we need to fix that.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1431

Quoted 6 lines

fs/nfsd/vfs.c +6 -0

+			} 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;

-EINVAL can be lot more things than alignment failure. And more
importantly alignment failures should not happen with the proper checks in place.

Responding to a WRITE with NFS_FILE_SYNC flag set means that the data the client wrote is now on stable storage (and hence the client doesn't need to follow up with a COMMIT). This patch is using DIO for the aligned middle section but any unaligned ends use buffered I/O. If we want to return NFS_FILE_SYNC here then all of the data and metadata need to be on disk when the reply goes out.

Don't we need IOCB_SYNC here in this case? Otherwise, the server could crash while the metadata is still in memory. When it comes back up, the client could see stale timestamps. Maybe that's not fatal, but it seems pretty sketchy.

Quoted 7 lines · Jeff Layton

Responding to a WRITE with NFS_FILE_SYNC flag set means that the data the client wrote is now on stable storage (and hence the client doesn't need to follow up with a COMMIT). This patch is using DIO for the aligned middle section but any unaligned ends use buffered I/O. If we want to return NFS_FILE_SYNC here then all of the data and metadata need to be on disk when the reply goes out.

Quoted 4 lines

Don't we need IOCB_SYNC here in this case? Otherwise, the server could crash while the metadata is still in memory. When it comes back up, the client could see stale timestamps. Maybe that's not fatal, but it seems pretty sketchy.

Why do you care about the timestamps which in NFS AFAIK aren't tied to the data writeback in any way?

Cache coherency. If the timestamps roll back, some clients may not notice that data has changed after a write.

It's not the end of the world -- non-evident timestamp changes were typical prior to the multigrain timestamp patches going in, but it's not ideal.

Quoted 6 lines · Jeff Layton

Cache coherency. If the timestamps roll back, some clients may not notice that data has changed after a write.

It's not the end of the world -- non-evident timestamp changes were typical prior to the multigrain timestamp patches going in, but it's not ideal.

Well, in that case nfsd_vfs_write needs to use IOCB_SYNC as well. And both that and this code need a big fat comment about it.

Quoted 6 lines · Jeff Layton

Cache coherency. If the timestamps roll back, some clients may not notice that data has changed after a write.

It's not the end of the world -- non-evident timestamp changes were typical prior to the multigrain timestamp patches going in, but it's not ideal.

Well, in that case nfsd_vfs_write needs to use IOCB_SYNC as well. And both that and this code need a big fat comment about it.

Quoted 10 lines · Christoph Hellwig

Quoted 6 lines · Jeff Layton

Cache coherency. If the timestamps roll back, some clients may not notice that data has changed after a write.

It's not the end of the world -- non-evident timestamp changes were typical prior to the multigrain timestamp patches going in, but it's not ideal.

Well, in that case nfsd_vfs_write needs to use IOCB_SYNC as well. And both that and this code need a big fat comment about it.

NFSD's historical behavior doesn't match the spec, so yes, I'd like a comment or two that calls this out if we agree that's something we do not want to change.

Otherwise, I'm not opposed to bringing NFSD into compliance. But that might need patches that can be backported. Not technically difficult, but some planning is necessary.

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 6 lines

fs/nfsd/vfs.c +6 -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 | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

Quoted 9 lines · Christoph Hellwig

fs/nfsd/vfs.c static int wait_for_concurrent_writes(struct file *file) · line 1404

Quoted 6 lines

fs/nfsd/vfs.c +6 -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 | IOCB_SYNC);

What do you mean with completed before returning? I guess you mean writeback actually happening, right? Why do you need that, why do you also force it for the direct I/O?

This is the only comment where I'm not clear on what corrective action to take.

I think IOCB_SYNC would be needed with O_DIRECT to force timestamp updates. Otherwise, IOCB_SYNC is relevant only when the function is forced to fall back to some form of write through the page cache.