[PATCH] nfsd4: fix response size estimation for OP_SEQUENCE

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

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-***@public.gmane.org
Reported-by: Chuck Lever <chucklever-***@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
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,

[Christoph Hellwig]

Post by J. Bruce Fields
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

Post by Christoph Hellwig

Post by J. Bruce Fields
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-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
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-***@public.gmane.org
Reported-by: Chuck Lever <chucklever-***@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

Post by 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Anna Schumaker]

Post by J. Bruce Fields

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

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

Post by J. Bruce Fields
--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

Post by J. Bruce Fields

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

- 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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tom Haynes]

Post by J. Bruce Fields

Post by J. Bruce Fields

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

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

Post by J. Bruce Fields
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
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Christoph Hellwig]

Post by Tom Haynes

Post by J. Bruce Fields
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+***@public.gmane.org>

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-***@public.gmane.org
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
fs/nfsd/nfs4proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

On Wed, Oct 22, 2014 at 03:2

Post by J. Bruce Fields
- 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

[Jeff Layton]

On Tue, 21 Oct 2014 09:14:06 -0400

Post by J. Bruce Fields

Post by Christoph Hellwig

Post by J. Bruce Fields
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
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"
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
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-7I+n7zu2hftEKMMhf/***@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html