Return-Path: Received: from mail-bw0-f46.google.com ([209.85.214.46]:46709 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752377Ab0JHPg0 convert rfc822-to-8bit (ORCPT ); Fri, 8 Oct 2010 11:36:26 -0400 Received: by bwz15 with SMTP id 15so350198bwz.19 for ; Fri, 08 Oct 2010 08:36:24 -0700 (PDT) In-Reply-To: References: <1286397317-17881-1-git-send-email-iisaman@netapp.com> <1286397317-17881-3-git-send-email-iisaman@netapp.com> <4CADCC48.4030908@panasas.com> <4CADD3E9.7040307@panasas.com> From: "P.B.Shelley" Date: Fri, 8 Oct 2010 23:36:02 +0800 Message-ID: Subject: Re: [PATCH 2/4] pnfs_submit: Only update stateid if it is more recent than current To: Fred Isaman Cc: Benny Halevy , linux-nfs@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Fri, Oct 8, 2010 at 10:35 PM, Fred Isaman wrote: > On Fri, Oct 8, 2010 at 10:13 AM, P.B.Shelley wrote: >> On Thu, Oct 7, 2010 at 10:06 PM, Benny Halevy wrote: >>> On 2010-10-07 10:01, Fred Isaman wrote: >>>> On Thu, Oct 7, 2010 at 9:34 AM, Benny Halevy 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 >>>>>> --- >>>>>>  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? -- Thanks, Shelley