Return-Path: Received: from mail-yk0-f175.google.com ([209.85.160.175]:34043 "EHLO mail-yk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751205AbbGQRms (ORCPT ); Fri, 17 Jul 2015 13:42:48 -0400 Received: by ykax123 with SMTP id x123so95429834yka.1 for ; Fri, 17 Jul 2015 10:42:47 -0700 (PDT) Date: Fri, 17 Jul 2015 13:42:43 -0400 From: Jeff Layton To: "J. Bruce Fields" Cc: Kinglong Mee , "linux-nfs@vger.kernel.org" Subject: Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid() Message-ID: <20150717134243.7bf809fa@tlielax.poochiereds.net> In-Reply-To: <20150717155846.GC6263@fieldses.org> References: <55A384B1.8030207@gmail.com> <55A38505.6040405@gmail.com> <20150715204748.GA21669@fieldses.org> <20150715204950.GB21669@fieldses.org> <55A726A0.5090907@gmail.com> <55A72A1F.7060507@gmail.com> <20150717155846.GC6263@fieldses.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, 17 Jul 2015 11:58:46 -0400 "J. Bruce Fields" wrote: > On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote: > > On 7/16/2015 11:36, Kinglong Mee wrote: > > > On 7/16/2015 04:49, J. Bruce Fields wrote: > > >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote: > > >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote: > > >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" > > >>>> have moved gen_confirm() to gen_clid(). > > >>> > > >>> This means the statement in that earlier commit is wrong: > > >>> > > >>> > > >>> With this, there's no need to keep two counters as they'd always > > >>> be in sync anyway, so just use the clientid_counter for both. > > >>> > > >>> Looks to me like this may need a separate counter to eliminate the > > >>> possibibility of returning the same confirm twice for a one clientid? > > > > > > Yes, nfsd will generate same confirm for one clientid in one second. > > > > > > verf[0] = (__force __be32)jiffies; > > > verf[1] = (__force __be32)nn->clientid_counter; > > > > > > for case 1: probable callback update, the new unconf client needs > > > a different confirm. > > > > Ignore this patch, and just revert commit 294ac32e99 > > "nfsd: protect clid and verifier generation with client_lock" > > is a better solve. > > We can't revert that completely, it does fix a real locking bug at > least, I think. > > I'd agree to reinstating a separate counter for the verifier. That > verifier probably also needs to be per-network namespace to make the > per-network-namespace locking correct. > > --b. > Sorry, just getting caught up on this. At the time I was just "following the code" and not necessarily paying much attention to the spec. Yes, I agree -- a separate counter sounds like the right fix for now, in conjunction with Kinglong's patch (or something like it). > > > > thanks, > > Kinglong Mee > > > > > > > > Rereading rfc7530, > > > x be the value of the client.id subfield of the SETCLIENTID4args > > > structure. > > > > > > v be the value of the client.verifier subfield of the > > > SETCLIENTID4args structure. > > > > > > c be the value of the client ID field returned in the > > > SETCLIENTID4resok structure. > > > > > > k represent the value combination of the callback and callback_ident > > > fields of the SETCLIENTID4args structure. > > > > > > s be the setclientid_confirm value returned in the SETCLIENTID4resok > > > structure. > > > > > > { v, x, c, k, s } be a quintuple for a client record. A client > > > record is confirmed if there has been a SETCLIENTID_CONFIRM > > > operation to confirm it. Otherwise, it is unconfirmed. An > > > unconfirmed record is established by a SETCLIENTID call. > > > > > > ... /* case 1: probable callback update */ ... > > > > > > o The server checks if it has recorded a confirmed record for { v, > > > x, c, l, s }, where l may or may not equal k. If so, and since > > > the id verifier v of the request matches that which is confirmed > > > and recorded, the server treats this as a probable callback > > > information update and records an unconfirmed { v, x, c, k, t } > > > and leaves the confirmed { v, x, c, l, s } in place, such that > > > t != s. It does not matter whether k equals l or not. Any > > > pre-existing unconfirmed { v, x, c, *, * } is removed. > > > > > > The server returns { c, t }. It is indeed returning the old > > > clientid4 value c, because the client apparently only wants to > > > update callback value k to value l. It's possible this request is > > > one from the Byzantine router that has stale callback information, > > > but this is not a problem. The callback information update is > > > only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }. > > > > > > The server awaits confirmation of k via SETCLIENTID_CONFIRM > > > { c, t }. > > > > > > The server does NOT remove client (lock/share/delegation) state > > > for x. > > > > > >> > > >> (but frankly I can never completely review changes to the > > >> setclientid/setclientid_confirm behavior without rereading RFC 7530 > > >> 16.33.5 every time, which is a slog. Might help to contrive a pynfs > > >> test derived from that text which tests for this particular behavior.) > > >> > > > > > > Make sense. > > > I will make it later. > > > > > > thanks, > > > Kinglong Mee > > > > > > > > >>> > > >>> --b. > > >>> > > >>>> > > >>>> After it, setclientid will return a bad reply with all zero confirms > > >>>> after copy_clid(). > > >>>> > > >>>> Signed-off-by: Kinglong Mee > > >>>> --- > > >>>> fs/nfsd/nfs4state.c | 5 +++-- > > >>>> 1 file changed, 3 insertions(+), 2 deletions(-) > > >>>> > > >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > >>>> index e0a4556..b1f84fc 100644 > > >>>> --- a/fs/nfsd/nfs4state.c > > >>>> +++ b/fs/nfsd/nfs4state.c > > >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > >>>> unconf = find_unconfirmed_client_by_name(&clname, nn); > > >>>> if (unconf) > > >>>> unhash_client_locked(unconf); > > >>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier)) > > >>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) { > > >>>> /* case 1: probable callback update */ > > >>>> copy_clid(new, conf); > > >>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */ > > >>>> + gen_confirm(new, nn); > > >>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */ > > >>>> gen_clid(new, nn); > > >>>> new->cl_minorversion = 0; > > >>>> gen_callback(new, setclid, rqstp); > > >>>> -- > > >>>> 2.4.3 > > >> > > > > -- > 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 -- Jeff Layton