Skip to contentSkip to conversation

NFSD: Decouple the write path from NFSv3 XDR constants

PATCH New Chuck Lever 1 participant 3 messages Jul 23

Subsystems (3)
NFS client NFS server Tracing
lore
Share & tools
lore

Chuck Lever

author maintainer
#0 lore raw

cover NFSD: Decouple the write path from NFSv3 XDR constants

NFSD's internal write helpers are meant to be NFS-version-agnostic, yet nfsd_write() has taken an NFSv3 stable_how value lifted straight off the wire, and the NFSv4 WRITE and COPY paths borrow those same NFSv3 constants for their own stable values, correct only because stable_how and stable_how4 happen to share numeric values. That coincidence is the debt: it binds version-neutral VFS helpers to one protocol version's XDR and leaves NFSv4 depending on nfs3.h.

To remove the API's dependence on version-specific wire protocol, change the generic VFS helpers to take IOCB flags instead of a stable_how value. This removes a dependency on the nfs3.h header in code that is not related to NFSv3.

Chuck Lever (2):
NFS: Move definition of enum nfs3_stable_how
NFSD: Replace nfsd_write()'s "stable" argument with "iocb_flags"

 fs/nfsd/nfs3proc.c       | 17 ++++++++++++++++-
 fs/nfsd/nfs3xdr.c        |  2 ++
 fs/nfsd/nfs4proc.c       | 18 ++++++++++++++++--
 fs/nfsd/nfs4xdr.c        |  2 +-
 fs/nfsd/nfsproc.c        |  2 +-
 fs/nfsd/vfs.c            | 30 ++++++++++--------------------
 fs/nfsd/vfs.h            |  6 ++++--
 fs/nfsd/xdr3.h           |  2 +-
 include/linux/nfs.h      |  9 ---------
 include/linux/nfs3.h     |  8 ++++++++
 include/linux/nfs4.h     |  6 ++++++
 include/trace/misc/nfs.h |  1 +
 12 files changed, 66 insertions(+), 37 deletions(-)
Signature · 1 line
2.54.0
author maintainer
#1 lore raw

PATCH 1/2 NFS: Move definition of enum nfs3_stable_how

Clean up: enum nfs3_stable_how was introduced in NFSv3. NFSv2 has no stable_how on the wire; its write path passes NFS_FILE_SYNC only as a placeholder that the protocol ignores. The stable_how constants describe an NFSv3 wire value, so they belong in linux/nfs3.h.

Signed-off-by Chuck Lever <[email protected]>
---
 include/linux/nfs.h      | 9 ---------
 include/linux/nfs3.h     | 8 ++++++++
 include/trace/misc/nfs.h | 1 +
 3 files changed, 9 insertions(+), 9 deletions(-)
include/linux/nfs.h +0 -9
index 0e2a0b1e3061..0e2b210c103b 100644
@@ -22,13 +22,4 @@
 #define LOCALIOPROC_NULL		0
 #define LOCALIOPROC_UUID_IS_LOCAL	1
 
-enum nfs3_stable_how {
-	NFS_UNSTABLE = 0,
-	NFS_DATA_SYNC = 1,
-	NFS_FILE_SYNC = 2,
-
-	/* used by direct.c to mark verf as invalid */
-	NFS_INVALID_STABLE_HOW = -1
-};
-
 #endif /* _LINUX_NFS_H */
include/linux/nfs3.h +8 -0
index 404b8f724fc9..1d18da0860d5 100644
@@ -7,6 +7,14 @@
 
 #include <uapi/linux/nfs3.h>
 
+enum nfs3_stable_how {
+	NFS_UNSTABLE = 0,
+	NFS_DATA_SYNC = 1,
+	NFS_FILE_SYNC = 2,
+
+	/* used to mark verf as invalid */
+	NFS_INVALID_STABLE_HOW = -1
+};
 
 /* Number of 32bit words in post_op_attr */
 #define NFS3_POST_OP_ATTR_WORDS		22
include/trace/misc/nfs.h +1 -0
index a394b4d38e18..b5fb77d7954b 100644
@@ -8,6 +8,7 @@
  */
 
 #include <linux/nfs.h>
+#include <linux/nfs3.h>
 #include <linux/nfs4.h>
 #include <uapi/linux/nfs.h>
Signature · 1 line
2.54.0
author maintainer
#2 lore raw

PATCH 2/2 NFSD: Replace nfsd_write()'s "stable" argument with "iocb_flags"

In reply to Chuck Lever

The current nfsd_write() API is not NFS version-agnostic, as it relies on callers to pass an NFSv3 stable_how value to determine the persistence of the requested WRITE. NFSv2 does not use a stable-how value on the wire, and NFSv4 has its own stable_how4 (though stable_how and stable_how4 happen to share the same numeric values).

To remove the dependence on NFSv3-specific XDR values from NFSD's generic VFS APIs, replace nfsd_write()'s stable argument with an argument that passes a set of IOCB flags instead of an XDR-defined value.

