| Message ID | cd03fb7419f886c8c79bb2ee4889dbc0768a1652.1693326366.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED | expand |
On Tue, 2023-08-29 at 12:26 -0400, Benjamin Coddington wrote: > This patch sets the SEQ4_STATUS_RECALLABLE_STATE_REVOKED bit for a single > SEQUENCE response after writing "revoke" to the client's ctl file in procfs. > It has been generally useful to test various NFS client implementations, so > I'm sending it along for others to find and use. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfsd/nfs4state.c | 19 +++++++++++++++---- > fs/nfsd/state.h | 1 + > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index daf305daa751..f91e2857df65 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2830,18 +2830,28 @@ static ssize_t client_ctl_write(struct file *file, const char __user *buf, > { > char *data; > struct nfs4_client *clp; > + ssize_t rc = size; > > data = simple_transaction_get(file, buf, size); > if (IS_ERR(data)) > return PTR_ERR(data); > - if (size != 7 || 0 != memcmp(data, "expire\n", 7)) > + > + if (size != 7) > return -EINVAL; > + > clp = get_nfsdfs_clp(file_inode(file)); > if (!clp) > return -ENXIO; > - force_expire_client(clp); > + > + if (!memcmp(data, "revoke\n", 7)) > + set_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags); > + else if (!memcmp(data, "expire\n", 7)) > + force_expire_client(clp); > + else > + rc = -EINVAL; > + > drop_client(clp); > - return 7; > + return rc; > } > > static const struct file_operations client_ctl_fops = { > @@ -4042,7 +4052,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > default: > seq->status_flags = 0; > } > - if (!list_empty(&clp->cl_revoked)) > + if (!list_empty(&clp->cl_revoked) || > + test_and_clear_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags)) > seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED; > out_no_session: > if (conn) > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index d49d3060ed4f..a9154b7da022 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -369,6 +369,7 @@ struct nfs4_client { > #define NFSD4_CLIENT_CB_FLAG_MASK (1 << NFSD4_CLIENT_CB_UPDATE | \ > 1 << NFSD4_CLIENT_CB_KILL) > #define NFSD4_CLIENT_CB_RECALL_ANY (6) > +#define NFSD4_CLIENT_CL_REVOKED (7) > unsigned long cl_flags; > const struct cred *cl_cb_cred; > struct rpc_clnt *cl_cb_client; Seems like a useful knob. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue, Aug 29, 2023 at 12:26:56PM -0400, Benjamin Coddington wrote: > This patch sets the SEQ4_STATUS_RECALLABLE_STATE_REVOKED bit for a single > SEQUENCE response after writing "revoke" to the client's ctl file in procfs. > It has been generally useful to test various NFS client implementations, so > I'm sending it along for others to find and use. Intriguing! It looks to me like the client would probe its state and find nothing missing... fair enough. Would it be even more useful if the server administrator could actually revoke some state, rather than just pretending to? How difficult do you think that might be? Or, conversely, what exactly can you test with this mechanism? > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfsd/nfs4state.c | 19 +++++++++++++++---- > fs/nfsd/state.h | 1 + > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index daf305daa751..f91e2857df65 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2830,18 +2830,28 @@ static ssize_t client_ctl_write(struct file *file, const char __user *buf, > { > char *data; > struct nfs4_client *clp; > + ssize_t rc = size; > > data = simple_transaction_get(file, buf, size); > if (IS_ERR(data)) > return PTR_ERR(data); > - if (size != 7 || 0 != memcmp(data, "expire\n", 7)) > + > + if (size != 7) > return -EINVAL; > + > clp = get_nfsdfs_clp(file_inode(file)); > if (!clp) > return -ENXIO; > - force_expire_client(clp); > + > + if (!memcmp(data, "revoke\n", 7)) > + set_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags); > + else if (!memcmp(data, "expire\n", 7)) > + force_expire_client(clp); > + else > + rc = -EINVAL; > + > drop_client(clp); > - return 7; > + return rc; > } > > static const struct file_operations client_ctl_fops = { > @@ -4042,7 +4052,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > default: > seq->status_flags = 0; > } > - if (!list_empty(&clp->cl_revoked)) > + if (!list_empty(&clp->cl_revoked) || > + test_and_clear_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags)) > seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED; > out_no_session: > if (conn) > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index d49d3060ed4f..a9154b7da022 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -369,6 +369,7 @@ struct nfs4_client { > #define NFSD4_CLIENT_CB_FLAG_MASK (1 << NFSD4_CLIENT_CB_UPDATE | \ > 1 << NFSD4_CLIENT_CB_KILL) > #define NFSD4_CLIENT_CB_RECALL_ANY (6) > +#define NFSD4_CLIENT_CL_REVOKED (7) > unsigned long cl_flags; > const struct cred *cl_cb_cred; > struct rpc_clnt *cl_cb_client; > -- > 2.40.1 >
On 29 Aug 2023, at 16:35, Chuck Lever wrote: > On Tue, Aug 29, 2023 at 12:26:56PM -0400, Benjamin Coddington wrote: >> This patch sets the SEQ4_STATUS_RECALLABLE_STATE_REVOKED bit for a single >> SEQUENCE response after writing "revoke" to the client's ctl file in procfs. >> It has been generally useful to test various NFS client implementations, so >> I'm sending it along for others to find and use. > > Intriguing! > > It looks to me like the client would probe its state and > find nothing missing... fair enough. > > Would it be even more useful if the server administrator could > actually revoke some state, rather than just pretending to? > How difficult do you think that might be? Probably not difficult, we'd just move some state to cl_revoked. We'd need to work out a syntax for it, or add another procfs file, but I don't have any need for this.. yet. > Or, conversely, what exactly can you test with this mechanism? You can test how well the client performs TEST_STATEID for all its known state, looking for unfairness or perhaps skipped state. Doing a TEST_STATEID walk can be a big lift for clients with a lot of delegations. There's been recent reports of the linux client getting into a live jam trying to use TEST_STATEID to satisfy a server that keeps setting this bit. Its pretty hard to catch in the wild, and network captures are typically too large to handle. In the process of trying to optimize how the client performs TEST_STATEID ops for all its state, this patch made it easier to artificially trigger that client behavior. I'm ambivalent about whether this goes upstream, but here it is if others find it useful. Ben
On Wed, 30 Aug 2023, Chuck Lever wrote: > On Tue, Aug 29, 2023 at 12:26:56PM -0400, Benjamin Coddington wrote: > > This patch sets the SEQ4_STATUS_RECALLABLE_STATE_REVOKED bit for a single > > SEQUENCE response after writing "revoke" to the client's ctl file in procfs. > > It has been generally useful to test various NFS client implementations, so > > I'm sending it along for others to find and use. > > Intriguing! > > It looks to me like the client would probe its state and > find nothing missing... fair enough. > > Would it be even more useful if the server administrator could > actually revoke some state, rather than just pretending to? > How difficult do you think that might be? I have patches for revoking state. I sent them some time ago and we discussed them, but you wanted to wait for the courteous client code to land (which was fair enough) and then I forgot to come back with them. I'll try to make time to repost them. NeilBrown > > Or, conversely, what exactly can you test with this mechanism? > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfsd/nfs4state.c | 19 +++++++++++++++---- > > fs/nfsd/state.h | 1 + > > 2 files changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index daf305daa751..f91e2857df65 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -2830,18 +2830,28 @@ static ssize_t client_ctl_write(struct file *file, const char __user *buf, > > { > > char *data; > > struct nfs4_client *clp; > > + ssize_t rc = size; > > > > data = simple_transaction_get(file, buf, size); > > if (IS_ERR(data)) > > return PTR_ERR(data); > > - if (size != 7 || 0 != memcmp(data, "expire\n", 7)) > > + > > + if (size != 7) > > return -EINVAL; > > + > > clp = get_nfsdfs_clp(file_inode(file)); > > if (!clp) > > return -ENXIO; > > - force_expire_client(clp); > > + > > + if (!memcmp(data, "revoke\n", 7)) > > + set_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags); > > + else if (!memcmp(data, "expire\n", 7)) > > + force_expire_client(clp); > > + else > > + rc = -EINVAL; > > + > > drop_client(clp); > > - return 7; > > + return rc; > > } > > > > static const struct file_operations client_ctl_fops = { > > @@ -4042,7 +4052,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > default: > > seq->status_flags = 0; > > } > > - if (!list_empty(&clp->cl_revoked)) > > + if (!list_empty(&clp->cl_revoked) || > > + test_and_clear_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags)) > > seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED; > > out_no_session: > > if (conn) > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index d49d3060ed4f..a9154b7da022 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -369,6 +369,7 @@ struct nfs4_client { > > #define NFSD4_CLIENT_CB_FLAG_MASK (1 << NFSD4_CLIENT_CB_UPDATE | \ > > 1 << NFSD4_CLIENT_CB_KILL) > > #define NFSD4_CLIENT_CB_RECALL_ANY (6) > > +#define NFSD4_CLIENT_CL_REVOKED (7) > > unsigned long cl_flags; > > const struct cred *cl_cb_cred; > > struct rpc_clnt *cl_cb_client; > > -- > > 2.40.1 > > > > -- > Chuck Lever >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index daf305daa751..f91e2857df65 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -2830,18 +2830,28 @@ static ssize_t client_ctl_write(struct file *file, const char __user *buf, { char *data; struct nfs4_client *clp; + ssize_t rc = size; data = simple_transaction_get(file, buf, size); if (IS_ERR(data)) return PTR_ERR(data); - if (size != 7 || 0 != memcmp(data, "expire\n", 7)) + + if (size != 7) return -EINVAL; + clp = get_nfsdfs_clp(file_inode(file)); if (!clp) return -ENXIO; - force_expire_client(clp); + + if (!memcmp(data, "revoke\n", 7)) + set_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags); + else if (!memcmp(data, "expire\n", 7)) + force_expire_client(clp); + else + rc = -EINVAL; + drop_client(clp); - return 7; + return rc; } static const struct file_operations client_ctl_fops = { @@ -4042,7 +4052,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, default: seq->status_flags = 0; } - if (!list_empty(&clp->cl_revoked)) + if (!list_empty(&clp->cl_revoked) || + test_and_clear_bit(NFSD4_CLIENT_CL_REVOKED, &clp->cl_flags)) seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED; out_no_session: if (conn) diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index d49d3060ed4f..a9154b7da022 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -369,6 +369,7 @@ struct nfs4_client { #define NFSD4_CLIENT_CB_FLAG_MASK (1 << NFSD4_CLIENT_CB_UPDATE | \ 1 << NFSD4_CLIENT_CB_KILL) #define NFSD4_CLIENT_CB_RECALL_ANY (6) +#define NFSD4_CLIENT_CL_REVOKED (7) unsigned long cl_flags; const struct cred *cl_cb_cred; struct rpc_clnt *cl_cb_client;
This patch sets the SEQ4_STATUS_RECALLABLE_STATE_REVOKED bit for a single SEQUENCE response after writing "revoke" to the client's ctl file in procfs. It has been generally useful to test various NFS client implementations, so I'm sending it along for others to find and use. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfsd/nfs4state.c | 19 +++++++++++++++---- fs/nfsd/state.h | 1 + 2 files changed, 16 insertions(+), 4 deletions(-)