NFSv4: Recovery of recalled read delegations is broken

When a read delegation is being recalled, and we're reclaiming the
cached opens, we need to make sure that we only reclaim read-only
modes.
A previous attempt to do this, relied on retrieving the delegation
type from the nfs4_opendata structure. Unfortunately, as Kinglong
pointed out, this field can only be set when performing reboot recovery.

Furthermore, if we call nfs4_open_recover(), then we end up clobbering
the state->flags for all modes that we're not recovering...

The fix is to have the delegation recall code pass this information
to the recovery call, and then refactor the recovery code so that
nfs4_open_delegation_recall() does not need to call nfs4_open_recover().

Reported-by: Kinglong Mee <kinglongmee@gmail.com>
Fixes: 39f897fdbd46 ("NFSv4: When returning a delegation, don't...")
Tested-by: Kinglong Mee <kinglongmee@gmail.com>
Cc: NeilBrown <neilb@suse.com>
Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 2714ef835bdd4261cbc301838c050f42896c24c0..be806ead7f4d4abfde04321b78ebace12043925e 100644 (file)
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -113,7 +113,8 @@ out:
        return status;
 }
 
-static int nfs_delegation_claim_opens(struct inode *inode, const nfs4_stateid *stateid)
+static int nfs_delegation_claim_opens(struct inode *inode,
+               const nfs4_stateid *stateid, fmode_t type)
 {
        struct nfs_inode *nfsi = NFS_I(inode);
        struct nfs_open_context *ctx;
@@ -140,7 +141,7 @@ again:
                /* Block nfs4_proc_unlck */
                mutex_lock(&sp->so_delegreturn_mutex);
                seq = raw_seqcount_begin(&sp->so_reclaim_seqcount);
-               err = nfs4_open_delegation_recall(ctx, state, stateid);
+               err = nfs4_open_delegation_recall(ctx, state, stateid, type);
                if (!err)
                        err = nfs_delegation_claim_locks(ctx, state, stateid);
                if (!err && read_seqcount_retry(&sp->so_reclaim_seqcount, seq))
@@ -411,7 +412,8 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
        do {
                if (test_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
                        break;
-               err = nfs_delegation_claim_opens(inode, &delegation->stateid);
+               err = nfs_delegation_claim_opens(inode, &delegation->stateid,
+                               delegation->type);
                if (!issync || err != -EAGAIN)
                        break;
                /*

diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index a44829173e573d1ceca061a9f23b76752f7bb30e..333063e032f01813762bc89f6d6db23ddc696c92 100644 (file)
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -54,7 +54,7 @@ void nfs_delegation_reap_unclaimed(struct nfs_client *clp);
 
 /* NFSv4 delegation-related procedures */
 int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4_stateid *stateid, int issync);
-int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state *state, const nfs4_stateid *stateid);
+int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state *state, const nfs4_stateid *stateid, fmode_t type);
 int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state, const nfs4_stateid *stateid);
 bool nfs4_copy_delegation_stateid(nfs4_stateid *dst, struct inode *inode, fmode_t flags);
 

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 10ba6c1968e2110eba30488f7dc75230cf481ba8..e129347b656fe29f3d0b4e0e5ee1ec1d423d61ef 100644 (file)
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1127,6 +1127,21 @@ static int nfs4_wait_for_completion_rpc_task(struct rpc_task *task)
        return ret;
 }
 
+static bool nfs4_mode_match_open_stateid(struct nfs4_state *state,
+               fmode_t fmode)
+{
+       switch(fmode & (FMODE_READ|FMODE_WRITE)) {
+       case FMODE_READ|FMODE_WRITE:
+               return state->n_rdwr != 0;
+       case FMODE_WRITE:
+               return state->n_wronly != 0;
+       case FMODE_READ:
+               return state->n_rdonly != 0;
+       }
+       WARN_ON_ONCE(1);
+       return false;
+}
+
 static int can_open_cached(struct nfs4_state *state, fmode_t mode, int open_mode)
 {
        int ret = 0;
@@ -1571,17 +1586,13 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
        return opendata;
 }
 
