git: ead50c94cb60 - main - nfscl: Fix must_commit/writeverf handling for Direct I/O
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Sat, 11 Dec 2021 23:04:48 UTC
The branch main has been updated by rmacklem:
URL: https://cgit.FreeBSD.org/src/commit/?id=ead50c94cb604594987e6512289268891a427725
commit ead50c94cb604594987e6512289268891a427725
Author: Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-11 23:00:30 +0000
Commit: Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-11 23:00:30 +0000
nfscl: Fix must_commit/writeverf handling for Direct I/O
Without this patch, the KASSERT(must_commit == 0,..) can be
triggered by the writeverf in the Direct I/O write reply changing.
This is not a situation that should cause a panic(). Correct
handling is to ignore the change in "writeverf" for Direct
I/O, since it is done with NFSWRITE_FILESYNC.
This patch modifies the semantics of the "must_commit"
argument slightly, allowing an initial value of 2 to indicate
that a change in "writeverf" should be ignored.
It also fixes the KASSERT()s.
This bug would affect few, since Direct I/O is not enabled
by default and "writeverf" rarely changes. Normally "writeverf"
only changes when a NFS server reboots, however I found the
bug when testing against a Linux 5.15.1 kernel nfsd, which
replied to a NFSWRITE_FILESYNC write with a "writeverf" of all
0x0 bytes.
MFC after: 2 weeks
---
sys/fs/nfsclient/nfs_clbio.c | 37 +++++++++++++++++++++++++++++++++----
sys/fs/nfsclient/nfs_clrpcops.c | 14 +++++++++-----
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/sys/fs/nfsclient/nfs_clbio.c b/sys/fs/nfsclient/nfs_clbio.c
index 73f559ad82f8..29bc66669dfb 100644
--- a/sys/fs/nfsclient/nfs_clbio.c
+++ b/sys/fs/nfsclient/nfs_clbio.c
@@ -784,12 +784,26 @@ do_sync:
uio.uio_rw = UIO_WRITE;
uio.uio_td = td;
iomode = NFSWRITE_FILESYNC;
+ /*
+ * When doing direct I/O we do not care if the
+ * server's write verifier has changed, but we
+ * do not want to update the verifier if it has
+ * changed, since that hides the change from
+ * writes being done through the buffer cache.
+ * By passing must_commit in set to two, the code
+ * in nfsrpc_writerpc() will not update the
+ * verifier on the mount point.
+ */
+ must_commit = 2;
error = ncl_writerpc(vp, &uio, cred, &iomode,
&must_commit, 0);
- KASSERT((must_commit == 0),
- ("ncl_directio_write: Did not commit write"));
+ KASSERT((must_commit == 2),
+ ("ncl_directio_write: Updated write verifier"));
if (error)
return (error);
+ if (iomode != NFSWRITE_FILESYNC)
+ printf("nfs_directio_write: Broken server "
+ "did not reply FILE_SYNC\n");
uiop->uio_offset += size;
uiop->uio_resid -= size;
if (uiop->uio_iov->iov_len <= size) {
@@ -1592,8 +1606,23 @@ ncl_doio_directwrite(struct buf *bp)
iomode = NFSWRITE_FILESYNC;
uiop->uio_td = NULL; /* NULL since we're in nfsiod */
+ /*
+ * When doing direct I/O we do not care if the
+ * server's write verifier has changed, but we
+ * do not want to update the verifier if it has
+ * changed, since that hides the change from
+ * writes being done through the buffer cache.
+ * By passing must_commit in set to two, the code
+ * in nfsrpc_writerpc() will not update the
+ * verifier on the mount point.
+ */
+ must_commit = 2;
ncl_writerpc(bp->b_vp, uiop, bp->b_wcred, &iomode, &must_commit, 0);
- KASSERT((must_commit == 0), ("ncl_doio_directwrite: Did not commit write"));
+ KASSERT((must_commit == 2), ("ncl_doio_directwrite: Updated write"
+ " verifier"));
+ if (iomode != NFSWRITE_FILESYNC)
+ printf("ncl_doio_directwrite: Broken server "
+ "did not reply FILE_SYNC\n");
free(iov_base, M_NFSDIRECTIO);
free(uiop->uio_iov, M_NFSDIRECTIO);
free(uiop, M_NFSDIRECTIO);
@@ -1864,7 +1893,7 @@ ncl_doio(struct vnode *vp, struct buf *bp, struct ucred *cr, struct thread *td,
}
}
bp->b_resid = uiop->uio_resid;
- if (must_commit)
+ if (must_commit == 1)
ncl_clearcommit(vp->v_mount);
bufdone(bp);
return (error);
diff --git a/sys/fs/nfsclient/nfs_clrpcops.c b/sys/fs/nfsclient/nfs_clrpcops.c
index 08bfac3575f7..56ce2a5e2077 100644
--- a/sys/fs/nfsclient/nfs_clrpcops.c
+++ b/sys/fs/nfsclient/nfs_clrpcops.c
@@ -1857,7 +1857,8 @@ nfsrpc_write(vnode_t vp, struct uio *uiop, int *iomode, int *must_commit,
nfsv4stateid_t stateid;
void *lckp;
- *must_commit = 0;
+ KASSERT(*must_commit >= 0 && *must_commit <= 2,
+ ("nfsrpc_write: must_commit out of range=%d", *must_commit));
if (nmp->nm_clp != NULL)
clidrev = nmp->nm_clp->nfsc_clientidrev;
newcred = cred;
@@ -2070,7 +2071,7 @@ nfsrpc_writerpc(vnode_t vp, struct uio *uiop, int *iomode,
NFSX_VERF);
NFSSETWRITEVERF(nmp);
} else if (NFSBCMP(tl, nmp->nm_verf,
- NFSX_VERF)) {
+ NFSX_VERF) && *must_commit != 2) {
*must_commit = 1;
NFSBCOPY(tl, nmp->nm_verf, NFSX_VERF);
}
@@ -6814,7 +6815,8 @@ nfsrpc_writeds(vnode_t vp, struct uio *uiop, int *iomode, int *must_commit,
if (!NFSHASWRITEVERF(nmp)) {
NFSBCOPY(tl, nmp->nm_verf, NFSX_VERF);
NFSSETWRITEVERF(nmp);
- } else if (NFSBCMP(tl, nmp->nm_verf, NFSX_VERF)) {
+ } else if (NFSBCMP(tl, nmp->nm_verf, NFSX_VERF) &&
+ *must_commit != 2) {
*must_commit = 1;
NFSBCOPY(tl, nmp->nm_verf, NFSX_VERF);
}
@@ -6824,7 +6826,8 @@ nfsrpc_writeds(vnode_t vp, struct uio *uiop, int *iomode, int *must_commit,
if ((dsp->nfsclds_flags & NFSCLDS_HASWRITEVERF) == 0) {
NFSBCOPY(tl, dsp->nfsclds_verf, NFSX_VERF);
dsp->nfsclds_flags |= NFSCLDS_HASWRITEVERF;
- } else if (NFSBCMP(tl, dsp->nfsclds_verf, NFSX_VERF)) {
+ } else if (NFSBCMP(tl, dsp->nfsclds_verf, NFSX_VERF) &&
+ *must_commit != 2) {
*must_commit = 1;
NFSBCOPY(tl, dsp->nfsclds_verf, NFSX_VERF);
}
@@ -6932,7 +6935,8 @@ nfsrpc_writedsmir(vnode_t vp, int *iomode, int *must_commit,
if ((dsp->nfsclds_flags & NFSCLDS_HASWRITEVERF) == 0) {
NFSBCOPY(tl, dsp->nfsclds_verf, NFSX_VERF);
dsp->nfsclds_flags |= NFSCLDS_HASWRITEVERF;
- } else if (NFSBCMP(tl, dsp->nfsclds_verf, NFSX_VERF)) {
+ } else if (NFSBCMP(tl, dsp->nfsclds_verf, NFSX_VERF) &&
+ *must_commit != 2) {
*must_commit = 1;
NFSBCOPY(tl, dsp->nfsclds_verf, NFSX_VERF);
}