how to properly handle failures during delegation recall process

how to properly handle failures during delegation recall process

[Olga Kornievskaia]

I'd like to hear community's thought about how to properly handle
failures during delegation recall process and/or thoughts about a
proposed fixed listed at the end.

There are two problems seen during the following situation:
A client get a cb_call for a delegation it currently holds. Consider
the case where the client has a delegated lock for this open. Callback
thread will send an open with delegation_cur, followed by a lock
operation, and finally delegreturn.

Problem#1: the client will send a lock operation regardless of whether
or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
the lock operation will choose to use the open_stateid. However, when
the open failed, the stateid is 0. Thus, we send an erroneous stateid
of 0.

Problem#2: if the open fails with admin_revoked, bad_stateid errors,
it leads to an infinite loop of sending an open with deleg_cur and
getting a bad_stateid error back.

It seems to me that we shouldn't even be trying to recover if we get a
bad_stateid-type of errors on open with deleg_cur because they are
unrecoverable.

Furthermore, I propose that if we get an error in
nfs4_open_delegation_recall() then we mark any delegation locks as
lost and in nfs4_lock_delegation_recall() don't attempt to recover
lost lock.

I have tested to following code as a fix:

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..523fae0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
nfs_open_context *ctx, struct nfs4_state
        nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
        err = nfs4_open_recover(opendata, state);
        nfs4_opendata_put(opendata);
+       switch(err) {
+               case -NFS4ERR_DELEG_REVOKED:
+               case -NFS4ERR_ADMIN_REVOKED:
+               case -NFS4ERR_BAD_STATEID:
+               case -NFS4ERR_OPENMODE: {
+                       struct nfs4_lock_state *lock;
+                       /* go through open locks and mark them lost */
+                       spin_lock(&state->state_lock);
+                       list_for_each_entry(lock, &state->lock_states,
ls_locks) {
+                               if (!test_bit(NFS_LOCK_INITIALIZED,
&lock->ls_flags))
+                                       set_bit(NFS_LOCK_LOST, &lock->ls_flags);
+                       }
+                       spin_unlock(&state->state_lock);
+                       return 0;
+               }
+       }
        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);
        return nfs4_handle_delegation_recall_error(server, state, stateid, err);
 }
--
1.7.1

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

Hi Olga,

On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> I'd like to hear community's thought about how to properly handle
> failures during delegation recall process and/or thoughts about a
> proposed fixed listed at the end.
>
> There are two problems seen during the following situation:
> A client get a cb_call for a delegation it currently holds. Consider
> the case where the client has a delegated lock for this open. Callback
> thread will send an open with delegation_cur, followed by a lock
> operation, and finally delegreturn.
>
> Problem#1: the client will send a lock operation regardless of whether
> or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
> the lock operation will choose to use the open_stateid. However, when
> the open failed, the stateid is 0. Thus, we send an erroneous stateid
> of 0.
>
> Problem#2: if the open fails with admin_revoked, bad_stateid errors,
> it leads to an infinite loop of sending an open with deleg_cur and
> getting a bad_stateid error back.
>
> It seems to me that we shouldn't even be trying to recover if we get a
> bad_stateid-type of errors on open with deleg_cur because they are
> unrecoverable.
>
> Furthermore, I propose that if we get an error in
> nfs4_open_delegation_recall() then we mark any delegation locks as
> lost and in nfs4_lock_delegation_recall() don't attempt to recover
> lost lock.
>
> I have tested to following code as a fix:
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 5aa55c1..523fae0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
> nfs_open_context *ctx, struct nfs4_state
>         nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
>         err = nfs4_open_recover(opendata, state);
>         nfs4_opendata_put(opendata);
> +       switch(err) {
> +               case -NFS4ERR_DELEG_REVOKED:
> +               case -NFS4ERR_ADMIN_REVOKED:
> +               case -NFS4ERR_BAD_STATEID:
> +               case -NFS4ERR_OPENMODE: {
> +                       struct nfs4_lock_state *lock;
> +                       /* go through open locks and mark them lost */
> +                       spin_lock(&state->state_lock);
> +                       list_for_each_entry(lock, &state->lock_states,
> ls_locks) {
> +                               if (!test_bit(NFS_LOCK_INITIALIZED,
> &lock->ls_flags))
> +                                       set_bit(NFS_LOCK_LOST, &lock->ls_flags);
> +                       }
> +                       spin_unlock(&state->state_lock);
> +                       return 0;

If the open stated is marked for "nograce" recovery by
nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
should do the above for you automatically. Are you reproducing this
bug with a 3.17 kernel?

> +               }
> +       }
>         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.

