Return-Path: Received: from mail-bw0-f46.google.com ([209.85.214.46]:60731 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758619Ab0JHPwP convert rfc822-to-8bit (ORCPT ); Fri, 8 Oct 2010 11:52:15 -0400 Received: by bwz15 with SMTP id 15so365541bwz.19 for ; Fri, 08 Oct 2010 08:52:13 -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> Date: Fri, 8 Oct 2010 11:52:13 -0400 Message-ID: Subject: Re: [PATCH 2/4] pnfs_submit: Only update stateid if it is more recent than current From: Fred Isaman To: "P.B.Shelley" Cc: Benny Halevy , linux-nfs@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Fri, Oct 8, 2010 at 11:36 AM, P.B.Shelley wrote: > 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? > 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@vger.kernel.org > More majordomo info at ?http://vger.kernel.org/majordomo-info.html >