backchannel overflows

RFC PATCH In review Christoph Hellwig 5 participants 19 messages Apr 28, 2015 – May 1, 2015 1 message not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Christoph Hellwig

author

2015-04-28 20:21 UTC #0 lore raw

RFC PATCH 1/1 backchannel overflows

Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.

I don't really understand how the spec wants to treat CB_NULL in relation to the available backchannel slots, but given that it's not part of the sequences in CB_SEQUENCE it somehow nees to bypass them.

If we make sure to overallocate the rpc-level buffers so that we have more than the available NFS-level slots we should be safe from this condition which makes a 4.1 server doing heavy recalls under load very unhapy by not returning an NFS level reply to its layout recalls.

I dont really like this patch much, so any idea for a better solution would be highly welcome!

fs/nfs/callback.h +1 -1

index 84326e9..7afb3ef 100644
@@ -205,7 +205,7 @@ extern int nfs4_set_callback_sessionid(struct nfs_client *clp);
  * so we limit their concurrency to 1 by setting up the maximum number
  * of slots for the backchannel.
  */
-#define NFS41_BC_MIN_CALLBACKS 1
+#define NFS41_BC_MIN_CALLBACKS 2
 #define NFS41_BC_MAX_CALLBACKS 1
 
 extern unsigned int nfs_callback_set_tcpport;

Kinglong Mee

2015-04-29 12:08 UTC #1 lore raw

Quoted 21 lines · Christoph Hellwig

Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.
I don't really understand how the spec wants to treat CB_NULL in relation to the available backchannel slots, but given that it's not part of the sequences in CB_SEQUENCE it somehow nees to bypass them.
If we make sure to overallocate the rpc-level buffers so that we have more than the available NFS-level slots we should be safe from this condition which makes a 4.1 server doing heavy recalls under load very unhapy by not returning an NFS level reply to its layout recalls.
I dont really like this patch much, so any idea for a better solution would be highly welcome!
fs/nfs/callback.h +0 -0
index 84326e9..7afb3ef 100644

fs/nfs/callback.h extern int nfs4_set_callback_sessionid(struct nfs_client *clp); · line 209

Quoted 6 lines

fs/nfs/callback.h +1 -1

@@ -205,7 +205,7 @@ extern int nfs4_set_callback_sessionid(struct nfs_client *clp);
  * so we limit their concurrency to 1 by setting up the maximum number
  * of slots for the backchannel.
  */
-#define NFS41_BC_MIN_CALLBACKS 1
+#define NFS41_BC_MIN_CALLBACKS 2
 #define NFS41_BC_MAX_CALLBACKS 1

Are you sure that's okay without update NFS41_BC_MAX_CALLBACKS?

thanks,
Kinglong Mee

Christoph Hellwig

author

2015-04-29 13:46 UTC #2 lore raw

Quoted 5 lines · Kinglong Mee

Quoted 5 lines

fs/nfs/callback.h +0 -0

index 84326e9..7afb3ef 100644
@@ -205,7 +205,7 @@ extern int nfs4_set_callback_sessionid(struct nfs_client *clp);

Quoted 8 lines

fs/nfs/callback.h extern int nfs4_set_callback_sessionid(struct nfs_client *clp); · line 209
Quoted 6 lines
fs/nfs/callback.h +1 -1
  * so we limit their concurrency to 1 by setting up the maximum number
  * of slots for the backchannel.
  */
-#define NFS41_BC_MIN_CALLBACKS 1
+#define NFS41_BC_MIN_CALLBACKS 2
 #define NFS41_BC_MAX_CALLBACKS 1
Are you sure that's okay without update NFS41_BC_MAX_CALLBACKS?

Yes, NFS41_BC_MIN_CALLBACKS is the number of slots we tell the sunrpc layer to allocate, while NFS41_BC_MAX_CALLBACKS is used for nfs-internal accounting. So having them different is intentional for this rfc. If we'll have to merge this instead of a better fix we defintively should fix up the naming, though.

Chuck Lever

2015-04-29 14:54 UTC #3 lore raw

In reply to Christoph Hellwig

Quoted 2 lines · Christoph Hellwig

Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.

Under what circumstances does the server send a CB_NULL while a CB_COMPOUND is in flight?

