[PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

We added this new estimator function but forgot to hook it up.  The
effect is that NFSv4.1 won't do zero-copy reads.

The estimate was also wrong by 8 bytes.

Fixes: ccae70a9ee41 "nfsd4: estimate sequence response size"
Cc: stable@vger.kernel.org
Reported-by: Chuck Lever <chucklever@gmail.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Whoops, this should have gone in some time ago but I lost it somehow.
Thanks to Chuck for noticing the oversight.

I'll pass it along upstream soon.--b.

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index cdeb3cf..f990983 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1589,7 +1589,8 @@ static inline u32 nfsd4_rename_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op
 static inline u32 nfsd4_sequence_rsize(struct svc_rqst *rqstp,
 				       struct nfsd4_op *op)
 {
-	return NFS4_MAX_SESSIONID_LEN + 20;
+	return (op_encode_hdr_size
+		+ XDR_QUADLEN(NFS4_MAX_SESSIONID_LEN + 5)) * sizeof(__be32);
 }
 
 static inline u32 nfsd4_setattr_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
@@ -1893,6 +1894,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
 		.op_func = (nfsd4op_func)nfsd4_sequence,
 		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
 		.op_name = "OP_SEQUENCE",
+		.op_rsize_bop = (nfsd4op_rsize)nfsd4_sequence_rsize,
 	},
 	[OP_DESTROY_CLIENTID] = {
 		.op_func = (nfsd4op_func)nfsd4_destroy_clientid,
-- 
1.9.1

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[Christoph Hellwig]

On Fri, Oct 17, 2014 at 05:24:46PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We added this new estimator function but forgot to hook it up.  The
> effect is that NFSv4.1 won't do zero-copy reads.
> 
> The estimate was also wrong by 8 bytes.

This would affect 4.0 and 4.2 as well, wouldn't it?

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[J. Bruce Fields]

On Tue, Oct 21, 2014 at 03:36:31AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 17, 2014 at 05:24:46PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > We added this new estimator function but forgot to hook it up.  The
> > effect is that NFSv4.1 won't do zero-copy reads.
> > 
> > The estimate was also wrong by 8 bytes.
> 
> This would affect 4.0 and 4.2 as well, wouldn't it?

It was introduced in 4.1, so yes to 4.2, no to 4.1.

Also, this still had an arithmetic mistake. Fixed version follows.

Also my tests are failing due to an unrelated crash in 18-rc1 which I
want to track down before sending this in.

--b.

commit d1d84c9626bb3a519863b3ffc40d347166f9fb83
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Thu Aug 21 15:04:31 2014 -0400

    nfsd4: fix response size estimation for OP_SEQUENCE
    
    We added this new estimator function but forgot to hook it up.  The
    effect is that NFSv4.1 (and greater) won't do zero-copy reads.
    
    The estimate was also wrong by 8 bytes.
    
    Fixes: ccae70a9ee41 "nfsd4: estimate sequence response size"
    Cc: stable@vger.kernel.org
    Reported-by: Chuck Lever <chucklever@gmail.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index cdeb3cf..f4bd578 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1589,7 +1589,8 @@ static inline u32 nfsd4_rename_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op
 static inline u32 nfsd4_sequence_rsize(struct svc_rqst *rqstp,
 				       struct nfsd4_op *op)
 {
-	return NFS4_MAX_SESSIONID_LEN + 20;
+	return (op_encode_hdr_size
+		+ XDR_QUADLEN(NFS4_MAX_SESSIONID_LEN) + 5) * sizeof(__be32);
 }
 
 static inline u32 nfsd4_setattr_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
@@ -1893,6 +1894,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
 		.op_func = (nfsd4op_func)nfsd4_sequence,
 		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
 		.op_name = "OP_SEQUENCE",
+		.op_rsize_bop = (nfsd4op_rsize)nfsd4_sequence_rsize,
 	},
 	[OP_DESTROY_CLIENTID] = {
 		.op_func = (nfsd4op_func)nfsd4_destroy_clientid,

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[J. Bruce Fields]

On Tue, Oct 21, 2014 at 09:14:06AM -0400, J. Bruce Fields wrote:
> Also my tests are failing due to an unrelated crash in 18-rc1 which I
> want to track down before sending this in.

There are two bugs:

	- the client is sending SEEK over minorversion 1.
	- this sometimes causes the server to crash.

I'm testing a fix for the latter.

On the former: looks like if 4.2 support is built in, then llseek is set
to nfs4_file_llseek, which unconditionally calls nfs42_proc_llseek().

Does nfs4_file_llseek need an explicit minorversion check, or should it
be handled some other way?

--b.

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[Anna Schumaker]

On 10/22/14 15:22, J. Bruce Fields wrote:
> On Tue, Oct 21, 2014 at 09:14:06AM -0400, J. Bruce Fields wrote:
>> Also my tests are failing due to an unrelated crash in 18-rc1 which I
>> want to track down before sending this in.
> 
> There are two bugs:
> 
> 	- the client is sending SEEK over minorversion 1.
> 	- this sometimes causes the server to crash.
> 
> I'm testing a fix for the latter.
> 
> On the former: looks like if 4.2 support is built in, then llseek is set
> to nfs4_file_llseek, which unconditionally calls nfs42_proc_llseek().
> 
> Does nfs4_file_llseek need an explicit minorversion check, or should it
> be handled some other way?

The client should be checking for CAP_SEEK, which should only be set on NFS v4.2.  I'll look into this!
> 
> --b.
> 

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[J. Bruce Fields]

On Wed, Oct 22, 2014 at 03:22:58PM -0400, J. Bruce Fields wrote:
> On Tue, Oct 21, 2014 at 09:14:06AM -0400, J. Bruce Fields wrote:
> > Also my tests are failing due to an unrelated crash in 18-rc1 which I
> > want to track down before sending this in.
> 
> There are two bugs:
> 
> 	- the client is sending SEEK over minorversion 1.
> 	- this sometimes causes the server to crash.
> 
> I'm testing a fix for the latter.
> 
> On the former: looks like if 4.2 support is built in, then llseek is set
> to nfs4_file_llseek, which unconditionally calls nfs42_proc_llseek().
> 
> Does nfs4_file_llseek need an explicit minorversion check, or should it
> be handled some other way?

By the way, a purely theoretical issue for now, but: on unknown
operations the server returns either NFS4ERR_OP_ILLEGAL or
NFS4ERR_OP_NOTSUPP depending on whether we think it's in the range of
defined nfs4.2 operations.

That means that if a revision of the 4.2 draft adds a new operation
beyond the current end (OP_WRITE_SAME = 70), a client would need to be
prepared for old servers returning OP_ILLEGAL to that operation.

Freezing the definitions of the ops and attributes we care about may not
be quite enough to make implementing-as-we-go-along work?

--b.

[PATCH] nfsd4: fix crash on unknown operation number

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Unknown operation numbers are caught in nfsd4_decode_compound() which
sets op->opnum to OP_ILLEGAL and op->status to nfserr_op_illegal.  The
error causes the main loop in nfsd4_proc_compound() to skip most
processing.  But nfsd4_proc_compound also peeks ahead at the next
operation in one case and doesn't take similar precautions there.

Cc: stable@vger.kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Wed, Oct 22, 2014 at 03:2
> There are two bugs:
> 
> 	- the client is sending SEEK over minorversion 1.
> 	- this sometimes causes the server to crash.
> 
> I'm testing a fix for the latter.

I think this is all it needs.

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f4bd578bed55..0beb023f25ac 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1272,7 +1272,8 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
 	 */
 	if (argp->opcnt == resp->opcnt)
 		return false;
-
+	if (next->opnum == OP_ILLEGAL)
+		return false;
 	nextd = OPDESC(next);
 	/*
 	 * Rest of 2.6.3.1.1: certain operations will return WRONGSEC
-- 
1.9.3

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[Tom Haynes]

> On Oct 22, 2014, at 12:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
>> On Wed, Oct 22, 2014 at 03:22:58PM -0400, J. Bruce Fields wrote:
>>> On Tue, Oct 21, 2014 at 09:14:06AM -0400, J. Bruce Fields wrote:
>>> Also my tests are failing due to an unrelated crash in 18-rc1 which I
>>> want to track down before sending this in.
>> 
>> There are two bugs:
>> 
>>    - the client is sending SEEK over minorversion 1.
>>    - this sometimes causes the server to crash.
>> 
>> I'm testing a fix for the latter.
>> 
>> On the former: looks like if 4.2 support is built in, then llseek is set
>> to nfs4_file_llseek, which unconditionally calls nfs42_proc_llseek().
>> 
>> Does nfs4_file_llseek need an explicit minorversion check, or should it
>> be handled some other way?
> 
> By the way, a purely theoretical issue for now, but: on unknown
> operations the server returns either NFS4ERR_OP_ILLEGAL or
> NFS4ERR_OP_NOTSUPP depending on whether we think it's in the range of
> defined nfs4.2 operations.
> 
> That means that if a revision of the 4.2 draft adds a new operation
> beyond the current end (OP_WRITE_SAME = 70), a client would need to be
> prepared for old servers returning OP_ILLEGAL to that operation.
> 

Or if the new minor versioning rules take effect...



> Freezing the definitions of the ops and attributes we care about may not
> be quite enough to make implementing-as-we-go-along work?
> 
> --b.
> --
> 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

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[Christoph Hellwig]

On Wed, Oct 22, 2014 at 01:12:12PM -0700, Tom Haynes wrote:
> > That means that if a revision of the 4.2 draft adds a new operation
> > beyond the current end (OP_WRITE_SAME = 70), a client would need to be
> > prepared for old servers returning OP_ILLEGAL to that operation.
> > 
> 
> Or if the new minor versioning rules take effect...

I guess that's a good reason to start future proofing clients to treat
OP_ILLEGAL the same as NFS4ERR_NOTSUP.

Re: [PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[Jeff Layton]

On Tue, 21 Oct 2014 09:14:06 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Oct 21, 2014 at 03:36:31AM -0700, Christoph Hellwig wrote:
> > On Fri, Oct 17, 2014 at 05:24:46PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > We added this new estimator function but forgot to hook it up.  The
> > > effect is that NFSv4.1 won't do zero-copy reads.
> > > 
> > > The estimate was also wrong by 8 bytes.
> > 
> > This would affect 4.0 and 4.2 as well, wouldn't it?
> 
> It was introduced in 4.1, so yes to 4.2, no to 4.1.
> 
> Also, this still had an arithmetic mistake. Fixed version follows.
> 
> Also my tests are failing due to an unrelated crash in 18-rc1 which I
> want to track down before sending this in.
> 
> --b.
> 
> commit d1d84c9626bb3a519863b3ffc40d347166f9fb83
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Thu Aug 21 15:04:31 2014 -0400
> 
>     nfsd4: fix response size estimation for OP_SEQUENCE
>     
>     We added this new estimator function but forgot to hook it up.  The
>     effect is that NFSv4.1 (and greater) won't do zero-copy reads.
>     
>     The estimate was also wrong by 8 bytes.
>     
>     Fixes: ccae70a9ee41 "nfsd4: estimate sequence response size"
>     Cc: stable@vger.kernel.org
>     Reported-by: Chuck Lever <chucklever@gmail.com>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index cdeb3cf..f4bd578 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1589,7 +1589,8 @@ static inline u32 nfsd4_rename_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op
>  static inline u32 nfsd4_sequence_rsize(struct svc_rqst *rqstp,
>  				       struct nfsd4_op *op)
>  {
> -	return NFS4_MAX_SESSIONID_LEN + 20;
> +	return (op_encode_hdr_size
> +		+ XDR_QUADLEN(NFS4_MAX_SESSIONID_LEN) + 5) * sizeof(__be32);
>  }
>  
>  static inline u32 nfsd4_setattr_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
> @@ -1893,6 +1894,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
>  		.op_func = (nfsd4op_func)nfsd4_sequence,
>  		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
>  		.op_name = "OP_SEQUENCE",
> +		.op_rsize_bop = (nfsd4op_rsize)nfsd4_sequence_rsize,
>  	},
>  	[OP_DESTROY_CLIENTID] = {
>  		.op_func = (nfsd4op_func)nfsd4_destroy_clientid,
> --
> 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


With the earlier version of this patch, I was seeing a bunch of these
messages:

[56121.351277] RPC request reserved 124 but used 136
[56121.532839] RPC request reserved 204 but used 208
[56121.574473] RPC request reserved 116 but used 128
[56121.634628] RPC request reserved 204 but used 208
[56121.663675] RPC request reserved 116 but used 128

...but this version seems to have silenced those warnings. You can add:

Tested-by: Jeff Layton <jlayton@primarydata.com>