>         return nfs4_handle_delegation_recall_error(server, state, stateid, err);
>  }
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Hi Olga,
>
> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> I'd like to hear community's thought about how to properly handle
>> failures during delegation recall process and/or thoughts about a
>> proposed fixed listed at the end.
>>
>> There are two problems seen during the following situation:
>> A client get a cb_call for a delegation it currently holds. Consider
>> the case where the client has a delegated lock for this open. Callback
>> thread will send an open with delegation_cur, followed by a lock
>> operation, and finally delegreturn.
>>
>> Problem#1: the client will send a lock operation regardless of whether
>> or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
>> the lock operation will choose to use the open_stateid. However, when
>> the open failed, the stateid is 0. Thus, we send an erroneous stateid
>> of 0.
>>
>> Problem#2: if the open fails with admin_revoked, bad_stateid errors,
>> it leads to an infinite loop of sending an open with deleg_cur and
>> getting a bad_stateid error back.
>>
>> It seems to me that we shouldn't even be trying to recover if we get a
>> bad_stateid-type of errors on open with deleg_cur because they are
>> unrecoverable.
>>
>> Furthermore, I propose that if we get an error in
>> nfs4_open_delegation_recall() then we mark any delegation locks as
>> lost and in nfs4_lock_delegation_recall() don't attempt to recover
>> lost lock.
>>
>> I have tested to following code as a fix:
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 5aa55c1..523fae0 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
>> nfs_open_context *ctx, struct nfs4_state
>>         nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
>>         err = nfs4_open_recover(opendata, state);
>>         nfs4_opendata_put(opendata);
>> +       switch(err) {
>> +               case -NFS4ERR_DELEG_REVOKED:
>> +               case -NFS4ERR_ADMIN_REVOKED:
>> +               case -NFS4ERR_BAD_STATEID:
>> +               case -NFS4ERR_OPENMODE: {
>> +                       struct nfs4_lock_state *lock;
>> +                       /* go through open locks and mark them lost */
>> +                       spin_lock(&state->state_lock);
>> +                       list_for_each_entry(lock, &state->lock_states,
>> ls_locks) {
>> +                               if (!test_bit(NFS_LOCK_INITIALIZED,
>> &lock->ls_flags))
>> +                                       set_bit(NFS_LOCK_LOST, &lock->ls_flags);
>> +                       }
>> +                       spin_unlock(&state->state_lock);
>> +                       return 0;
>
> If the open stated is marked for "nograce" recovery by
> nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
> should do the above for you automatically. Are you reproducing this
> bug with a 3.17 kernel?

Yes, the bug is with the 3.17 kernel.

Yes, the nfs4_lock_expired() will mark it. However, the state_manager
thread (most frequently) doesn't get to run to do that before
nfs4_open_delegation_recall() returns 0 and calls the
nfs_delegation_claim_locks(). Therefore, I found the code needed
before returning from nfs4_open_delegation_recall().

>
>> +               }
>> +       }
>>         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.

>
>>         return nfs4_handle_delegation_recall_error(server, state, stateid, err);
>>  }
>> --
>> 1.7.1
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> Hi Olga,
>>
>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> I'd like to hear community's thought about how to properly handle
>>> failures during delegation recall process and/or thoughts about a
>>> proposed fixed listed at the end.
>>>
>>> There are two problems seen during the following situation:
>>> A client get a cb_call for a delegation it currently holds. Consider
>>> the case where the client has a delegated lock for this open. Callback
>>> thread will send an open with delegation_cur, followed by a lock
>>> operation, and finally delegreturn.
>>>
>>> Problem#1: the client will send a lock operation regardless of whether
>>> or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
>>> the lock operation will choose to use the open_stateid. However, when
>>> the open failed, the stateid is 0. Thus, we send an erroneous stateid
>>> of 0.
>>>
>>> Problem#2: if the open fails with admin_revoked, bad_stateid errors,
>>> it leads to an infinite loop of sending an open with deleg_cur and
>>> getting a bad_stateid error back.
>>>
>>> It seems to me that we shouldn't even be trying to recover if we get a
>>> bad_stateid-type of errors on open with deleg_cur because they are
>>> unrecoverable.
>>>
>>> Furthermore, I propose that if we get an error in
>>> nfs4_open_delegation_recall() then we mark any delegation locks as
>>> lost and in nfs4_lock_delegation_recall() don't attempt to recover
>>> lost lock.
>>>
>>> I have tested to following code as a fix:
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 5aa55c1..523fae0 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
>>> nfs_open_context *ctx, struct nfs4_state
>>>         nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
>>>         err = nfs4_open_recover(opendata, state);
>>>         nfs4_opendata_put(opendata);
>>> +       switch(err) {
>>> +               case -NFS4ERR_DELEG_REVOKED:
>>> +               case -NFS4ERR_ADMIN_REVOKED:
>>> +               case -NFS4ERR_BAD_STATEID:
>>> +               case -NFS4ERR_OPENMODE: {
>>> +                       struct nfs4_lock_state *lock;
>>> +                       /* go through open locks and mark them lost */
>>> +                       spin_lock(&state->state_lock);
>>> +                       list_for_each_entry(lock, &state->lock_states,
>>> ls_locks) {
>>> +                               if (!test_bit(NFS_LOCK_INITIALIZED,
>>> &lock->ls_flags))
>>> +                                       set_bit(NFS_LOCK_LOST, &lock->ls_flags);
>>> +                       }
>>> +                       spin_unlock(&state->state_lock);
>>> +                       return 0;
>>
>> If the open stated is marked for "nograce" recovery by
>> nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
>> should do the above for you automatically. Are you reproducing this
>> bug with a 3.17 kernel?
>
> Yes, the bug is with the 3.17 kernel.
>
> Yes, the nfs4_lock_expired() will mark it. However, the state_manager
> thread (most frequently) doesn't get to run to do that before
> nfs4_open_delegation_recall() returns 0 and calls the
> nfs_delegation_claim_locks(). Therefore, I found the code needed
> before returning from nfs4_open_delegation_recall().

Right. We probably should not be returning a value of 0 in that case.
If the delegation has been revoked, then we want
nfs4_open_delegation_recall() to just return immediately, and then we
want nfs_end_delegation_return() to detach the delegation and free it
without calling delegreturn.

>>> +               }
>>> +       }
>>>         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?