Quoted 27 lines

I don't really understand how the spec wants to treat CB_NULL in relation to the available backchannel slots, but given that it's not part of the sequences in CB_SEQUENCE it somehow nees to bypass them.
If we make sure to overallocate the rpc-level buffers so that we have more than the available NFS-level slots we should be safe from this condition which makes a 4.1 server doing heavy recalls under load very unhapy by not returning an NFS level reply to its layout recalls.
I dont really like this patch much, so any idea for a better solution would be highly welcome!
fs/nfs/callback.h +1 -2
index 84326e9..7afb3ef 100644
@@ -205,7 +205,7 @@ extern int nfs4_set_callback_sessionid(struct nfs_client *clp);
 * so we limit their concurrency to 1 by setting up the maximum number
 * of slots for the backchannel.
 */
-#define NFS41_BC_MIN_CALLBACKS 1
+#define NFS41_BC_MIN_CALLBACKS 2
 #define NFS41_BC_MAX_CALLBACKS 1
 
 extern unsigned int nfs_callback_set_tcpport;
--

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

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Trond Myklebust

2015-04-29 14:58 UTC #4 lore raw

Quoted 9 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig
Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.
Under what circumstances does the server send a CB_NULL while a CB_COMPOUND is in flight?

I agree with Chuck. Why does knfsd send a CB_NULL at all if the intention is to send CB_COMPOUND?

Cheers
Trond

Christoph Hellwig

author

2015-04-29 15:14 UTC #5 lore raw

In reply to Chuck Lever

Quoted 8 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig
Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.
Under what circumstances does the server send a CB_NULL while a CB_COMPOUND is in flight?

When a client is under heavy loads from fsx or aio-stress, and we lose the connection (nfsd4_conn_lost is called) while doing heavy recalls.

xfstests against a server offering pnfs layouts for which the client can't reach the storage devices is an easy reproducer.

Trond Myklebust

2015-04-29 15:24 UTC #6 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Chuck Lever
Quoted 2 lines · Christoph Hellwig
Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.

Quoted 8 lines

Quoted 2 lines
Under what circumstances does the server send a CB_NULL while a CB_COMPOUND is in flight?
When a client is under heavy loads from fsx or aio-stress, and we lose the connection (nfsd4_conn_lost is called) while doing heavy recalls.
xfstests against a server offering pnfs layouts for which the client can't reach the storage devices is an easy reproducer.

Why does it need to do this? If the client has sent the BIND_CONN_TO_SESSION (which I believe that knfsd asks for), then the server knows that this is a bi-directional connection.
The difference between NFSv4 and NFSv4.1 is that the CB_NULL should almost always be redundant, because the client initiates the connection and it explicitly tells the server whether or not it is to be used for the callback channel.

The CB_NULL should always be redundant.

Trond

J. Bruce Fields

2015-04-29 17:34 UTC #7 lore raw

Quoted 25 lines · Trond Myklebust

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Chuck Lever
Quoted 2 lines · Christoph Hellwig
Currently the client will just crap out if a CB_NULL comes in at the same time as a slot controlled CB_COMPOUND that includes a CB_SEQUENCE.
Quoted 8 lines
Quoted 2 lines
Under what circumstances does the server send a CB_NULL while a CB_COMPOUND is in flight?
When a client is under heavy loads from fsx or aio-stress, and we lose the connection (nfsd4_conn_lost is called) while doing heavy recalls.
xfstests against a server offering pnfs layouts for which the client can't reach the storage devices is an easy reproducer.
Why does it need to do this? If the client has sent the BIND_CONN_TO_SESSION (which I believe that knfsd asks for), then the server knows that this is a bi-directional connection.
The difference between NFSv4 and NFSv4.1 is that the CB_NULL should almost always be redundant, because the client initiates the connection and it explicitly tells the server whether or not it is to be used for the callback channel.

Quoted 1 line

The CB_NULL should always be redundant.

I'd be fine with suppressing it. I think I actually intended to but screwed it up. (Chuck or somebody convinced me the NFSD4_CB_UP/UNKNOWN/DOWN logic is totally broken but I never got around to fixing it.)

--b.

Christoph Hellwig

author

2015-04-30 06:26 UTC #8 lore raw

