NFSD: Revert setting op_encode_lockowner_maxsz

Commit 8c7424cff6 (nfsd4: don't try to encode conflicting owner if low on space) set op_encode_lockowner_maxsz to zero.

If setting op_encode_lockowner_maxsz to zero, nfsd will not encode the owner of conflock forever.

Signed-off-by Kinglong Mee <[email protected]>

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

fs/nfsd/nfs4proc.c +1 -2

index 084e46e..55635a8 100644
@@ -1417,8 +1417,7 @@ out:
 #define op_encode_change_info_maxsz	(5)
 #define nfs4_fattr_bitmap_maxsz		(4)
 
-/* We'll fall back on returning no lockowner if run out of space: */
-#define op_encode_lockowner_maxsz	(0)
+#define op_encode_lockowner_maxsz	(1 + XDR_QUADLEN(IDMAP_NAMESZ))
 #define op_encode_lock_denied_maxsz	(8 + op_encode_lockowner_maxsz)
 
 #define nfs4_owner_maxsz		(1 + XDR_QUADLEN(IDMAP_NAMESZ))

Signature · 1 line

1.9.3

Quoted 5 lines · Kinglong Mee

Commit 8c7424cff6 (nfsd4: don't try to encode conflicting owner if low on space) set op_encode_lockowner_maxsz to zero.

If setting op_encode_lockowner_maxsz to zero, nfsd will not encode the owner of conflock forever.

Right, so the problem is that the lock reply encoder is unique in that it will happily adjust the xdr encoding if it's running out of space.

This came about with 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space". The problem is that:

- the maximum size of a lock reply is kind of big (the original calculation below is actually wrong, IDMAP_NAMESZ should be NFS4_OPAQUE_LIMIT).
- we may not be the only server that's been sloppy about enforcing the theoretical maximum here, and I'd rather be forgiving to clients that don't insist on the theoretical maximum maxresp_cached.

So best seems just to allow a LOCK even if space is insufficient and just throw out the conflicting lockowner if there isn't enough space, since large lockowners should be rare and we don't care about the conflicting lockowner anyway.

So anyway we need to leave the maximum reserved in rq_reserved without changing the check we make before executing the LOCK.

Quoted 21 lines · J. Bruce Fields

Quoted 5 lines · Kinglong Mee

Commit 8c7424cff6 (nfsd4: don't try to encode conflicting owner if low on space) set op_encode_lockowner_maxsz to zero.

If setting op_encode_lockowner_maxsz to zero, nfsd will not encode the owner of conflock forever.

Right, so the problem is that the lock reply encoder is unique in that it will happily adjust the xdr encoding if it's running out of space.

This came about with 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space". The problem is that:

- the maximum size of a lock reply is kind of big (the original calculation below is actually wrong, IDMAP_NAMESZ should be NFS4_OPAQUE_LIMIT).
- we may not be the only server that's been sloppy about enforcing the theoretical maximum here, and I'd rather be forgiving to clients that don't insist on the theoretical maximum maxresp_cached.

Quoted 7 lines

So best seems just to allow a LOCK even if space is insufficient and just throw out the conflicting lockowner if there isn't enough space, since large lockowners should be rare and we don't care about the conflicting lockowner anyway.

So anyway we need to leave the maximum reserved in rq_reserved without changing the check we make before executing the LOCK.

I think this is all we need, but I haven't actually tested whether it fixes the warnings.

--b.

commit 5e78bb7e34d6

Author: J. Bruce Fields <[email protected]>
Date:   Tue Aug 12 11:41:40 2014 -0400

nfsd4: reserve adequate space for LOCK op

As of 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space", we permit the server to process a LOCK operation even if there might not be space to return the conflicting lockowner, because we've made returning the conflicting lockowner optional.

However, the rpc server still wants to know the most we might possibly return, so we need to take into account the possible conflicting lockowner in the svc_reserve_space() call here.

Symptoms were log messages like "RPC request reserved 88 but used 108".

Fixes 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space"

Reported-by Kinglong Mee <[email protected]>

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

fs/nfsd/nfs4xdr.c +1 -0

index 8112ce8f4b23..e771a1a7c6f1 100644
@@ -1663,6 +1663,14 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			readbytes += nfsd4_max_reply(argp->rqstp, op);
 		} else
 			max_reply += nfsd4_max_reply(argp->rqstp, op);
+		/*

From efa33c8c4e9dc99f7addeec935d7172437d6ba10 Mon Sep 17 00:00:00 2001

Quoted 73 lines · J. Bruce Fields

Quoted 21 lines · J. Bruce Fields

Quoted 5 lines · Kinglong Mee

Commit 8c7424cff6 (nfsd4: don't try to encode conflicting owner if low on space) set op_encode_lockowner_maxsz to zero.

If setting op_encode_lockowner_maxsz to zero, nfsd will not encode the owner of conflock forever.

Right, so the problem is that the lock reply encoder is unique in that it will happily adjust the xdr encoding if it's running out of space.

This came about with 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space". The problem is that:

- the maximum size of a lock reply is kind of big (the original calculation below is actually wrong, IDMAP_NAMESZ should be NFS4_OPAQUE_LIMIT).
- we may not be the only server that's been sloppy about enforcing the theoretical maximum here, and I'd rather be forgiving to clients that don't insist on the theoretical maximum maxresp_cached.

Quoted 7 lines

So best seems just to allow a LOCK even if space is insufficient and just throw out the conflicting lockowner if there isn't enough space, since large lockowners should be rare and we don't care about the conflicting lockowner anyway.

So anyway we need to leave the maximum reserved in rq_reserved without changing the check we make before executing the LOCK.

I think this is all we need, but I haven't actually tested whether it fixes the warnings.

--b.

commit 5e78bb7e34d6

Author: J. Bruce Fields <[email protected]>
Date:   Tue Aug 12 11:41:40 2014 -0400

nfsd4: reserve adequate space for LOCK op

As of 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space", we permit the server to process a LOCK operation even if there might not be space to return the conflicting lockowner, because we've made returning the conflicting lockowner optional.

However, the rpc server still wants to know the most we might possibly return, so we need to take into account the possible conflicting lockowner in the svc_reserve_space() call here.

Symptoms were log messages like "RPC request reserved 88 but used 108".

Fixes 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space"

Reported-by Kinglong Mee <[email protected]>

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

fs/nfsd/nfs4xdr.c +8 -0

index 8112ce8f4b23..e771a1a7c6f1 100644
@@ -1663,6 +1663,14 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			readbytes += nfsd4_max_reply(argp->rqstp, op);
 		} else
 			max_reply += nfsd4_max_reply(argp->rqstp, op);
+		/*
+		 * OP_LOCK may return a conflicting lock.  (Special case
+		 * because it will just skip encoding this if it runs
+		 * out of xdr buffer space, and it is the only operation
+		 * that behaves this way.)
+		 */
+		if (op->opnum == OP_LOCK)
+			max_reply += NFS4_OPAQUE_LIMIT;
 

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compoundargs *argp) · line 1676

Quoted 3 lines

fs/nfsd/nfs4xdr.c +0 -0

 		if (op->status) {
 			argp->opcnt = i+1;

Yes, this patch can fixes the warnings.
But, I don't think it's the best fix for the problem.

Why not save reply size of NFS4_OPAQUE_LIMIT in op_encode_lockowner_maxsz,

nfsd4_lockt also needs it.

The nfsd4_lock reply contains only stateid when success, only conflock when denied, so the max size should be max(stateid size, denied lock size).

--------------------------snip---------------------------------------------

Quoted 1 line

From efa33c8c4e9dc99f7addeec935d7172437d6ba10 Mon Sep 17 00:00:00 2001

From: Kinglong Mee <[email protected]>
Date: Sat, 16 Aug 2014 05:33:32 +0800
Subject: [PATCH] NFSD: Correct max reply size for LOCK/LOCKT

As of 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space", we permit the server to process a LOCK operation even if there might not be space to return the conflicting lockowner, because we've made returning the conflicting lockowner optional.

However, the rpc server still wants to know the most we might possibly return, so we need to take into account the possible conflicting lockowner in the svc_reserve() call here.

Symptoms were log messages like "RPC request reserved 88 but used 108".

Also, max_reply will be PAGE_SIZE for nfsd4_lockt's .op_rsize_bop is NULL. New .op_rsize_bop named nfsd4_lockt_rsize for nfsd4_lockt.

Fixes 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space"

Signed-off-by Kinglong Mee <[email protected]>

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

---
 fs/nfsd/nfs4proc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

fs/nfsd/nfs4proc.c +9 -3

index 5e0dc52..64557c9 100644
@@ -1414,8 +1414,7 @@ out:
 #define op_encode_change_info_maxsz	(5)
 #define nfs4_fattr_bitmap_maxsz		(4)
 
-/* We'll fall back on returning no lockowner if run out of space: */
-#define op_encode_lockowner_maxsz	(0)
+#define op_encode_lockowner_maxsz	(XDR_QUADLEN(NFS4_OPAQUE_LIMIT))
 #define op_encode_lock_denied_maxsz	(8 + op_encode_lockowner_maxsz)
 
 #define nfs4_owner_maxsz		(1 + XDR_QUADLEN(IDMAP_NAMESZ))

@@ -1498,10 +1497,16 @@ static inline u32 nfsd4_link_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 
 static inline u32 nfsd4_lock_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 {
-	return (op_encode_hdr_size + op_encode_lock_denied_maxsz)
+	return (op_encode_hdr_size
+		+ max(op_encode_lock_denied_maxsz, op_encode_stateid_maxsz))
 		* sizeof(__be32);
 }
 
+static inline u32 nfsd4_lockt_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
+{
+	return (op_encode_hdr_size + op_encode_lock_denied_maxsz) * sizeof(__be32);
+}
+
 static inline u32 nfsd4_open_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 {
 	return (op_encode_hdr_size + op_encode_stateid_maxsz

@@ -1654,6 +1659,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
 	[OP_LOCKT] = {
 		.op_func = (nfsd4op_func)nfsd4_lockt,
 		.op_name = "OP_LOCKT",
+		.op_rsize_bop = (nfsd4op_rsize)nfsd4_lockt_rsize,
 	},
 	[OP_LOCKU] = {
 		.op_func = (nfsd4op_func)nfsd4_locku,

Signature · 1 line

1.9.3

Quoted 81 lines · Kinglong Mee

Quoted 66 lines · J. Bruce Fields

Quoted 21 lines · J. Bruce Fields

Quoted 5 lines · Kinglong Mee

Commit 8c7424cff6 (nfsd4: don't try to encode conflicting owner if low on space) set op_encode_lockowner_maxsz to zero.

If setting op_encode_lockowner_maxsz to zero, nfsd will not encode the owner of conflock forever.

Right, so the problem is that the lock reply encoder is unique in that it will happily adjust the xdr encoding if it's running out of space.

This came about with 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space". The problem is that:

- the maximum size of a lock reply is kind of big (the original calculation below is actually wrong, IDMAP_NAMESZ should be NFS4_OPAQUE_LIMIT).
- we may not be the only server that's been sloppy about enforcing the theoretical maximum here, and I'd rather be forgiving to clients that don't insist on the theoretical maximum maxresp_cached.

Quoted 7 lines

So best seems just to allow a LOCK even if space is insufficient and just throw out the conflicting lockowner if there isn't enough space, since large lockowners should be rare and we don't care about the conflicting lockowner anyway.

So anyway we need to leave the maximum reserved in rq_reserved without changing the check we make before executing the LOCK.

I think this is all we need, but I haven't actually tested whether it fixes the warnings.

--b.

commit 5e78bb7e34d6

Author: J. Bruce Fields <[email protected]>
Date:   Tue Aug 12 11:41:40 2014 -0400

nfsd4: reserve adequate space for LOCK op

As of 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space", we permit the server to process a LOCK operation even if there might not be space to return the conflicting lockowner, because we've made returning the conflicting lockowner optional.

However, the rpc server still wants to know the most we might possibly return, so we need to take into account the possible conflicting lockowner in the svc_reserve_space() call here.

Symptoms were log messages like "RPC request reserved 88 but used 108".

Fixes 8c7424cff6 "nfsd4: don't try to encode conflicting owner if low on space"

Reported-by Kinglong Mee <[email protected]>

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

fs/nfsd/nfs4xdr.c +1 -0

index 8112ce8f4b23..e771a1a7c6f1 100644
@@ -1663,6 +1663,14 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			readbytes += nfsd4_max_reply(argp->rqstp, op);
 		} else
 			max_reply += nfsd4_max_reply(argp->rqstp, op);
+		/*

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compoundargs *argp) · line 1676

Quoted 10 lines

fs/nfsd/nfs4xdr.c +7 -0

+		 * OP_LOCK may return a conflicting lock.  (Special case
+		 * because it will just skip encoding this if it runs
+		 * out of xdr buffer space, and it is the only operation
+		 * that behaves this way.)
+		 */
+		if (op->opnum == OP_LOCK)
+			max_reply += NFS4_OPAQUE_LIMIT;
 
 		if (op->status) {
 			argp->opcnt = i+1;

Yes, this patch can fixes the warnings.
But, I don't think it's the best fix for the problem.

Quoted 1 line

Why not save reply size of NFS4_OPAQUE_LIMIT in op_encode_lockowner_maxsz,

A lock request is nonidempotent, so clients usually want to cache it. This would force maxresp_cached to be larger than otherwise necessary.

Which I guess the spec already requires. But I'd rather be forgiving if a client overlooks this and requests too small a maxresp_cached.

Quoted 2 lines

nfsd4_lockt also needs it.

nfsd4_lockt is nonidempotent (and probably not terribly common) so we don't care about it that much--there's not even a lockt_rsize defined for it.