[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.

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 73730222a7d33f38faa820619b5bed4cabaa2fc7..23442b1767d5bf055cc22f451c6b07d3d7239331 100644 (file)
--- a/fs/nfsd/nfs4proc.c
+++ b/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);

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7f1abbc5aa904fed6e91cda240c8b8f33039b77f..5ded0c0f70a39b8e585a256051cda8cba6941c89 100644 (file)
--- a/fs/nfsd/nfs4state.c
+++ b/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 @@ instantiate_new_owner:
 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 @@ out:
        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 @@ check_replay:
        } 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 @@ out_destroy_new_stateid:
                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:

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ad49241d0b2f075f8b7781a25cf3975d3b68827f..b98b5fe49efce355a135b525d7dd6fee1bf4148c 100644 (file)
--- a/fs/nfsd/nfs4xdr.c
+++ b/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();
 }
 
 /*