diff mbox

nfsd4: fix response size estimation for OP_SEQUENCE

Message ID 20141021131406.GE9863@fieldses.org (mailing list archive)
State New, archived
Headers show

Commit Message

J. Bruce Fields Oct. 21, 2014, 1:14 p.m. UTC 
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>

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

Comments

J. Bruce Fields Oct. 22, 2014, 7:22 p.m. UTC | #1 
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.
--
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
Schumaker, Anna Oct. 22, 2014, 7:33 p.m. UTC | #2 
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.
> 

--
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
J. Bruce Fields Oct. 22, 2014, 7:42 p.m. UTC | #3 
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.
--
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
Thomas Haynes Oct. 22, 2014, 8:12 p.m. UTC | #4 
> 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
--
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
Christoph Hellwig Oct. 23, 2014, 7:34 a.m. UTC | #5 
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.

--
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
Jeff Layton Oct. 23, 2014, 11:54 a.m. UTC | #6 
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>
--
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
diff mbox

Patch

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,