On Tue, Oct 14, 2014 at 11:48 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>>> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>>> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>>>> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>>>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>>>>> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> return nfs4_handle_delegation_recall_error(server, state, stateid, err);
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
>>>>>>>> file_lock *fl, struct nfs4_state *state,
>>>>>>>> err = nfs4_set_lock_state(state, fl);
>>>>>>>> if (err != 0)
>>>>>>>> return err;
>>>>>>>> + if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
>>>>>>>> + pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
>>>>>>>> __func__);
>>>>>>>> + return -EIO;
>>>>>>>> + }
>>>>>>>> err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
>>>>>>>
>>>>>>> Note that there is a potential race here if the server is playing with
>>>>>>> NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
>>>>>>> present the delegation as part of the LOCK request, we have no way of
>>>>>>> knowing if the delegation is still in effect. I guess we can check the
>>>>>>> return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
>>>>>>> or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
>>>>>>> LOCK is safe.
>>>>>>
>>>>>> I'm not following you. We send LOCK before we send DELEGRETURN? Also,
>>>>>> we normally send in LOCK the open_stateid returned by the open with
>>>>>> cur so do we know that delegation is still in effect.
>>>>>
>>>>> How so? The open stateid doesn't tell you that the delegation is still
>>>>> in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
>>>>> you tell if the delegation was revoked before or after the LOCK
>>>>> request was handled?
>>>>
>>>> Actually, let me answer that myself. You can sort of figure things out
>>>> in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
>>>> flag. If it is set, you should probably distrust the lock stateid that
>>>> you just attempted to recover, since you now know that at least one
>>>> delegation was just revoked.
>>>>
>>>> In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
>>>> on the delegation stateid.
>>>
>>> I think we are mis-communicating here by talking about different
>>> nuances. I agree with you that when an operation is sent there is no
>>> way of knowing if in the mean while the server has decided to revoke
>>> the delegation. However, this is not what I'm confused about regarding
>>> your comment. I'm noticing that in the flow of operations, we send (1)
>>> open with cur, then (2) lock, then (3) delegreturn. So I was confused
>>> about how can we check return of delegreturn, step 3, if we are in
>>> step 2.
>>>
>>> I think the LOCK is safe if the reply to the LOCK is successful.
>>
>> It needs to be concurrent with the delegation as well, otherwise
>> general lock atomicity is broken.
>>
>>> Let me just step back from this to note that your solution to "lost
>>> locks during delegation" is to recognize the open with cure failure
>>> and skip locking and delegreturn. I can work on a patch for that.
>>>
>>> Do you agree that the state recovery should not be initiated in case
>>> we get those errors?
>>
>> State recovery _should_ be initiated so that we do reclaim opens (I
>> don't care about share lock atomicity on Linux). However
>> nfs_delegation_claim_locks() _should_not_ be called.
>>
>> I'll give some more thought as to how we can ensure the missing lock atomicity.
>
> If state recover is initiated, it will go into an infinite loop. There
> is no way to stop it once it's initiated. A "recovery" open will get a
> BAD_STATEID which will again initiated state recovery.
>
> Are proposing to add smarts to the state manager when it should not
> recover from a BAD_STATEID?
Ok. How about something like this?
[PATCH 1/1] Fixing infinite state recovery loop due to failed delegation return
If we get a bad-stateid-type of error when we send OPEN with delegate_cur
to return currently held delegation, we shouldn't be trying to reclaim locks
associated with that delegation state_id because we don't have an
open_stateid to be used for the LOCK operation. Thus, we should
return an error from the nfs4_open_delegation_recall() in that case.
Furthermore, if an error occurs the delegation code will call
nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN
flags in the state and it leads the state manager to into an infinite loop
for trying to reclaim the delegated state.
Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
---
fs/nfs/delegation.c | 5 +++--
fs/nfs/nfs4proc.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5853f53..8016d89 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode
*inode, struct nfs_delegation
err = nfs4_wait_clnt_recover(clp);
} while (err == 0);
- if (err) {
+ if (err && err != -EIO) {
nfs_abort_delegation_return(delegation, clp);
goto out;
}
@@ -458,7 +458,8 @@ restart:
iput(inode);
if (!err)
goto restart;
- set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
+ if (err != -EIO)
+ set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
return err;
}
}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..6871055 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1655,7 +1655,7 @@ static int
nfs4_handle_delegation_recall_error(struct nfs_server *server, struct
nfs_inode_find_state_and_recover(state->inode,
stateid);
nfs4_schedule_stateid_recovery(server, state);
- return 0;
+ return -EIO;
case -NFS4ERR_DELAY:
case -NFS4ERR_GRACE:
set_bit(NFS_DELEGATED_STATE, &state->flags);
--
1.7.1
>
>>
>> --
>> Trond Myklebust
>>
>> Linux NFS client maintainer, PrimaryData
>>
>> trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