>
>>
>>>         return nfs4_handle_delegation_recall_error(server, state, stateid, err);
>>>  }
>>> --
>>> 1.7.1
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Trond Myklebust
>>
>> Linux NFS client maintainer, PrimaryData
>>
>> trond.myklebust@primarydata.com



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>> <trond.myklebust@primarydata.com> wrote:
>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.

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?

>
> --
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>>> <trond.myklebust@primarydata.com> wrote:
>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>>> <trond.myklebust@primarydata.com> wrote:
>>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>>>> <trond.myklebust@primarydata.com> wrote:
>>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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?

>
> --
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

On Tue, Oct 14, 2014 at 11:48 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>>> <trond.myklebust@primarydata.com> wrote:
>>>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>>>> <trond.myklebust@primarydata.com> wrote:
>>>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>>>>> <trond.myklebust@primarydata.com> wrote:
>>>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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@netapp.com>
---
 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@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

I'd like to checkin to see what folks think about the proposed fix?

On Thu, Oct 16, 2014 at 2:43 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Tue, Oct 14, 2014 at 11:48 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>>>> <trond.myklebust@primarydata.com> wrote:
>>>>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>>>>> <trond.myklebust@primarydata.com> wrote:
>>>>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>>>>>>> <trond.myklebust@primarydata.com> wrote:
>>>>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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@netapp.com>
> ---
>  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@primarydata.com

Re: how to properly handle failures during delegation recall process

[Jeff Layton]

On Mon, 13 Oct 2014 18:24:21 -0400
Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
> > <trond.myklebust@primarydata.com> wrote:
> >> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
> >> <trond.myklebust@primarydata.com> wrote:
> >>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
> >>>> <trond.myklebust@primarydata.com> wrote:
> >>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.
> 


(cc'ing Tom here since we may want to consider providing guidance in
 the spec for this situation)

Ok, I think both of you are right here :). Here's my interpretation:

Olga is correct that the LOCK operation itself is safe since LOCK
doesn't actually modify the contents of the file. What it's not safe to
do is to trust that LOCK unless and until the DELEGRETURN is also
successful.

First, let's clarify the potential race that Trond pointed out:

Suppose we have a delegation and delegated locks. That delegation is
recalled and we do something like this:

OPEN with DELEGATE_CUR: NFS4_OK
LOCK:                   NFS4_OK
LOCK:                   NFS4_OK
...(maybe more successful locks here)...
DELEGRETURN:            NFS4ERR_ADMIN_REVOKED

...at that point, we're screwed.

The delegation was obviously revoked after we did the OPEN but before
the DELEGRETURN. None of those LOCK requests can be trusted since
another client may have opened the file at any point in there, acquired
any one of those locks and then released it.

For v4.1+ the client can do what Trond suggests. Check for
SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
fails, then we must consider the most recently acquired lock lost.
LOCKU it and give up trying to reclaim the rest of them.

For v4.0, I'm not sure what the client can do other than wait until the
DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
have to try to unwind the whole mess. Send LOCKUs for all of them and
consider them all to be lost.

Actually, it may be reasonable to just do the same thing for v4.1. The
client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
any unreclaimable lock, any I/O done with that stateid is going to fail
anyway. You might as well just release any locks you do hold at that
point.

The other question is whether the server ought to have any role to play
here. In principle it could track whether an open/lock stateid is
descended from a still outstanding delegation, and revoke those
stateids if the delegation is revoked. That would probably not be
trivial to do with the current Linux server implementation, however.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> On Mon, 13 Oct 2014 18:24:21 -0400
> Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>
>> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> > On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>> > <trond.myklebust@primarydata.com> wrote:
>> >> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>> >> <trond.myklebust@primarydata.com> wrote:
>> >>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> >>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>> >>>> <trond.myklebust@primarydata.com> wrote:
>> >>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.
>>
>
>
> (cc'ing Tom here since we may want to consider providing guidance in
>  the spec for this situation)
>
> Ok, I think both of you are right here :). Here's my interpretation:
>
> Olga is correct that the LOCK operation itself is safe since LOCK
> doesn't actually modify the contents of the file. What it's not safe to
> do is to trust that LOCK unless and until the DELEGRETURN is also
> successful.
>
> First, let's clarify the potential race that Trond pointed out:
>
> Suppose we have a delegation and delegated locks. That delegation is
> recalled and we do something like this:
>
> OPEN with DELEGATE_CUR: NFS4_OK
> LOCK:                   NFS4_OK
> LOCK:                   NFS4_OK
> ...(maybe more successful locks here)...
> DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
>
> ...at that point, we're screwed.
>
> The delegation was obviously revoked after we did the OPEN but before
> the DELEGRETURN. None of those LOCK requests can be trusted since
> another client may have opened the file at any point in there, acquired
> any one of those locks and then released it.
>
> For v4.1+ the client can do what Trond suggests. Check for
> SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> fails, then we must consider the most recently acquired lock lost.
> LOCKU it and give up trying to reclaim the rest of them.
>
> For v4.0, I'm not sure what the client can do other than wait until the
> DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> have to try to unwind the whole mess. Send LOCKUs for all of them and
> consider them all to be lost.
>
> Actually, it may be reasonable to just do the same thing for v4.1. The
> client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> any unreclaimable lock, any I/O done with that stateid is going to fail
> anyway. You might as well just release any locks you do hold at that
> point.
>
> The other question is whether the server ought to have any role to play
> here. In principle it could track whether an open/lock stateid is
> descended from a still outstanding delegation, and revoke those
> stateids if the delegation is revoked. That would probably not be
> trivial to do with the current Linux server implementation, however.
>