Quoted 8 lines · J. Bruce Fields

Quoted 8 lines
Why does it need to do this? If the client has sent the BIND_CONN_TO_SESSION (which I believe that knfsd asks for), then the server knows that this is a bi-directional connection.
The difference between NFSv4 and NFSv4.1 is that the CB_NULL should almost always be redundant, because the client initiates the connection and it explicitly tells the server whether or not it is to be used for the callback channel.

Quoted 6 lines

Quoted 1 line
The CB_NULL should always be redundant.
I'd be fine with suppressing it. I think I actually intended to but screwed it up. (Chuck or somebody convinced me the NFSD4_CB_UP/UNKNOWN/DOWN logic is totally broken but I never got around to fixing it.)

I've dived into removing CB_NULL, and fixed various major breakage in the nfsd callback path. for which I will send you an RFC series ASAP.

However, even with that I see the "Callback slot table overflowed" from the client under load. I think the problem is the following:

Between sending the callback response in call_bc_transmit -> xprt_transmit and actually releasing the request from rpc_exit_task -> xprt_release -> xprt_free_bc_request there is race window, and between and overloaded client and a fast connection we can hit this one easily.

My patch to increase the number of buffers for the backchannel ensures this doesn't happen in my setup, but of course I could envinsion a theoretical setu where the client is so slow that multiple already processed requests might not be returned yet.

Chuck Lever

2015-04-30 14:33 UTC #9 lore raw

Quoted 27 lines · Christoph Hellwig

Quoted 8 lines · J. Bruce Fields
Quoted 8 lines
Why does it need to do this? If the client has sent the BIND_CONN_TO_SESSION (which I believe that knfsd asks for), then the server knows that this is a bi-directional connection.
The difference between NFSv4 and NFSv4.1 is that the CB_NULL should almost always be redundant, because the client initiates the connection and it explicitly tells the server whether or not it is to be used for the callback channel.
Quoted 6 lines
Quoted 1 line
The CB_NULL should always be redundant.
I'd be fine with suppressing it. I think I actually intended to but screwed it up. (Chuck or somebody convinced me the NFSD4_CB_UP/UNKNOWN/DOWN logic is totally broken but I never got around to fixing it.)
I've dived into removing CB_NULL, and fixed various major breakage in the nfsd callback path. for which I will send you an RFC series ASAP.
However, even with that I see the "Callback slot table overflowed" from the client under load. I think the problem is the following:
Between sending the callback response in call_bc_transmit -> xprt_transmit and actually releasing the request from rpc_exit_task -> xprt_release -> xprt_free_bc_request there is race window, and between and overloaded client and a fast connection we can hit this one easily.

Quoted 4 lines

My patch to increase the number of buffers for the backchannel ensures this doesn't happen in my setup, but of course I could envinsion a theoretical setu where the client is so slow that multiple already processed requests might not be returned yet.

I’ve been discussing the possibility of adding more session slots on the Linux NFS client with jlayton. We think it would be straightforward, once the workqueue-based NFSD patches are in, to make the backchannel service into a workqueue. Then it would be a simple matter to increase the number of session slots.

We haven’t discussed what would be needed on the server side of this equation, but sounds like it has some deeper problems if it is not obeying the session slot table limits advertised by the client.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Christoph Hellwig

author

2015-04-30 14:37 UTC #10 lore raw

Quoted 9 lines · Chuck Lever

I?ve been discussing the possibility of adding more session slots on the Linux NFS client with jlayton. We think it would be straightforward, once the workqueue-based NFSD patches are in, to make the backchannel service into a workqueue. Then it would be a simple matter to increase the number of session slots.
We haven?t discussed what would be needed on the server side of this equation, but sounds like it has some deeper problems if it is not obeying the session slot table limits advertised by the client.

No, the client isn't obeying it's own slot limits

The problem is when the client responds to a callback it still holds a references on rpc_rqst for a while. If the server sends the next callback fast enough to hit that race window the client incorrectly rejects it. Note that we never even get to the nfs code that check the slot id in this case, it's low-level sunrpc code that is the problem.

Note that with my patches the server can recover from this client problem by resetting the connection, but that's not very optimal behavior.

My current patch which includes the explanation is below.

---
commit 7e995048c9917766e0ae96889ef48733c2229b7e

