[PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[Benjamin Coddington]

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;
-- 
2.40.1

Re: [PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[Jeff Layton]

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>

Re: [PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[Chuck Lever]

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
> 

-- 
Chuck Lever

Re: [PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[Benjamin Coddington]

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

Re: [PATCH] nfsd: allow setting SEQ4_STATUS_RECALLABLE_STATE_REVOKED

[NeilBrown]

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
>