What the server could (and probably should) do is revoke all
open/lock/layout state for the clientid+file combination for which it
is also revoking the delegation. That means that all applications that
were using that file on that client would be screwed, but they
probably will be anyway if the file gets corrupted due to non-atomic
locking.

Cheers
  Trond

Re: how to properly handle failures during delegation recall process

[J. Bruce Fields]

On Wed, Nov 05, 2014 at 07:41:58AM -0500, Trond Myklebust wrote:
> On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> > (cc'ing Tom here since we may want to consider providing guidance in
> >  the spec for this situation)
> >
> > Ok, I think both of you are right here :). Here's my interpretation:
> >
> > Olga is correct that the LOCK operation itself is safe since LOCK
> > doesn't actually modify the contents of the file. What it's not safe to
> > do is to trust that LOCK unless and until the DELEGRETURN is also
> > successful.
> >
> > First, let's clarify the potential race that Trond pointed out:
> >
> > Suppose we have a delegation and delegated locks. That delegation is
> > recalled and we do something like this:
> >
> > OPEN with DELEGATE_CUR: NFS4_OK
> > LOCK:                   NFS4_OK
> > LOCK:                   NFS4_OK
> > ...(maybe more successful locks here)...
> > DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
> >
> > ...at that point, we're screwed.
> >
> > The delegation was obviously revoked after we did the OPEN but before
> > the DELEGRETURN. None of those LOCK requests can be trusted since
> > another client may have opened the file at any point in there, acquired
> > any one of those locks and then released it.
> >
> > For v4.1+ the client can do what Trond suggests. Check for
> > SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> > then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> > fails, then we must consider the most recently acquired lock lost.
> > LOCKU it and give up trying to reclaim the rest of them.
> >
> > For v4.0, I'm not sure what the client can do other than wait until the
> > DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> > have to try to unwind the whole mess. Send LOCKUs for all of them and
> > consider them all to be lost.
> >
> > Actually, it may be reasonable to just do the same thing for v4.1. The
> > client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> > any unreclaimable lock, any I/O done with that stateid is going to fail
> > anyway. You might as well just release any locks you do hold at that
> > point.
> >
> > The other question is whether the server ought to have any role to play
> > here. In principle it could track whether an open/lock stateid is
> > descended from a still outstanding delegation, and revoke those
> > stateids if the delegation is revoked. That would probably not be
> > trivial to do with the current Linux server implementation, however.

That sounds like a problem for whoever wants to implement support for
administrative revocation of state.  We don't really support it
currently.

Oops, right, except for the case where the delegation's revoked just
because the client ran out of time doing the recall.  In which case I
think the final error's going to be either EXPIRED (4.0) or
DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
BAD_STATEID in the 4.0 case, which looks wrong.)

--b.

> What the server could (and probably should) do is revoke all
> open/lock/layout state for the clientid+file combination for which it
> is also revoking the delegation. That means that all applications that
> were using that file on that client would be screwed, but they
> probably will be anyway if the file gets corrupted due to non-atomic
> locking.

Re: how to properly handle failures during delegation recall process

[Jeff Layton]

On Wed, 5 Nov 2014 13:31:52 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Nov 05, 2014 at 07:41:58AM -0500, Trond Myklebust wrote:
> > On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> > > (cc'ing Tom here since we may want to consider providing guidance in
> > >  the spec for this situation)
> > >
> > > Ok, I think both of you are right here :). Here's my interpretation:
> > >
> > > Olga is correct that the LOCK operation itself is safe since LOCK
> > > doesn't actually modify the contents of the file. What it's not safe to
> > > do is to trust that LOCK unless and until the DELEGRETURN is also
> > > successful.
> > >
> > > First, let's clarify the potential race that Trond pointed out:
> > >
> > > Suppose we have a delegation and delegated locks. That delegation is
> > > recalled and we do something like this:
> > >
> > > OPEN with DELEGATE_CUR: NFS4_OK
> > > LOCK:                   NFS4_OK
> > > LOCK:                   NFS4_OK
> > > ...(maybe more successful locks here)...
> > > DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
> > >
> > > ...at that point, we're screwed.
> > >
> > > The delegation was obviously revoked after we did the OPEN but before
> > > the DELEGRETURN. None of those LOCK requests can be trusted since
> > > another client may have opened the file at any point in there, acquired
> > > any one of those locks and then released it.
> > >
> > > For v4.1+ the client can do what Trond suggests. Check for
> > > SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> > > then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> > > fails, then we must consider the most recently acquired lock lost.
> > > LOCKU it and give up trying to reclaim the rest of them.
> > >
> > > For v4.0, I'm not sure what the client can do other than wait until the
> > > DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> > > have to try to unwind the whole mess. Send LOCKUs for all of them and
> > > consider them all to be lost.
> > >
> > > Actually, it may be reasonable to just do the same thing for v4.1. The
> > > client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> > > any unreclaimable lock, any I/O done with that stateid is going to fail
> > > anyway. You might as well just release any locks you do hold at that
> > > point.
> > >
> > > The other question is whether the server ought to have any role to play
> > > here. In principle it could track whether an open/lock stateid is
> > > descended from a still outstanding delegation, and revoke those
> > > stateids if the delegation is revoked. That would probably not be
> > > trivial to do with the current Linux server implementation, however.
> 
> That sounds like a problem for whoever wants to implement support for
> administrative revocation of state.  We don't really support it
> currently.
> 
> Oops, right, except for the case where the delegation's revoked just
> because the client ran out of time doing the recall.  In which case I
> think the final error's going to be either EXPIRED (4.0) or
> DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
> BAD_STATEID in the 4.0 case, which looks wrong.)
> 