Author: Christoph Hellwig <[email protected]>
Date:   Thu Apr 30 11:53:11 2015 +0200

nfs: overallocate backchannel requests

The RPC code releases the rpc_rqst much later than sending the reply to the server. To avoid low-level errors that lead connection requests under load allocate twice as many rpc_rqst structures as callback slots to allow for double buffering.

Signed-off-by Christoph Hellwig <[email protected]>

fs/nfs/callback.h +7 -2

index 84326e9..7f79345 100644
@@ -200,13 +200,18 @@ extern int nfs4_validate_delegation_stateid(struct nfs_delegation *delegation,
 					    const nfs4_stateid *stateid);
 extern int nfs4_set_callback_sessionid(struct nfs_client *clp);
 #endif /* CONFIG_NFS_V4 */
+
 /*
  * nfs41: Callbacks are expected to not cause substantial latency,
  * so we limit their concurrency to 1 by setting up the maximum number
  * of slots for the backchannel.
+ *
+ * Note that we allocate a second RPC request structure because the
+ * RPC code holds on to the buffer for a while after sending a reply
+ * to the server.
  */
-#define NFS41_BC_MIN_CALLBACKS 1
-#define NFS41_BC_MAX_CALLBACKS 1
+#define NFS41_BC_SLOTS 1
+#define NFS41_BC_REQUESTS (NFS41_BC_SLOTS * 2)
 
 extern unsigned int nfs_callback_set_tcpport;
 extern unsigned short nfs_callback_tcpport;

fs/nfs/callback_proc.c +3 -3

index 197806f..d733f04 100644
@@ -320,7 +320,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 	dprintk("%s enter. slotid %u seqid %u\n",
 		__func__, args->csa_slotid, args->csa_sequenceid);
 
-	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
+	if (args->csa_slotid >= NFS41_BC_SLOTS)
 		return htonl(NFS4ERR_BADSLOT);
 
 	slot = tbl->slots + args->csa_slotid;

@@ -465,8 +465,8 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	       sizeof(res->csr_sessionid));
 	res->csr_sequenceid = args->csa_sequenceid;
 	res->csr_slotid = args->csa_slotid;
-	res->csr_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
-	res->csr_target_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
+	res->csr_highestslotid = NFS41_BC_SLOTS - 1;
+	res->csr_target_highestslotid = NFS41_BC_SLOTS - 1;
 
 out:
 	cps->clp = clp; /* put in nfs4_callback_compound */

fs/nfs/nfs4client.c +1 -1

index e42be52..e36717c 100644
@@ -248,7 +248,7 @@ static int nfs4_init_callback(struct nfs_client *clp)
 	xprt = rcu_dereference_raw(clp->cl_rpcclient->cl_xprt);
 
 	if (nfs4_has_session(clp)) {
-		error = xprt_setup_backchannel(xprt, NFS41_BC_MIN_CALLBACKS);
+		error = xprt_setup_backchannel(xprt, NFS41_BC_REQUESTS);
 		if (error < 0)
 			return error;
 	}

fs/nfs/nfs4session.c +1 -1

index e23366e..3ad4da4 100644
@@ -500,7 +500,7 @@ void nfs4_destroy_session(struct nfs4_session *session)
 	rcu_read_unlock();
 	dprintk("%s Destroy backchannel for xprt %p\n",
 		__func__, xprt);
-	xprt_destroy_backchannel(xprt, NFS41_BC_MIN_CALLBACKS);
+	xprt_destroy_backchannel(xprt, NFS41_BC_REQUESTS);
 	nfs4_destroy_session_slot_tables(session);
 	kfree(session);
 }

A message that is not in the archive (sent off-list or not yet received): CAHQdGtRgEVXidNNYtYf4c3uS0vc6fbm-SZ5AxrY=awXYynmACw@mail.gmail.com

Trond Myklebust

2015-04-30 15:02 UTC #12 lore raw

Apologies for the resend...

Quoted 32 lines · Trond Myklebust