-static int nfs4_open_recover_helper(struct nfs4_opendata *opendata, fmode_t fmode, struct nfs4_state **res)
+static int nfs4_open_recover_helper(struct nfs4_opendata *opendata,
+               fmode_t fmode)
 {
        struct nfs4_state *newstate;
        int ret;
 
-       if ((opendata->o_arg.claim == NFS4_OPEN_CLAIM_DELEGATE_CUR ||
-            opendata->o_arg.claim == NFS4_OPEN_CLAIM_DELEG_CUR_FH) &&
-           (opendata->o_arg.u.delegation_type & fmode) != fmode)
-               /* This mode can't have been delegated, so we must have
-                * a valid open_stateid to cover it - not need to reclaim.
-                */
+       if (!nfs4_mode_match_open_stateid(opendata->state, fmode))
                return 0;
        opendata->o_arg.open_flags = 0;
        opendata->o_arg.fmode = fmode;
@@ -1597,14 +1608,14 @@ static int nfs4_open_recover_helper(struct nfs4_opendata *opendata, fmode_t fmod
        newstate = nfs4_opendata_to_nfs4_state(opendata);
        if (IS_ERR(newstate))
                return PTR_ERR(newstate);
+       if (newstate != opendata->state)
+               ret = -ESTALE;
        nfs4_close_state(newstate, fmode);
-       *res = newstate;
-       return 0;
+       return ret;
 }
 
 static int nfs4_open_recover(struct nfs4_opendata *opendata, struct nfs4_state *state)
 {
-       struct nfs4_state *newstate;
        int ret;
 
        /* Don't trigger recovery in nfs_test_and_clear_all_open_stateid */
@@ -1615,27 +1626,15 @@ static int nfs4_open_recover(struct nfs4_opendata *opendata, struct nfs4_state *
        clear_bit(NFS_DELEGATED_STATE, &state->flags);
        clear_bit(NFS_OPEN_STATE, &state->flags);
        smp_rmb();
-       if (state->n_rdwr != 0) {
-               ret = nfs4_open_recover_helper(opendata, FMODE_READ|FMODE_WRITE, &newstate);
-               if (ret != 0)
-                       return ret;
-               if (newstate != state)
-                       return -ESTALE;
-       }
-       if (state->n_wronly != 0) {
-               ret = nfs4_open_recover_helper(opendata, FMODE_WRITE, &newstate);
-               if (ret != 0)
-                       return ret;
-               if (newstate != state)
-                       return -ESTALE;
-       }
-       if (state->n_rdonly != 0) {
-               ret = nfs4_open_recover_helper(opendata, FMODE_READ, &newstate);
-               if (ret != 0)
-                       return ret;
-               if (newstate != state)
-                       return -ESTALE;
-       }
+       ret = nfs4_open_recover_helper(opendata, FMODE_READ|FMODE_WRITE);
+       if (ret != 0)
+               return ret;
+       ret = nfs4_open_recover_helper(opendata, FMODE_WRITE);
+       if (ret != 0)
+               return ret;
+       ret = nfs4_open_recover_helper(opendata, FMODE_READ);
+       if (ret != 0)
+               return ret;
        /*
         * We may have performed cached opens for all three recoveries.
         * Check if we need to update the current stateid.
@@ -1759,18 +1758,32 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct
        return err;
 }
 
-int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state *state, const nfs4_stateid *stateid)
+int nfs4_open_delegation_recall(struct nfs_open_context *ctx,
+               struct nfs4_state *state, const nfs4_stateid *stateid,
+               fmode_t type)
 {
        struct nfs_server *server = NFS_SERVER(state->inode);
        struct nfs4_opendata *opendata;
-       int err;
+       int err = 0;
 
        opendata = nfs4_open_recoverdata_alloc(ctx, state,
                        NFS4_OPEN_CLAIM_DELEG_CUR_FH);
        if (IS_ERR(opendata))
                return PTR_ERR(opendata);
        nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
-       err = nfs4_open_recover(opendata, state);
+       clear_bit(NFS_DELEGATED_STATE, &state->flags);
+       switch (type & (FMODE_READ|FMODE_WRITE)) {
+       case FMODE_READ|FMODE_WRITE:
+       case FMODE_WRITE:
+               err = nfs4_open_recover_helper(opendata, FMODE_READ|FMODE_WRITE);
+               if (err)
+                       break;
+               err = nfs4_open_recover_helper(opendata, FMODE_WRITE);
+               if (err)
+                       break;
+       case FMODE_READ:
+               err = nfs4_open_recover_helper(opendata, FMODE_READ);
+       }
        nfs4_opendata_put(opendata);
        return nfs4_handle_delegation_recall_error(server, state, stateid, err);
 }