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

[Benny Halevy <bhalevy@panasas.com>]

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

Benny

> Fred
> 
>> Benny
>>
>>> +     }
>>> +     if (overwrite)
>>> +             memcpy(&old->stateid, &new->stateid, sizeof(new->stateid));
>>>       write_sequnlock(&lo->seqlock);
>>>  }
>>>
>> --
>> 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
>>