I'm not sure that that's right... RFC3530 says:

   NFS4ERR_EXPIRED       A lease has expired that is being used in the
                         current operation.

...implicit in the scenario I layed out above is that the lease is
being maintained. It's just that the client failed to return the
delegation in time. So, BAD_STATEID may be correct, actually?

> 
> > What the server could (and probably should) do is revoke all
> > open/lock/layout state for the clientid+file combination for which it
> > is also revoking the delegation. That means that all applications that
> > were using that file on that client would be screwed, but they
> > probably will be anyway if the file gets corrupted due to non-atomic
> > locking.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@primarydata.com>

Re: how to properly handle failures during delegation recall process

[J. Bruce Fields]

On Wed, Nov 05, 2014 at 02:42:51PM -0500, Jeff Layton wrote:
> On Wed, 5 Nov 2014 13:31:52 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Nov 05, 2014 at 07:41:58AM -0500, Trond Myklebust wrote:
> > > On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> > > > (cc'ing Tom here since we may want to consider providing guidance in
> > > >  the spec for this situation)
> > > >
> > > > Ok, I think both of you are right here :). Here's my interpretation:
> > > >
> > > > Olga is correct that the LOCK operation itself is safe since LOCK
> > > > doesn't actually modify the contents of the file. What it's not safe to
> > > > do is to trust that LOCK unless and until the DELEGRETURN is also
> > > > successful.
> > > >
> > > > First, let's clarify the potential race that Trond pointed out:
> > > >
> > > > Suppose we have a delegation and delegated locks. That delegation is
> > > > recalled and we do something like this:
> > > >
> > > > OPEN with DELEGATE_CUR: NFS4_OK
> > > > LOCK:                   NFS4_OK
> > > > LOCK:                   NFS4_OK
> > > > ...(maybe more successful locks here)...
> > > > DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
> > > >
> > > > ...at that point, we're screwed.
> > > >
> > > > The delegation was obviously revoked after we did the OPEN but before
> > > > the DELEGRETURN. None of those LOCK requests can be trusted since
> > > > another client may have opened the file at any point in there, acquired
> > > > any one of those locks and then released it.
> > > >
> > > > For v4.1+ the client can do what Trond suggests. Check for
> > > > SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> > > > then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> > > > fails, then we must consider the most recently acquired lock lost.
> > > > LOCKU it and give up trying to reclaim the rest of them.
> > > >
> > > > For v4.0, I'm not sure what the client can do other than wait until the
> > > > DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> > > > have to try to unwind the whole mess. Send LOCKUs for all of them and
> > > > consider them all to be lost.
> > > >
> > > > Actually, it may be reasonable to just do the same thing for v4.1. The
> > > > client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> > > > any unreclaimable lock, any I/O done with that stateid is going to fail
> > > > anyway. You might as well just release any locks you do hold at that
> > > > point.
> > > >
> > > > The other question is whether the server ought to have any role to play
> > > > here. In principle it could track whether an open/lock stateid is
> > > > descended from a still outstanding delegation, and revoke those
> > > > stateids if the delegation is revoked. That would probably not be
> > > > trivial to do with the current Linux server implementation, however.
> > 
> > That sounds like a problem for whoever wants to implement support for
> > administrative revocation of state.  We don't really support it
> > currently.
> > 
> > Oops, right, except for the case where the delegation's revoked just
> > because the client ran out of time doing the recall.  In which case I
> > think the final error's going to be either EXPIRED (4.0) or
> > DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
> > BAD_STATEID in the 4.0 case, which looks wrong.)
> > 
> 
> I'm not sure that that's right... RFC3530 says:
> 
>    NFS4ERR_EXPIRED       A lease has expired that is being used in the
>                          current operation.
> 
> ...implicit in the scenario I layed out above is that the lease is
> being maintained. It's just that the client failed to return the
> delegation in time. So, BAD_STATEID may be correct, actually?

Yes, I misread that EXPIRED text.

That's a bit of a digression--in any case we agree that it's this late
DELEGRETURN case that's the only real bug right now?

On the client side I guess I can't think of anything better than your
suggestion of waiting for the error on DELEGRETURN as you describe.

And on the server side:

> > > What the server could (and probably should) do is revoke all
> > > open/lock/layout state for the clientid+file combination for which it
> > > is also revoking the delegation. That means that all applications that
> > > were using that file on that client would be screwed, but they
> > > probably will be anyway if the file gets corrupted due to non-atomic
> > > locking.

That sounds harsh at first, but I guess it makes sense.