Quoted 12 lines · Christoph Hellwig
Quoted 9 lines · Chuck Lever
I?ve been discussing the possibility of adding more session slots on the Linux NFS client with jlayton. We think it would be straightforward, once the workqueue-based NFSD patches are in, to make the backchannel service into a workqueue. Then it would be a simple matter to increase the number of session slots.
We haven?t discussed what would be needed on the server side of this equation, but sounds like it has some deeper problems if it is not obeying the session slot table limits advertised by the client.
Quoted 8 lines
No, the client isn't obeying it's own slot limits
The problem is when the client responds to a callback it still holds a references on rpc_rqst for a while. If the server sends the next callback fast enough to hit that race window the client incorrectly rejects it. Note that we never even get to the nfs code that check the slot id in this case, it's low-level sunrpc code that is the problem.
We can add dynamic allocation of a new slot as part of the backchannel reply transmit workload. That way we close the race without opening for violation of session limits.
Cheers
Trond

Chuck Lever

2015-04-30 15:11 UTC #13 lore raw

In reply to a message not in the archive

Quoted 16 lines · Trond Myklebust

Quoted 16 lines · Christoph Hellwig
Quoted 9 lines · Chuck Lever
I?ve been discussing the possibility of adding more session slots on the Linux NFS client with jlayton. We think it would be straightforward, once the workqueue-based NFSD patches are in, to make the backchannel service into a workqueue. Then it would be a simple matter to increase the number of session slots.
We haven?t discussed what would be needed on the server side of this equation, but sounds like it has some deeper problems if it is not obeying the session slot table limits advertised by the client.
No, the client isn't obeying it's own slot limits

Quoted 8 lines

Quoted 6 lines
The problem is when the client responds to a callback it still holds a references on rpc_rqst for a while. If the server sends the next callback fast enough to hit that race window the client incorrectly rejects it. Note that we never even get to the nfs code that check the slot id in this case, it's low-level sunrpc code that is the problem.
We can add dynamic allocation of a new slot as part of the backchannel reply transmit workload. That way we close the race without opening for violation of session limits.

I’ll have to think about how that would affect RPC/RDMA backchannel. Transport resources are allocated when the transport is created, and can’t be dynamically added. (It certainly wouldn’t be a problem to overprovision, as Christoph has done here).

I was thinking maybe using a local copy of the rpc_rqst for sending the backchannel reply, and freeing the rpc_rqst before sending, might close the window.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Chuck Lever

2015-04-30 17:40 UTC #14 lore raw

Quoted 26 lines · Chuck Lever

Quoted 23 lines · Trond Myklebust
Quoted 22 lines · Christoph Hellwig
Quoted 9 lines · Chuck Lever
I?ve been discussing the possibility of adding more session slots on the Linux NFS client with jlayton. We think it would be straightforward, once the workqueue-based NFSD patches are in, to make the backchannel service into a workqueue. Then it would be a simple matter to increase the number of session slots.
We haven?t discussed what would be needed on the server side of this equation, but sounds like it has some deeper problems if it is not obeying the session slot table limits advertised by the client.
No, the client isn't obeying it's own slot limits
The problem is when the client responds to a callback it still holds a references on rpc_rqst for a while. If the server sends the next callback fast enough to hit that race window the client incorrectly rejects it. Note that we never even get to the nfs code that check the slot id in this case, it's low-level sunrpc code that is the problem.

Quoted 6 lines

Quoted 1 line
We can add dynamic allocation of a new slot as part of the backchannel reply transmit workload. That way we close the race without opening for violation of session limits.
I’ll have to think about how that would affect RPC/RDMA backchannel. Transport resources are allocated when the transport is created, and can’t be dynamically added. (It certainly wouldn’t be a problem to overprovision, as Christoph has done here).

We discussed this briefly during the Linux NFS town hall meeting. I agree using dynamic slot allocation for TCP is fine, and RPC/RDMA can use simple overprovisioning.

This way the upper layer (NFSv4.1 client) doesn’t have to be aware of limitations in the RPC layer mechanism.

Trond may have an additional concern that I didn’t capture.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Christoph Hellwig

author

2015-05-01 17:23 UTC #15 lore raw

Quoted 8 lines · Chuck Lever

We discussed this briefly during the Linux NFS town hall meeting. I agree using dynamic slot allocation for TCP is fine, and RPC/RDMA can use simple overprovisioning.
This way the upper layer (NFSv4.1 client) doesn?t have to be aware of limitations in the RPC layer mechanism.
Trond may have an additional concern that I didn?t capture.

