Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-qg0-f53.google.com ([209.85.192.53]:33415 "EHLO mail-qg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751176AbaIIVTw (ORCPT ); Tue, 9 Sep 2014 17:19:52 -0400 Received: by mail-qg0-f53.google.com with SMTP id q108so896492qgd.26 for ; Tue, 09 Sep 2014 14:19:51 -0700 (PDT) From: Jeff Layton Date: Tue, 9 Sep 2014 17:19:49 -0400 To: "J. Bruce Fields" Cc: linux-nfs@vger.kernel.org, steved@redhat.com Subject: Re: [PATCH v3 7/8] nfsd: set and test NFSD4_CLIENT_STABLE bit to reduce nfsdcltrack upcalls Message-ID: <20140909171949.36766a60@tlielax.poochiereds.net> In-Reply-To: <20140909165651.3f373521@tlielax.poochiereds.net> References: <1410187609-10319-1-git-send-email-jlayton@primarydata.com> <1410187609-10319-8-git-send-email-jlayton@primarydata.com> <20140909204617.GC17979@fieldses.org> <20140909165651.3f373521@tlielax.poochiereds.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, 9 Sep 2014 16:56:51 -0400 Jeff Layton wrote: > On Tue, 9 Sep 2014 16:46:17 -0400 > "J. Bruce Fields" wrote: > > > On Mon, Sep 08, 2014 at 10:46:48AM -0400, Jeff Layton wrote: > > > The nfsdcltrack upcall doesn't utilize the NFSD4_CLIENT_STABLE flag, > > > which basically results in an upcall every time we call into the client > > > tracking ops. > > > > > > Change it to set this bit on a successful "check" or "create" request, > > > and clear it on a "remove" request. Also, check to see if that bit is > > > set before upcalling on a "check" or "remove" request, and skip > > > upcalling appropriately, depending on its state. > > > > > > Signed-off-by: Jeff Layton > > > --- > > > fs/nfsd/nfs4recover.c | 22 ++++++++++++++++++---- > > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > > > index ae313861a6f5..5697e9d2e875 100644 > > > --- a/fs/nfsd/nfs4recover.c > > > +++ b/fs/nfsd/nfs4recover.c > > > @@ -1291,7 +1291,8 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp) > > > grace_start = nfsd4_cltrack_grace_start(nn->boot_time); > > > > > > nfsd4_cltrack_upcall_lock(clp); > > > - nfsd4_umh_cltrack_upcall("create", hexid, reclaim_complete, grace_start); > > > + if (!nfsd4_umh_cltrack_upcall("create", hexid, reclaim_complete, grace_start)) > > > + set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags); > > > > Don't you also want to skip the upcall when STABLE is set? > > > > (Not a big deal in the 4.1 case as it's only called once per client (in > > RECLAIM_COMPLETE) but annoying in the 4.0 case where it's called on > > every OPEN_CONFIRM.) > > > > --b. > > > > Ahh right, I was thinking that we'd only get one OPEN_CONFIRM per > client, but you have to do it for each openowner... > > No, we can't do that. Consider the case where we might do some "check" > ops before the "create" operation (i.e. we need to do some reclaims). > > The "check" will set the STABLE flag, and then the create operation > gets skipped. That means that the grace period won't get lifted early > even when the RECLAIM_COMPLETE comes in since the reclaim_complete > field in the DB didn't get updated. > > We could (in principle) add a new flag that indicates whether a > "create" has already been done for a client, and use that to skip > subsequent "create" ops. Sound reasonable? > ...or we could skip doing the upcall if the client's minorversion is 0 and the STABLE flag is set. In the v4.0 case, there's really no difference between a "check" and "create" operation, so there's no real benefit to doing a "create" if you already did a "check". > > > nfsd4_cltrack_upcall_unlock(clp); > > > > > > kfree(reclaim_complete); > > > @@ -1304,6 +1305,9 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp) > > > { > > > char *hexid; > > > > > > + if (!test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) > > > + return; > > > + > > > hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len); > > > if (!hexid) { > > > dprintk("%s: can't allocate memory for upcall!\n", __func__); > > > @@ -1311,7 +1315,9 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp) > > > } > > > > > > nfsd4_cltrack_upcall_lock(clp); > > > - nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL); > > > + if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags) && > > > + nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL) == 0) > > > + clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags); > > > nfsd4_cltrack_upcall_unlock(clp); > > > > > > kfree(hexid); > > > @@ -1323,6 +1329,9 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp) > > > int ret; > > > char *hexid, *legacy; > > > > > > + if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) > > > + return 0; > > > + > > > hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len); > > > if (!hexid) { > > > dprintk("%s: can't allocate memory for upcall!\n", __func__); > > > @@ -1331,9 +1340,14 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp) > > > legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name); > > > > > > nfsd4_cltrack_upcall_lock(clp); > > > - ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL); > > > + if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) { > > > + ret = 0; > > > + } else { > > > + ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy, NULL); > > > + if (!ret) > > > + set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags); > > > + } > > > nfsd4_cltrack_upcall_unlock(clp); > > > - > > > kfree(legacy); > > > kfree(hexid); > > > return ret; > > > -- > > > 1.9.3 > > > > > -- Jeff Layton