Re: [PATCH 2/4] pnfs_submit: Only update stateid if it is more recent than current

["P.B.Shelley" <shelleypt@gmail.com>]

On Fri, Oct 8, 2010 at 10:35 PM, Fred Isaman <iisaman@netapp.com> wrote:
> On Fri, Oct 8, 2010 at 10:13 AM, P.B.Shelley <shelleypt@gmail.com> wrote:
>> On Thu, Oct 7, 2010 at 10:06 PM, Benny Halevy <bhalevy@panasas.com> wrote:
>>> On 2010-10-07 10:01, Fred Isaman wrote:
>>>> On Thu, Oct 7, 2010 at 9:34 AM, Benny Halevy <bhalevy@panasas.com> wrote:
>>>>> On 2010-10-06 16:35, Fred Isaman wrote:
>>>>>> Right now, when we set the stateid, we blindly overwrite the current
>>>>>> one, allowing the seqid to incorrectly roll backward.
>>>>>>
>>>>>> Signed-off-by: Fred Isaman <iisaman@netapp.com>
>>>>>> ---
>>>>>>  fs/nfs/pnfs.c |   38 ++++++++++++++++++++++++++++++++------
>>>>>>  1 files changed, 32 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>>>> index 39bce9b..555955b 100644
>>>>>> --- a/fs/nfs/pnfs.c
>>>>>> +++ b/fs/nfs/pnfs.c
>>>>>> @@ -459,16 +459,42 @@ pnfs_destroy_all_layouts(struct nfs_client *clp)
>>>>>>       }
>>>>>>  }
>>>>>>
>>>>>> +/* update lo->stateid with new if is more recent
>>>>>> + *
>>>>>> + * lo->stateid could be the open stateid, in which case we just use what given.
>>>>>> + */
>>>>>>  static void
>>>>>>  pnfs_set_layout_stateid(struct pnfs_layout_hdr *lo,
>>>>>> -                     const nfs4_stateid *stateid)
>>>>>> +                     const nfs4_stateid *new)
>>>>>>  {
>>>>>> -     /* TODO - should enforce that embedded seqid, in the case
>>>>>> -      * that the two stateid.others are equal,  only increases.
>>>>>> -      * Complicated by wrap-around.
>>>>>> -      */
>>>>>> +     nfs4_stateid *old = &lo->stateid;
>>>>>> +     bool overwrite = false;
>>>>>> +
>>>>>>       write_seqlock(&lo->seqlock);
>>>>>> -     memcpy(lo->stateid.data, stateid->data, sizeof(lo->stateid.data));
>>>>>> +     if (!test_bit(NFS_LAYOUT_STATEID_SET, &lo->state) ||
>>>>>> +         memcmp(old->stateid.other, new->stateid.other, sizeof(new->stateid.other)))
>>>>>> +             overwrite = true;
>>>>>> +     else {
>>>>>> +             u32 oldseq, newseq, limit;
>>>>>> +
>>>>>> +             oldseq = be32_to_cpu(old->stateid.seqid);
>>>>>> +             newseq = be32_to_cpu(new->stateid.seqid);
>>>>>> +             /* There are no good bounds on window size, so just
>>>>>> +              * use a ridiculously large window of 2^31.
>>>>>> +              */
>>>>>> +             limit = oldseq + (1 << 31);
>>>>>> +             if (oldseq < limit) {
>>>>>> +                     /* The easy, non-wraparound case */
>>>>>> +                     if (oldseq < newseq && newseq < limit)
>>>>>> +                             overwrite = true;
>>>>>> +             } else {
>>>>>> +                     /* Near wraparound edge */
>>>>>> +                     if (oldseq < newseq || newseq < limit)
>>>>>> +                             overwrite = true;
>>>>>> +             }
>>>>>
>>>>> Wouldn't it be simpler to just look at (int32_t)(newseq - oldseq)?
>>>>>
>>>>
>>>> Why yes it would.  I'll send a new version of this patch shortly.
>>>>
>>>
>>> No need :)
>>> I'll just change this as follows:
>>>
>>> +       else {
>>> +               u32 oldseq, newseq, limit;
>>> +
>>> +               oldseq = be32_to_cpu(old->stateid.seqid);
>>> +               newseq = be32_to_cpu(new->stateid.seqid);
>>> +               if ((int)(newseq - oldseq) > 0)
>>> +                       overwrite = true;
>> Do we also need to verify the other field of the stateid? Will there
>> be situations that server change the other field and reset the seqid?
>
> The server is going to use the "other" we sent, except in the case we
> sent an open stateid.  The only potential for trouble I see is if a
> LAYOUTGET reply gets lost in the network for a long time and is
> received after the layout stateid has been reset for some reason.
> However, that implies an error elsewhere (which may well exist at the
> moment...careful stateid handling is next on the agenda), as we should
> have been waiting for that lseg to arrive before continuing.
Oops, I missed the other field comparing code. Thank you for pointing
it out, Benny.

Can client choose to send an open stateid for LATYOUTGET request, even
if client has a layout stateid for the file?

-- 
Thanks,
Shelley