[PATCH] kNFSdv4: Improve how locking copes with replays

From: NeilBrown <neilb@cse.unsw.edu.au>

From: "J. Bruce Fields" <bfields@fieldses.org>

From: Andros: Hold state_lock longer so the stateowner doesn't diseappear
out from under us before we get the chance to encode the replay.  Don't
attempt to save replay if we failed to find a stateowner.

fs/nfsd/nfs4proc.c

@@ -113,17 +113,24 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 	return status;
 }
 
+/*
+ * nfs4_unlock_state() called in encode
+ */
 static inline int
 nfsd4_open(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
 {
 	int status;
-	dprintk("NFSD: nfsd4_open filename %.*s\n",
-		(int)open->op_fname.len, open->op_fname.data);
+	dprintk("NFSD: nfsd4_open filename %.*s op_stateowner %p\n",
+		(int)open->op_fname.len, open->op_fname.data,
+		open->op_stateowner);
 
 	/* This check required by spec. */
 	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
 		return nfserr_inval;
 
+	open->op_stateowner = NULL;
+	nfs4_lock_state();
+
 	/* check seqid for replay. set nfs4_owner */
 	status = nfsd4_process_open1(open);
 	if (status == NFSERR_REPLAY_ME) {
@@ -761,7 +768,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			break;
 		case OP_CLOSE:
 			op->status = nfsd4_close(rqstp, &current_fh, &op->u.close);
-			op->replay = &op->u.close.cl_stateowner->so_replay;
+			if (op->u.close.cl_stateowner)
+				op->replay =
+					&op->u.close.cl_stateowner->so_replay;
 			break;
 		case OP_COMMIT:
 			op->status = nfsd4_commit(rqstp, &current_fh, &op->u.commit);
@@ -780,14 +789,18 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			break;
 		case OP_LOCK:
 			op->status = nfsd4_lock(rqstp, &current_fh, &op->u.lock);
-			op->replay = &op->u.lock.lk_stateowner->so_replay;
+			if (op->u.lock.lk_stateowner)
+				op->replay =
+					&op->u.lock.lk_stateowner->so_replay;
 			break;
 		case OP_LOCKT:
 			op->status = nfsd4_lockt(rqstp, &current_fh, &op->u.lockt);
 			break;
 		case OP_LOCKU:
 			op->status = nfsd4_locku(rqstp, &current_fh, &op->u.locku);
-			op->replay = &op->u.locku.lu_stateowner->so_replay;
+			if (op->u.locku.lu_stateowner)
+				op->replay =
+					&op->u.locku.lu_stateowner->so_replay;
 			break;
 		case OP_LOOKUP:
 			op->status = nfsd4_lookup(rqstp, &current_fh, &op->u.lookup);
@@ -802,15 +815,21 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			break;
 		case OP_OPEN:
 			op->status = nfsd4_open(rqstp, &current_fh, &op->u.open);
-			op->replay = &op->u.open.op_stateowner->so_replay;
+			if (op->u.open.op_stateowner)
+				op->replay =
+					&op->u.open.op_stateowner->so_replay;
 			break;
 		case OP_OPEN_CONFIRM:
 			op->status = nfsd4_open_confirm(rqstp, &current_fh, &op->u.open_confirm);
-			op->replay = &op->u.open_confirm.oc_stateowner->so_replay;
+			if (op->u.open_confirm.oc_stateowner)
+				op->replay =
+					&op->u.open_confirm.oc_stateowner->so_replay;
 			break;
 		case OP_OPEN_DOWNGRADE:
 			op->status = nfsd4_open_downgrade(rqstp, &current_fh, &op->u.open_downgrade);
-			op->replay = &op->u.open_downgrade.od_stateowner->so_replay;
+			if (op->u.open_downgrade.od_stateowner)
+				op->replay =
+					&op->u.open_downgrade.od_stateowner->so_replay;
 			break;
 		case OP_PUTFH:
 			op->status = nfsd4_putfh(rqstp, &current_fh, &op->u.putfh);

fs/nfsd/nfs4state.c

@@ -89,6 +89,9 @@ nfs4_lock_state(void)
 	down(&client_sema);
 }
 
+/*
+ * nfs4_unlock_state(); called in encode
+ */
 void
 nfs4_unlock_state(void)
 {
@@ -1069,6 +1072,8 @@ nfs4_file_downgrade(struct file *filp, unsigned int share_access)
  * 		notfound:
  * 			verify clientid
  * 			create new owner
+ *
+ * called with nfs4_lock_state() held.
  */
 int
 nfsd4_process_open1(struct nfsd4_open *open)
@@ -1087,7 +1092,6 @@ nfsd4_process_open1(struct nfsd4_open *open)
 	if (STALE_CLIENTID(&open->op_clientid))
 		goto out;
 
-	nfs4_lock_state();
 	strhashval = ownerstr_hashval(clientid->cl_id, open->op_owner);
 	if (find_openstateowner_str(strhashval, open, &sop)) {
 		open->op_stateowner = sop;
@@ -1145,10 +1149,11 @@ nfsd4_process_open1(struct nfsd4_open *open)
 renew:
 	renew_client(sop->so_client);
 out:
-	nfs4_unlock_state();
 	return status;
 }
-
+/*
+ * called with nfs4_lock_state() held.
+ */
 int
 nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
 {
@@ -1170,7 +1175,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	if (!TEST_ACCESS(open->op_share_access) || !TEST_DENY(open->op_share_deny))
 		goto out;
 
-	nfs4_lock_state();
 	fi_hashval = file_hashval(ino);
 	if (find_file(fi_hashval, ino, &fp)) {
 		/* Search for conflicting share reservations */
@@ -1261,7 +1265,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	if (!open->op_stateowner->so_confirmed)
 		open->op_rflags |= NFS4_OPEN_RESULT_CONFIRM;
 
-	nfs4_unlock_state();
 	return status;
 out_free:
 	kfree(stp);
@@ -1576,11 +1579,15 @@ nfs4_preprocess_seqid_op(struct svc_fh *current_fh, u32 seqid, stateid_t *statei
 	} else  {
 		printk("NFSD: preprocess_seqid_op: bad seqid (expected %d, got %d\n", sop->so_seqid +1, seqid);
 
+		*sopp = NULL;
 		status = nfserr_bad_seqid;
 	}
 	goto out;
 }
 
+/*
+ * nfs4_unlock_state(); called in encode
+ */
 int
 nfsd4_open_confirm(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open_confirm *oc)
 {
@@ -1616,7 +1623,6 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 		         stp->st_stateid.si_generation);
 	status = nfs_ok;
 out:
-	nfs4_unlock_state();
 	return status;
 }
 
@@ -1645,6 +1651,9 @@ reset_union_bmap_deny(unsigned long deny, unsigned long *bmap)
 	}
 }
 
+/*
+ * nfs4_unlock_state(); called in encode
+ */
 
 int
 nfsd4_open_downgrade(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open_downgrade *od)
@@ -1657,6 +1666,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct n
 			(int)current_fh->fh_dentry->d_name.len,
 			current_fh->fh_dentry->d_name.name);
 