--b.

Re: how to properly handle failures during delegation recall process

[Jeff Layton]

On Wed, 5 Nov 2014 14:54:20 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Nov 05, 2014 at 02:42:51PM -0500, Jeff Layton wrote:
> > On Wed, 5 Nov 2014 13:31:52 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Wed, Nov 05, 2014 at 07:41:58AM -0500, Trond Myklebust wrote:
> > > > On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> > > > > (cc'ing Tom here since we may want to consider providing guidance in
> > > > >  the spec for this situation)
> > > > >
> > > > > Ok, I think both of you are right here :). Here's my interpretation:
> > > > >
> > > > > Olga is correct that the LOCK operation itself is safe since LOCK
> > > > > doesn't actually modify the contents of the file. What it's not safe to
> > > > > do is to trust that LOCK unless and until the DELEGRETURN is also
> > > > > successful.
> > > > >
> > > > > First, let's clarify the potential race that Trond pointed out:
> > > > >
> > > > > Suppose we have a delegation and delegated locks. That delegation is
> > > > > recalled and we do something like this:
> > > > >
> > > > > OPEN with DELEGATE_CUR: NFS4_OK
> > > > > LOCK:                   NFS4_OK
> > > > > LOCK:                   NFS4_OK
> > > > > ...(maybe more successful locks here)...
> > > > > DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
> > > > >
> > > > > ...at that point, we're screwed.
> > > > >
> > > > > The delegation was obviously revoked after we did the OPEN but before
> > > > > the DELEGRETURN. None of those LOCK requests can be trusted since
> > > > > another client may have opened the file at any point in there, acquired
> > > > > any one of those locks and then released it.
> > > > >
> > > > > For v4.1+ the client can do what Trond suggests. Check for
> > > > > SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> > > > > then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> > > > > fails, then we must consider the most recently acquired lock lost.
> > > > > LOCKU it and give up trying to reclaim the rest of them.
> > > > >
> > > > > For v4.0, I'm not sure what the client can do other than wait until the
> > > > > DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> > > > > have to try to unwind the whole mess. Send LOCKUs for all of them and
> > > > > consider them all to be lost.
> > > > >
> > > > > Actually, it may be reasonable to just do the same thing for v4.1. The
> > > > > client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> > > > > any unreclaimable lock, any I/O done with that stateid is going to fail
> > > > > anyway. You might as well just release any locks you do hold at that
> > > > > point.
> > > > >
> > > > > The other question is whether the server ought to have any role to play
> > > > > here. In principle it could track whether an open/lock stateid is
> > > > > descended from a still outstanding delegation, and revoke those
> > > > > stateids if the delegation is revoked. That would probably not be
> > > > > trivial to do with the current Linux server implementation, however.
> > > 
> > > That sounds like a problem for whoever wants to implement support for
> > > administrative revocation of state.  We don't really support it
> > > currently.
> > > 
> > > Oops, right, except for the case where the delegation's revoked just
> > > because the client ran out of time doing the recall.  In which case I
> > > think the final error's going to be either EXPIRED (4.0) or
> > > DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
> > > BAD_STATEID in the 4.0 case, which looks wrong.)
> > > 
> > 
> > I'm not sure that that's right... RFC3530 says:
> > 
> >    NFS4ERR_EXPIRED       A lease has expired that is being used in the
> >                          current operation.
> > 
> > ...implicit in the scenario I layed out above is that the lease is
> > being maintained. It's just that the client failed to return the
> > delegation in time. So, BAD_STATEID may be correct, actually?
> 
> Yes, I misread that EXPIRED text.
> 
> That's a bit of a digression--in any case we agree that it's this late
> DELEGRETURN case that's the only real bug right now?
> 

Yes I think so, given that we have no mechanism to administratively
revoke delegations (other than the fault injection code, which I'll
just ignore here ;).

> On the client side I guess I can't think of anything better than your
> suggestion of waiting for the error on DELEGRETURN as you describe.
> 

...and given the comments above, we have to handle a BAD_STATEID error
on a DELEGRETURN the same way we would an ADMIN_REVOKED error, I think.

> And on the server side:
> 
> > > > What the server could (and probably should) do is revoke all
> > > > open/lock/layout state for the clientid+file combination for which it
> > > > is also revoking the delegation. That means that all applications that
> > > > were using that file on that client would be screwed, but they
> > > > probably will be anyway if the file gets corrupted due to non-atomic
> > > > locking.
> 
> That sounds harsh at first, but I guess it makes sense.
> 

Agreed. We could also consider expiring the client prematurely, but
that's even more harsh.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: how to properly handle failures during delegation recall process

[J. Bruce Fields]

