> On Jun 12, 2015, at 5:42 PM, Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jun 12, 2015 at 5:37 PM, Kornievskaia, Olga
> <Olga.Kornievskaia@xxxxxxxxxx> wrote:
>>
>>> On Jun 12, 2015, at 5:21 PM, Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>>
>>> On Fri, Jun 12, 2015 at 4:53 PM, Olga Kornievskaia <kolga@xxxxxxxxxx> wrote:
>>>> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
>>>> delegation stateid was used. When no open state exists, in case of application
>>>> calling truncate() on the file, client has no state to recover and fails with
>>>> EIO.
>>>>
>>>> Instead, upon such error, return the bad delegation and then resend the
>>>> SETATTR with a zero stateid.
>>>>
>>>> Signed-off: Olga Kornievskaia <kolga@xxxxxxxxxx>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index ad7cf7e..2a85af3 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -360,8 +360,14 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
>>>> case -NFS4ERR_DELEG_REVOKED:
>>>> case -NFS4ERR_ADMIN_REVOKED:
>>>> case -NFS4ERR_BAD_STATEID:
>>>> - if (state == NULL)
>>>> + if (state == NULL) {
>>>> + if (inode && nfs4_have_delegation(inode,
>>>> + FMODE_READ)) {
>>>> + nfs4_inode_return_delegation(inode);
>>>> + exception->retry = 1;
>>>> + }
>>>> break;
>>>> + }
>>>> ret = nfs4_schedule_stateid_recovery(server, state);
>>>> if (ret < 0)
>>>> break;
>>>
>>> My remaining question here is: We don't seem to be communicating the
>>> _REVOKED state of the delegation to nfs4_schedule_stateid_recovery(),
>>> so won't we continue to loop if state != NULL?
>>
>> I’m not following. This patch is to handle the case when state is NULL and therefore we can’t initiate recovery. Before this patch, the problem was that when state was NULL we didn’t (a) clean up by doing delegreturn and (b) we weren’t retrying when we can — i.e. with a special stateid for SETATTR.
>>
>> If state is not null, I’m assuming the code is handling correctly by calling nfs4_schedule_stateid_recovery. The correctness of that code is beyond the scope of the problem i’m proposing to solve here.
>
> That's my point. I'm not seeing that the code is handling that case,
> and so I'm not seeing why we should treat state == NULL as being
> special.
>
The case “state==null” is special because state is null so we can’t initiate state recovery. While calling nfs4_inode_return_delegation is just to do internal clean up, the important piece in this case is to retry because in SETATTR case we have a special stateid to try. When state isn’t null, initiating recovery and trying to reclaim open state before returing the delegation state that’s the server is complaining about is needed before returning the delegation. Therefore calling nfs4_inode_return_delegation() would not be appropriate like it’s done in OPENMODE case.
Given current testing, I don’t see an issue of recovery from receiving BAD_STATEID on a synchronous operation when state is not NULL. Yet, I have a special test failure case for when state is NULL. Thus I’m trying to get this fix in. If another problem is uncovered for when state is not null, I think that would be the time to fix that problem?
>>
>>
>>> IOW: is there any reason not to do the same thing here that we already
>>> do for NFS4ERR_OPENMODE?
>>
>> The OPENMODE error mode code seems to have slightly different logic. It wants to return the delegation regardless of whether or not state is NULL.
>
> See above.
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