+	od->od_stateowner = NULL;
 	status = nfserr_inval;
 	if (!TEST_ACCESS(od->od_share_access) || !TEST_DENY(od->od_share_deny))
 		goto out;
@@ -1690,10 +1700,12 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct n
 	memcpy(&od->od_stateid, &stp->st_stateid, sizeof(stateid_t));
 	status = nfs_ok;
 out:
-	nfs4_unlock_state();
 	return status;
 }
 
+/*
+ * nfs4_unlock_state() called after encode
+ */
 int
 nfsd4_close(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_close *close)
 {
@@ -1704,6 +1716,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_clos
 			(int)current_fh->fh_dentry->d_name.len,
 			current_fh->fh_dentry->d_name.name);
 
+	close->cl_stateowner = NULL;
 	nfs4_lock_state();
 	/* check close_lru for replay */
 	if ((status = nfs4_preprocess_seqid_op(current_fh, close->cl_seqid, 
@@ -1721,7 +1734,6 @@ nfsd4_close(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_clos
 	/* release_state_owner() calls nfsd_close() if needed */
 	release_state_owner(stp, &close->cl_stateowner, OPEN_STATE);
 out:
-	nfs4_unlock_state();
 	return status;
 }
 
@@ -1924,6 +1936,8 @@ check_lock_length(u64 offset, u64 length)
 
 /*
  *  LOCK operation 
+ *
+ * nfs4_unlock_state(); called in encode
  */
 int
 nfsd4_lock(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock *lock)
@@ -2088,7 +2102,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
 		release_state_owner(lock_stp, &lock->lk_stateowner, LOCK_STATE);
 	}
 out:
-	nfs4_unlock_state();
 	return status;
 }
 
@@ -2199,6 +2212,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
 	if (check_lock_length(locku->lu_offset, locku->lu_length))
 		 return nfserr_inval;
 
+	locku->lu_stateowner = NULL;
 	nfs4_lock_state();
 									        
 	if ((status = nfs4_preprocess_seqid_op(current_fh, 
@@ -2241,7 +2255,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
 	memcpy(&locku->lu_stateid, &stp->st_stateid, sizeof(stateid_t));
 
 out:
-	nfs4_unlock_state();
 	return status;
 
 out_nfserr:

fs/nfsd/nfs4xdr.c

@@ -484,11 +484,14 @@ nfsd4_decode_access(struct nfsd4_compoundargs *argp, struct nfsd4_access *access
 	DECODE_TAIL;
 }
 
+#define NFS4_STATE_NOT_LOCKED	((void *)-1)
+
 static int
 nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
 {
 	DECODE_HEAD;
 
+	close->cl_stateowner = NFS4_STATE_NOT_LOCKED;
 	READ_BUF(4 + sizeof(stateid_t));
 	READ32(close->cl_seqid);
 	READ32(close->cl_stateid.si_generation);
@@ -579,6 +582,7 @@ nfsd4_decode_lock(struct nfsd4_compoundargs *argp, struct nfsd4_lock *lock)
 {
 	DECODE_HEAD;
 
+	lock->lk_stateowner = NFS4_STATE_NOT_LOCKED;
 	/*
 	* type, reclaim(boolean), offset, length, new_lock_owner(boolean)
 	*/
@@ -636,6 +640,7 @@ nfsd4_decode_locku(struct nfsd4_compoundargs *argp, struct nfsd4_locku *locku)
 {
 	DECODE_HEAD;
 
+	locku->lu_stateowner = NFS4_STATE_NOT_LOCKED;
 	READ_BUF(24 + sizeof(stateid_t));
 	READ32(locku->lu_type);
 	if ((locku->lu_type < NFS4_READ_LT) || (locku->lu_type > NFS4_WRITEW_LT))
@@ -671,6 +676,7 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 
 	memset(open->op_bmval, 0, sizeof(open->op_bmval));
 	open->op_iattr.ia_valid = 0;
+	open->op_stateowner = NFS4_STATE_NOT_LOCKED;
 
 	/* seqid, share_access, share_deny, clientid, ownerlen */
 	READ_BUF(16 + sizeof(clientid_t));
@@ -746,6 +752,7 @@ nfsd4_decode_open_confirm(struct nfsd4_compoundargs *argp, struct nfsd4_open_con
 {
 	DECODE_HEAD;
 		    
+	open_conf->oc_stateowner = NFS4_STATE_NOT_LOCKED;
 	READ_BUF(4 + sizeof(stateid_t));
 	READ32(open_conf->oc_req_stateid.si_generation);
 	COPYMEM(&open_conf->oc_req_stateid.si_opaque, sizeof(stateid_opaque_t));
@@ -759,6 +766,7 @@ nfsd4_decode_open_downgrade(struct nfsd4_compoundargs *argp, struct nfsd4_open_d
 {
 	DECODE_HEAD;
 		    
+	open_down->od_stateowner = NFS4_STATE_NOT_LOCKED;
 	READ_BUF(4 + sizeof(stateid_t));
 	READ32(open_down->od_stateid.si_generation);
 	COPYMEM(&open_down->od_stateid.si_opaque, sizeof(stateid_opaque_t));
@@ -1259,7 +1267,8 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
  */
 
 #define ENCODE_SEQID_OP_TAIL(stateowner) do {			\
-	if (seqid_mutating_err(nfserr) && stateowner) {		\
+	if (seqid_mutating_err(nfserr) && stateowner		\
+	    && (stateowner != NFS4_STATE_NOT_LOCKED)) { 	\
 		if (stateowner->so_confirmed)			\
 			stateowner->so_seqid++;			\
 		stateowner->so_replay.rp_status = nfserr;   	\
@@ -1267,7 +1276,10 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			  (((char *)(resp)->p - (char *)save)); \
 		memcpy(stateowner->so_replay.rp_buf, save,      \
  			stateowner->so_replay.rp_buflen); 	\
-	} } while(0)
+	}							\
+	if (stateowner != NFS4_STATE_NOT_LOCKED)		\
+		nfs4_unlock_state();				\
+	} while (0);
 
 
 static u32 nfs4_ftypes[16] = {
@@ -1917,7 +1929,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, int nfserr, struct nfsd4_open
 	ENCODE_SEQID_OP_HEAD;
 
 	if (nfserr)
-		return;
+		goto out;
 
 	RESERVE_SPACE(36 + sizeof(stateid_t));
 	WRITE32(open->op_stateid.si_generation);
@@ -1972,7 +1984,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, int nfserr, struct nfsd4_open
 		BUG();
 	}
 	/* XXX save filehandle here */
-
+out:
 	ENCODE_SEQID_OP_TAIL(open->op_stateowner);
 }
 
@@ -2297,14 +2309,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 
 	RESERVE_SPACE(8);
 	WRITE32(op->opnum);
-	if ((op->opnum != OP_SETATTR) && (op->opnum != OP_LOCK) && (op->opnum != OP_LOCKT) && (op->opnum != OP_SETCLIENTID) && (op->status)) {
-		*p++ = op->status;
-		ADJUST_ARGS();
-		return;
-	} else {
-		statp = p++;        /* to be backfilled at the end */
-		ADJUST_ARGS();
-	}
+	statp = p++;	/* to be backfilled at the end */
+	ADJUST_ARGS();
 
 	switch (op->opnum) {
 	case OP_ACCESS:
@@ -2408,6 +2414,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
  * 
  * XDR note: do not encode rp->rp_buflen: the buffer contains the
  * previously sent already encoded operation.
+ *
+ * called with nfs4_lock_state() held
  */
 void
 nfsd4_encode_replay(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
@@ -2425,6 +2433,7 @@ nfsd4_encode_replay(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 	RESERVE_SPACE(rp->rp_buflen);
 	WRITEMEM(rp->rp_buf, rp->rp_buflen);
 	ADJUST_ARGS();
+	nfs4_unlock_state();
 }
 
 /*