On Wed, Nov 05, 2014 at 03:06:02PM -0500, Jeff Layton wrote:
> On Wed, 5 Nov 2014 14:54:20 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Nov 05, 2014 at 02:42:51PM -0500, Jeff Layton wrote:
> > > On Wed, 5 Nov 2014 13:31:52 -0500
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Wed, Nov 05, 2014 at 07:41:58AM -0500, Trond Myklebust wrote:
> > > > > On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> > > > > > (cc'ing Tom here since we may want to consider providing guidance in
> > > > > >  the spec for this situation)
> > > > > >
> > > > > > Ok, I think both of you are right here :). Here's my interpretation:
> > > > > >
> > > > > > Olga is correct that the LOCK operation itself is safe since LOCK
> > > > > > doesn't actually modify the contents of the file. What it's not safe to
> > > > > > do is to trust that LOCK unless and until the DELEGRETURN is also
> > > > > > successful.
> > > > > >
> > > > > > First, let's clarify the potential race that Trond pointed out:
> > > > > >
> > > > > > Suppose we have a delegation and delegated locks. That delegation is
> > > > > > recalled and we do something like this:
> > > > > >
> > > > > > OPEN with DELEGATE_CUR: NFS4_OK
> > > > > > LOCK:                   NFS4_OK
> > > > > > LOCK:                   NFS4_OK
> > > > > > ...(maybe more successful locks here)...
> > > > > > DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
> > > > > >
> > > > > > ...at that point, we're screwed.
> > > > > >
> > > > > > The delegation was obviously revoked after we did the OPEN but before
> > > > > > the DELEGRETURN. None of those LOCK requests can be trusted since
> > > > > > another client may have opened the file at any point in there, acquired
> > > > > > any one of those locks and then released it.
> > > > > >
> > > > > > For v4.1+ the client can do what Trond suggests. Check for
> > > > > > SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> > > > > > then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> > > > > > fails, then we must consider the most recently acquired lock lost.
> > > > > > LOCKU it and give up trying to reclaim the rest of them.
> > > > > >
> > > > > > For v4.0, I'm not sure what the client can do other than wait until the
> > > > > > DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> > > > > > have to try to unwind the whole mess. Send LOCKUs for all of them and
> > > > > > consider them all to be lost.
> > > > > >
> > > > > > Actually, it may be reasonable to just do the same thing for v4.1. The
> > > > > > client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> > > > > > any unreclaimable lock, any I/O done with that stateid is going to fail
> > > > > > anyway. You might as well just release any locks you do hold at that
> > > > > > point.
> > > > > >
> > > > > > The other question is whether the server ought to have any role to play
> > > > > > here. In principle it could track whether an open/lock stateid is
> > > > > > descended from a still outstanding delegation, and revoke those
> > > > > > stateids if the delegation is revoked. That would probably not be
> > > > > > trivial to do with the current Linux server implementation, however.
> > > > 
> > > > That sounds like a problem for whoever wants to implement support for
> > > > administrative revocation of state.  We don't really support it
> > > > currently.
> > > > 
> > > > Oops, right, except for the case where the delegation's revoked just
> > > > because the client ran out of time doing the recall.  In which case I
> > > > think the final error's going to be either EXPIRED (4.0) or
> > > > DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
> > > > BAD_STATEID in the 4.0 case, which looks wrong.)
> > > > 
> > > 
> > > I'm not sure that that's right... RFC3530 says:
> > > 
> > >    NFS4ERR_EXPIRED       A lease has expired that is being used in the
> > >                          current operation.
> > > 
> > > ...implicit in the scenario I layed out above is that the lease is
> > > being maintained. It's just that the client failed to return the
> > > delegation in time. So, BAD_STATEID may be correct, actually?
> > 
> > Yes, I misread that EXPIRED text.
> > 
> > That's a bit of a digression--in any case we agree that it's this late
> > DELEGRETURN case that's the only real bug right now?
> > 
> 
> Yes I think so, given that we have no mechanism to administratively
> revoke delegations (other than the fault injection code, which I'll
> just ignore here ;).
> 
> > On the client side I guess I can't think of anything better than your
> > suggestion of waiting for the error on DELEGRETURN as you describe.
> > 
> 
> ...and given the comments above, we have to handle a BAD_STATEID error
> on a DELEGRETURN the same way we would an ADMIN_REVOKED error, I think.

And DELEG_REVOKED in the >=4.1 case.

And now that I look at it I think there's a server bug there--it looks
like it's still returning BAD_STATEID in the >=4.1 case, but it should
be returning DELEG_REVOKED.

--b.

Re: how to properly handle failures during delegation recall process

[Trond Myklebust]

On Wed, Nov 5, 2014 at 2:42 PM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> On Wed, 5 Nov 2014 13:31:52 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>> Oops, right, except for the case where the delegation's revoked just
>> because the client ran out of time doing the recall.  In which case I
>> think the final error's going to be either EXPIRED (4.0) or
>> DELEG_REVOKED (4.1)?  (Except I think the Linux server's returning
>> BAD_STATEID in the 4.0 case, which looks wrong.)
>>
>
> I'm not sure that that's right... RFC3530 says:
>
>    NFS4ERR_EXPIRED       A lease has expired that is being used in the
>                          current operation.
>
> ...implicit in the scenario I layed out above is that the lease is
> being maintained. It's just that the client failed to return the
> delegation in time. So, BAD_STATEID may be correct, actually?

NFS4ERR_ADMIN_REVOKED is the expected error, but that requires the
server to keep the stateid around for a while (which is why NFSv4.1
added the FREE_STATEID requirement). NFS4ERR_BAD_STATEID will work in
a pinch...

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: how to properly handle failures during delegation recall process

[Olga Kornievskaia]

Resurrecting an old thread... To handle ADMIN_REVOKED for DELEGRETURN
when we are processing a CB_RECALL:

On Wed, Nov 5, 2014 at 6:57 AM, Jeff Layton <jeff.layton@primarydata.com> wrote:
> On Mon, 13 Oct 2014 18:24:21 -0400
> Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>
>> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> > On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
>> > <trond.myklebust@primarydata.com> wrote:
>> >> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
>> >> <trond.myklebust@primarydata.com> wrote:
>> >>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> >>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
>> >>>> <trond.myklebust@primarydata.com> wrote:
>> >>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia <aglo@umich.edu> 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.
>>
>
>
> (cc'ing Tom here since we may want to consider providing guidance in
>  the spec for this situation)
>
> Ok, I think both of you are right here :). Here's my interpretation:
>
> Olga is correct that the LOCK operation itself is safe since LOCK
> doesn't actually modify the contents of the file. What it's not safe to
> do is to trust that LOCK unless and until the DELEGRETURN is also
> successful.
>
> First, let's clarify the potential race that Trond pointed out:
>
> Suppose we have a delegation and delegated locks. That delegation is
> recalled and we do something like this:
>
> OPEN with DELEGATE_CUR: NFS4_OK
> LOCK:                   NFS4_OK
> LOCK:                   NFS4_OK
> ...(maybe more successful locks here)...
> DELEGRETURN:            NFS4ERR_ADMIN_REVOKED
>
> ...at that point, we're screwed.
>
> The delegation was obviously revoked after we did the OPEN but before
> the DELEGRETURN. None of those LOCK requests can be trusted since
> another client may have opened the file at any point in there, acquired
> any one of those locks and then released it.
>
> For v4.1+ the client can do what Trond suggests. Check for
> SEQ4_STATUS_RECALLABLE_STATE_REVOKED in each LOCK response. If it's set
> then we can do the TEST_STATEID/FREE_STATEID dance. If the TEST_STATEID
> fails, then we must consider the most recently acquired lock lost.
> LOCKU it and give up trying to reclaim the rest of them.
>
> For v4.0, I'm not sure what the client can do other than wait until the
> DELEGRETURN. If that fails with NFS4ERR_ADMIN_REVOKED, then we'll just
> have to try to unwind the whole mess. Send LOCKUs for all of them and
> consider them all to be lost.

Would it then be an acceptable solution to:
-- make deleg_return synchronous so that we wait for the reply to
handle the error
-- if we get an error in reply, don't ignore the error and propogate
it back to the delegation code.
-- then mark all the locks lost for the inode.

I don't have anything to send LOCKUs as wouldn't the application/vfs
take care of that when we fail the IO.

Also another problem is that ADMIN_REVOKED gets mapped into EIO by
nfs4_map_errors() before it gets returned into the delegation code so
the patch will mark locks lost for anything else that was translated
into the EIO.

Here's a patch that I've tried that seems to work:

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5166adc..e559407 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -430,6 +430,25 @@ static int nfs_end_delegation_return(struct inode
*inode, struct nfs_delegation
         goto out;

     err = nfs_do_return_delegation(inode, delegation, issync);
+    if (err == -EIO) {
+        struct file_lock *fl;
+        struct file_lock_context *flctx = inode->i_flctx;
+        struct list_head *list;
+
+        if (flctx == NULL)
+            goto out;
+
+        /* mark all locks lost */
+        list = &flctx->flc_posix;
+        down_write(&nfsi->rwsem);
+        spin_lock(&flctx->flc_lock);
+        list_for_each_entry(fl, list, fl_list) {
+            set_bit(NFS_LOCK_LOST,
+                &fl->fl_u.nfs4_fl.owner->ls_flags);
+        }
+        spin_unlock(&flctx->flc_lock);
+        up_write(&nfsi->rwsem);
+    }
 out:
     return err;
 }
@@ -490,7 +509,7 @@ restart:
             delegation = nfs_start_delegation_return_locked(NFS_I(inode));
             rcu_read_unlock();

-            err = nfs_end_delegation_return(inode, delegation, 0);
+            err = nfs_end_delegation_return(inode, delegation, 1);
             iput(inode);
             nfs_sb_deactive(server->super);
             if (!err)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 327b8c3..297a466 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5309,13 +5309,13 @@ static void nfs4_delegreturn_done(struct
rpc_task *task, void *calldata)
     switch (task->tk_status) {
     case 0:
         renew_lease(data->res.server, data->timestamp);
-    case -NFS4ERR_ADMIN_REVOKED:
-    case -NFS4ERR_DELEG_REVOKED:
     case -NFS4ERR_BAD_STATEID:
     case -NFS4ERR_OLD_STATEID:
     case -NFS4ERR_STALE_STATEID:
     case -NFS4ERR_EXPIRED:
         task->tk_status = 0;
+    case -NFS4ERR_ADMIN_REVOKED:
+    case -NFS4ERR_DELEG_REVOKED:
         if (data->roc)
             pnfs_roc_set_barrier(data->inode, data->roc_barrier);
         break;


>
> Actually, it may be reasonable to just do the same thing for v4.1. The
> client tracks NFS_LOCK_LOST on a per-lockstateid basis, so once you have
> any unreclaimable lock, any I/O done with that stateid is going to fail
> anyway. You might as well just release any locks you do hold at that
> point.
>
> The other question is whether the server ought to have any role to play
> here. In principle it could track whether an open/lock stateid is
> descended from a still outstanding delegation, and revoke those
> stateids if the delegation is revoked. That would probably not be
> trivial to do with the current Linux server implementation, however.
>
> --
> Jeff Layton <jlayton@primarydata.com>