The NFSv4 WRITE and COPY paths had been borrowing the NFSv3 stable_how constants for their own on-the-wire stable values, relying on the numeric coincidence noted above. Convert those sites to the stable_how4 enumerators so the v4 code expresses its own protocol's values directly, with no change in behavior.

While here, bound-check the decoded NFSv3 WRITE stable value, as the NFSv4 WRITE decoder already does, and make the nfsd3_writeargs stable field unsigned to suit.

The larger benefit is one less NFSv4 dependency on nfs3.h.

Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs3proc.c   | 17 ++++++++++++++++-
 fs/nfsd/nfs3xdr.c    |  2 ++
 fs/nfsd/nfs4proc.c   | 18 ++++++++++++++++--
 fs/nfsd/nfs4xdr.c    |  2 +-
 fs/nfsd/nfsproc.c    |  2 +-
 fs/nfsd/vfs.c        | 30 ++++++++++--------------------
 fs/nfsd/vfs.h        |  6 ++++--
 fs/nfsd/xdr3.h       |  2 +-
 include/linux/nfs4.h |  6 ++++++
 9 files changed, 57 insertions(+), 28 deletions(-)
fs/nfsd/nfs3proc.c +16 -1
index 4b3075c05b97..19ab0a713d82 100644
@@ -49,6 +49,20 @@ static bool nfsd3_time_in_range(const struct iattr *iap)
 	return true;
 }
 
+static int nfsd3_iocb_flags(enum nfs3_stable_how how)
+{
+	switch (how) {
+	case NFS_FILE_SYNC:
+		/* persist data and timestamps */
+		return IOCB_DSYNC | IOCB_SYNC;
+	case NFS_DATA_SYNC:
+		/* persist data only */
+		return IOCB_DSYNC;
+	default:
+		return 0;
+	}
+}
+
 static __be32 nfsd3_map_status(__be32 status)
 {
 	switch (status) {

@@ -261,7 +275,8 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
 	resp->committed = argp->stable;
 	resp->status = nfsd_write(rqstp, &resp->fh, argp->offset,
 				  &argp->payload, &cnt,
-				  resp->committed, resp->verf);
+				  nfsd3_iocb_flags(resp->committed),
+				  resp->verf);
 	resp->count = cnt;
 	resp->status = nfsd3_map_status(resp->status);
 	return rpc_success;
fs/nfsd/nfs3xdr.c +2 -0
index 196bcc6edebb..090cea8e545d 100644
@@ -557,6 +557,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 		return false;
 	if (xdr_stream_decode_u32(xdr, &args->stable) < 0)
 		return false;
+	if (args->stable > NFS_FILE_SYNC)
+		return false;
 
 	/* opaque data */
 	if (xdr_stream_decode_u32(xdr, &args->len) < 0)
fs/nfsd/nfs4proc.c +16 -2
index a39880208761..a2ff1c8f415b 100644
@@ -72,6 +72,20 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
 
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
+static int nfsd4_iocb_flags(enum stable_how4 how)
+{
+	switch (how) {
+	case FILE_SYNC4:
+		/* persist data and timestamps */
+		return IOCB_DSYNC | IOCB_SYNC;
+	case DATA_SYNC4:
+		/* persist data only */
+		return IOCB_DSYNC;
+	default:
+		return 0;
+	}
+}
+
 static u32 nfsd_attrmask[] = {
 	NFSD_WRITEABLE_ATTRS_WORD0,
 	NFSD_WRITEABLE_ATTRS_WORD1,

@@ -1417,7 +1431,7 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	write->wr_how_written = write->wr_stable_how;
 	status = nfsd_vfs_write(rqstp, &cstate->current_fh, nf,
 				write->wr_offset, &write->wr_payload,
-				&cnt, write->wr_how_written,
+				&cnt, nfsd4_iocb_flags(write->wr_how_written),
 				(__be32 *)write->wr_verifier.data);
 	nfsd_file_put(nf);
 

@@ -2001,7 +2015,7 @@ static void nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
 {
 	copy->cp_res.wr_stable_how =
 		test_bit(NFSD4_COPY_F_COMMITTED, &copy->cp_flags) ?
-			NFS_FILE_SYNC : NFS_UNSTABLE;
+			FILE_SYNC4 : UNSTABLE4;
 	nfsd4_copy_set_sync(copy, sync);
 }
fs/nfsd/nfs4xdr.c +1 -1
index 7ccc7c897b00..a47eb544b99f 100644
@@ -1607,7 +1607,7 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
 		return nfserr_bad_xdr;
 	if (xdr_stream_decode_u32(argp->xdr, &write->wr_stable_how) < 0)
 		return nfserr_bad_xdr;
-	if (write->wr_stable_how > NFS_FILE_SYNC)
+	if (write->wr_stable_how > FILE_SYNC4)
 		return nfserr_bad_xdr;
 	if (xdr_stream_decode_u32(argp->xdr, &write->wr_buflen) < 0)
 		return nfserr_bad_xdr;
fs/nfsd/nfsproc.c +1 -1
index 919acfba356a..48541ef7644f 100644
@@ -266,7 +266,7 @@ nfsd_proc_write(struct svc_rqst *rqstp)
 
 	fh_copy(&resp->fh, &argp->fh);
 	resp->status = nfsd_write(rqstp, &resp->fh, argp->offset,
-				  &argp->payload, &cnt, NFS_DATA_SYNC, NULL);
+				  &argp->payload, &cnt, IOCB_DSYNC, NULL);
 	if (resp->status == nfs_ok)
 		resp->status = fh_getattr(&resp->fh, &resp->stat);
 	else if (resp->status == nfserr_jukebox)
fs/nfsd/vfs.c +10 -20
index fdfcd95971db..edb08c006a23 100644
@@ -1433,7 +1433,7 @@ nfsd_direct_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
  * @offset: Byte offset of start
  * @payload: xdr_buf containing the write payload
  * @cnt: IN: number of bytes to write, OUT: number of bytes actually written
- * @stable: An NFS stable_how value
+ * @iocb_flags: VFS IOCB_* flags expressing the requested write stability
  * @verf: NFS WRITE verifier
  *
  * Upon return, caller must invoke fh_put on @fhp.

@@ -1445,7 +1445,7 @@ __be32
 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	       struct nfsd_file *nf, loff_t offset,
 	       const struct xdr_buf *payload, unsigned long *cnt,
-	       int stable, __be32 *verf)
+	       int iocb_flags, __be32 *verf)
 {
 	struct nfsd_net		*nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct file		*file = nf->nf_file;

@@ -1482,21 +1482,11 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	exp = fhp->fh_export;
 
 	if (!EX_ISSYNC(exp))
-		stable = NFS_UNSTABLE;
+		iocb_flags = 0;
 	init_sync_kiocb(&kiocb, file);
 	kiocb.ki_pos = offset;
-	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;
-		}
-	}
+	if (likely(!fhp->fh_use_wgather))
+		kiocb.ki_flags |= iocb_flags;
 
 	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
 	if (nvecs < 0) {

@@ -1537,7 +1527,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out_nfserr;
 	}
 
