On Fri, Oct 8, 2010 at 11:36 AM, P.B.Shelley <shelleypt@xxxxxxxxx> wrote:
> On Fri, Oct 8, 2010 at 10:35 PM, Fred Isaman <iisaman@xxxxxxxxxx> wrote:
>> On Fri, Oct 8, 2010 at 10:13 AM, P.B.Shelley <shelleypt@xxxxxxxxx> 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)))
>>>>>>> + 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?
>
No (see 12.5.2). But the spec seems to allow sending multiple
LAYOUTGETs with the open stateid until it processes the first reply
which includes a proper layout stateid.
Fred
> --
> 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
