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