-	if (stable && fhp->fh_use_wgather) {
+	if (iocb_flags && fhp->fh_use_wgather) {
 		host_err = wait_for_concurrent_writes(file);
 		if (host_err < 0)
 			commit_reset_write_verifier(nn, rqstp, host_err);

@@ -1628,7 +1618,7 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
  * @offset: Byte offset of start
  * @payload: xdr_buf containing the write payload
  * @cnt: IN: number of bytes to write, OUT: number of bytes actually written
- * @stable: An NFS stable_how value
+ * @iocb_flags: VFS IOCB_* flags expressing the requested write stability
  * @verf: NFS WRITE verifier
  *
  * Upon return, caller must invoke fh_put on @fhp.

@@ -1638,8 +1628,8 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
  */
 __be32
 nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t offset,
-	   const struct xdr_buf *payload, unsigned long *cnt, int stable,
-	   __be32 *verf)
+	   const struct xdr_buf *payload, unsigned long *cnt,
+	   int iocb_flags, __be32 *verf)
 {
 	struct nfsd_file *nf;
 	__be32 err;

@@ -1651,7 +1641,7 @@ nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t offset,
 		goto out;
 
 	err = nfsd_vfs_write(rqstp, fhp, nf, offset, payload, cnt,
-			     stable, verf);
+			     iocb_flags, verf);
 	nfsd_file_put(nf);
 out:
 	trace_nfsd_write_done(rqstp, fhp, offset, *cnt);
fs/nfsd/vfs.h +4 -2
index 5554878781f4..aa7679d4c54a 100644
@@ -135,11 +135,13 @@ __be32		nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				u32 *eof);
 __be32		nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				loff_t offset, const struct xdr_buf *payload,
-				unsigned long *cnt, int stable, __be32 *verf);
+				unsigned long *cnt, int iocb_flags,
+				__be32 *verf);
 __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				struct nfsd_file *nf, loff_t offset,
 				const struct xdr_buf *payload,
-				unsigned long *cnt, int stable, __be32 *verf);
+				unsigned long *cnt, int iocb_flags,
+				__be32 *verf);
 __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
 				char *, int *);
 __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
fs/nfsd/xdr3.h +1 -1
index 344203874b4c..cad875d14231 100644
@@ -39,7 +39,7 @@ struct nfsd3_writeargs {
 	svc_fh			fh;
 	__u64			offset;
 	__u32			count;
-	int			stable;
+	__u32			stable;
 	__u32			len;
 	struct xdr_buf		payload;
 };
include/linux/nfs4.h +6 -0
index 44e5e9fa12e1..9333f2716b4d 100644
@@ -263,6 +263,12 @@ enum why_no_delegation4 { /* new to v4.1 */
 	WND4_IS_DIR = 8,
 };
 
+enum stable_how4 {
+	UNSTABLE4	= 0,
+	DATA_SYNC4	= 1,
+	FILE_SYNC4	= 2,
+};
+
 enum lock_type4 {
 	NFS4_UNLOCK_LT = 0,
 	NFS4_READ_LT = 1,
Signature · 1 line
2.54.0