nfsd4: fix response size estimation for OP_SEQUENCE

PATCH In review J. Bruce Fields 5 participants 10 messages Oct 17, 2014 – Oct 23, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2014-10-17 21:24 UTC #0 lore raw

PATCH 1/1 nfsd4: fix response size estimation for OP_SEQUENCE

From: "J. Bruce Fields" <[email protected]>

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 [email protected]

Reported-by Chuck Lever <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

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

fs/nfsd/nfs4proc.c +3 -1

index cdeb3cf..f990983 100644
@@ -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,

Signature · 1 line

1.9.1

Christoph Hellwig

2014-10-21 10:36 UTC #1 lore raw

Quoted 6 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>
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?

J. Bruce Fields

author

2014-10-21 13:14 UTC #2 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
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 [email protected]

Reported-by Chuck Lever <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs4proc.c +3 -1

index cdeb3cf..f4bd578 100644
@@ -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,

J. Bruce Fields

author

2014-10-22 19:23 UTC #3 lore raw

Quoted 2 lines · J. Bruce Fields

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.

Anna Schumaker

2014-10-22 19:33 UTC #4 lore raw

Quoted 9 lines · J. Bruce Fields

Quoted 2 lines · J. Bruce Fields
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.

Quoted 7 lines

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!

Quoted 3 lines

--b.

J. Bruce Fields

author

2014-10-22 19:43 UTC #5 lore raw

In reply to J. Bruce Fields

Quoted 9 lines · J. Bruce Fields

Quoted 2 lines · J. Bruce Fields
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.

Quoted 7 lines

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.

Tom Haynes

2014-10-22 20:12 UTC #6 lore raw

Quoted 23 lines · J. Bruce Fields

Quoted 11 lines · J. Bruce Fields
Quoted 3 lines · J. Bruce Fields
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.
Quoted 7 lines
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?

Quoted 9 lines

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

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Christoph Hellwig

2014-10-23 07:35 UTC #7 lore raw

Quoted 6 lines · Tom Haynes

Quoted 4 lines
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.

J. Bruce Fields

author

2014-10-22 19:49 UTC #8 lore raw

PATCH 1/1 nfsd4: fix crash on unknown operation number

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 [email protected]

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Wed, Oct 22, 2014 at 03:2

Quoted 6 lines

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.

fs/nfsd/nfs4proc.c +2 -1

index f4bd578bed55..0beb023f25ac 100644
@@ -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

Signature · 1 line

1.9.3

Jeff Layton

Tested-by

2014-10-23 11:54 UTC #9 lore raw

In reply to J. Bruce Fields

Quoted 59 lines · On Tue, 21 Oct 2014 09:14:06 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 9 lines · Christoph Hellwig
Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
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 [email protected]
Reported-by Chuck Lever <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4proc.c +3 -2
index cdeb3cf..f4bd578 100644
@@ -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,
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
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 <[email protected]>