The other option would be to simply overallocate in the transport layer, as that is the layer which causes the problem to start with.

That being said, what is the argument for doing any sort of static allocation here? I'm fine with doing fully dynamic allocation if that works out fine, but a mixed static / dynamic allocation sounds like a nightmare.

Trond Myklebust

2015-05-01 17:28 UTC #16 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Chuck Lever
We discussed this briefly during the Linux NFS town hall meeting. I agree using dynamic slot allocation for TCP is fine, and RPC/RDMA can use simple overprovisioning.
This way the upper layer (NFSv4.1 client) doesn?t have to be aware of limitations in the RPC layer mechanism.
Trond may have an additional concern that I didn?t capture.

Quoted 7 lines

The other option would be to simply overallocate in the transport layer, as that is the layer which causes the problem to start with.
That being said, what is the argument for doing any sort of static allocation here? I'm fine with doing fully dynamic allocation if that works out fine, but a mixed static / dynamic allocation sounds like a nightmare.

The concern is not so much static vs dynamic. The concern is limiting incoming RPC calls to the number allowed by the NFSv4.1 session. Right now, the static allocation enforces the limit of 1 slot that the client offers to the server (albeit with the race) and so I want any replacement to meet the same requirement.

Cheers
Trond

Christoph Hellwig

author

2015-05-01 17:38 UTC #17 lore raw

Quoted 5 lines · Trond Myklebust

The concern is not so much static vs dynamic. The concern is limiting incoming RPC calls to the number allowed by the NFSv4.1 session. Right now, the static allocation enforces the limit of 1 slot that the client offers to the server (albeit with the race) and so I want any replacement to meet the same requirement.

Either variant will allow accepting more than one backchannel request at the RPC layer, that's the whole point. With the simple patch I posted it will accept a 2nd one, with dynamic allocation the number would be potentially unbound.

But given that the NFS client already enforces the slot limit properly in validate_seqid, and even returns the proper nfs error for this case I don't really see what enforcing it at a lower level we we can't even report proper errors buys us.

Trond Myklebust

2015-05-01 17:47 UTC #18 lore raw

Quoted 11 lines · Christoph Hellwig

Quoted 5 lines · Trond Myklebust
The concern is not so much static vs dynamic. The concern is limiting incoming RPC calls to the number allowed by the NFSv4.1 session. Right now, the static allocation enforces the limit of 1 slot that the client offers to the server (albeit with the race) and so I want any replacement to meet the same requirement.
Either variant will allow accepting more than one backchannel request at the RPC layer, that's the whole point. With the simple patch I posted it will accept a 2nd one, with dynamic allocation the number would be potentially unbound.

If you do it wrong, yes. The point is if you do the allocation as part of the send process, then you prevent unbounded growth.

Quoted 4 lines

But given that the NFS client already enforces the slot limit properly in validate_seqid, and even returns the proper nfs error for this case I don't really see what enforcing it at a lower level we we can't even report proper errors buys us.

It buys us better congestion control. We don't even need to process the request.

Cheers
Trond

Chuck Lever

2015-05-01 17:30 UTC #19 lore raw

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Chuck Lever
We discussed this briefly during the Linux NFS town hall meeting. I agree using dynamic slot allocation for TCP is fine, and RPC/RDMA can use simple overprovisioning.

Quoted 7 lines

Quoted 4 lines
This way the upper layer (NFSv4.1 client) doesn?t have to be aware of limitations in the RPC layer mechanism.
Trond may have an additional concern that I didn?t capture.
The other option would be to simply overallocate in the transport layer, as that is the layer which causes the problem to start with.

That’s exactly what the RDMA backchannel will do.

Quoted 4 lines

That being said, what is the argument for doing any sort of static allocation here? I'm fine with doing fully dynamic allocation if that works out fine, but a mixed static / dynamic allocation sounds like a nightmare.

RDMA resources must be allocated and pre-registered up front. The RDMA transport can’t support dynamic slot allocation without adding a lot more complexity.

The RDMA transport will need to have separate backchannel setup and destroy methods anyway. So doing dynamic for TCP and overprovision for RDMA will be simple.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com