On 2010-10-08 10:13, P.B.Shelley wrote:
> On Thu, Oct 7, 2010 at 10:06 PM, Benny Halevy <bhalevy@xxxxxxxxxxx> wrote:
>> On 2010-10-07 10:01, Fred Isaman wrote:
>>> On Thu, Oct 7, 2010 at 9:34 AM, Benny Halevy <bhalevy@xxxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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)))
here we check the 'other' part.
>>>>> + 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?
We do :)
see block above.
Benny
>
> Thanks,
> Shelley
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
