| Message ID | 1410187609-10319-8-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <jlayton@primarydata.com> > --- > 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. > 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 > -- 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
On Tue, 9 Sep 2014 16:46:17 -0400 "J. Bruce Fields" <bfields@fieldses.org> 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 <jlayton@primarydata.com> > > --- > > 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? > > 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 > >
On Tue, 9 Sep 2014 16:56:51 -0400 Jeff Layton <jlayton@primarydata.com> wrote: > On Tue, 9 Sep 2014 16:46:17 -0400 > "J. Bruce Fields" <bfields@fieldses.org> 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 <jlayton@primarydata.com> > > > --- > > > 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 > > > > >
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); 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;
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 <jlayton@primarydata.com> --- fs/nfsd/nfs4recover.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)