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.
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
