On Jul 18, 2014, at 3:12 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > On Fri, Jul 18, 2014 at 02:47:41PM -0400, Chuck Lever wrote: >> >> On Jul 17, 2014, at 2:59 PM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: >> >>> >>> On Jul 17, 2014, at 2:36 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: >>> >>>> On Wed, Jul 16, 2014 at 03:38:32PM -0400, Chuck Lever wrote: >>>>> The current code always selects XPRT_TRANSPORT_BC_TCP for the back >>>>> channel, even when the forward channel was not TCP (eg, RDMA). When >>>>> a 4.1 mount is attempted with RDMA, the server panics in the TCP BC >>>>> code when trying to send CB_NULL. >>>>> >>>>> Instead, construct the transport protocol number from the forward >>>>> channel transport or'd with XPRT_TRANSPORT_BC. Transports that do >>>>> not support bi-directional RPC will not have registered a "BC" >>>>> transport, causing create_backchannel_client() to fail immediately. >>>>> >>>>> Fixes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=265 >>>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >>>>> --- >>>>> Hi Bruce- >>>>> >>>>> What do you think of this approach? >>>> >>>> OK by me. (So clients use a separate tcp connection for the >>>> backchannel?) >>> >>> Right. >>> >>> The current plan is that, for NFSv4.1 over RDMA, the Linux client will >>> create an additional TCP connection and bind it to the same session as >>> the RDMA transport. >>> >>> The TCP connection will be used solely for callback operations. The >>> forward channel on that connection will not be used, except for >>> BIND_CONN_TO_SESSION operations. >> >> Hi Bruce, pursuant to this goal, I’m trying to understand commit >> 14a24e99, especially the interior comment: >> >> 3114 /* Should we give out recallable state?: */ >> 3115 static bool nfsd4_cb_channel_good(struct nfs4_client *clp) >> 3116 { >> 3117 if (clp->cl_cb_state == NFSD4_CB_UP) >> 3118 return true; >> 3119 /* >> 3120 * In the sessions case, since we don't have to establish a >> 3121 * separate connection for callbacks, we assume it's OK >> 3122 * until we hear otherwise: >> 3123 */ >> 3124 return clp->cl_minorversion && clp->cl_cb_state == NFSD4_CB_UNKNOWN; >> 3125 } >> >> I wonder if that’s a valid assumption? >> >> A separate virtual connection _does_ have to be established. If the >> client sent a CREATE_SESSION operation with the SESSION4_BACK_CHAN flag >> set, the server must verify that the client has an active bi-directional >> RPC service on the transport. > > If the client sets the BACK_CHAN flag, that means it wants that > connection to be used as a back channel. > > So in the RDMA case it sounds like the client needs to clear that flag > on the create session. Right. > Either that or send the original CREATE_SESSION over the connection you > want used for the backchannel, then send a BIND_CONNECTION_TO_SESSION > over the RDMA connection. > > That said, if the server knows there's no connection at all that's > available for the backchannel, then yes I agree that it should be > setting the PATH_DOWN flag: I’m trying a fix that sets cl_cb_state = NFSD4_CB_DOWN in init_session() if SESSION4_BACK_CHAN is clear. >From the looks of it, the server is architected to support one and only one session per client ID. Is that correct? Thus it expects just one CREATE_SESSION operation per EXCHANGE_ID. Does the server enforce this? Seems like cl_cb_state really should be a per-session thing for NFSv4.1. And, while we’re at it, the server should assert SEQ4_STATUS_CB_PATH_DOWN_SESSION as well as SEQ4_STATUS_CB_PATH_DOWN. > >> Otherwise it can hand out recallable state >> to a client with no callback service. Is that always safe? >> >> Or, if the client sent a CREATE_SESSION operation with the >> SESSION4_BACK_CHAN flag cleared, then either the client intends not to >> set up a backchannel, or it intends to establish a separate transport >> for the backchannel. There is no backchannel in that case until the >> client establishes it. >> >> Right now, if the client does not set SESSION4_BACK_CHAN, the server >> never asserts SEQ4_STATUS_CB_PATH_DOWN, even though there is no >> backchannel. That seems like a (minor, perhaps) bug. > > Yes, agreed, I'll take a look--how are you reproducing? Prototype Linux client that doesn’t set SESSION4_BACK_CHAN when it does CREATE_SESSION. Preparing it for RDMA support. You could code a pyNFS test. > The > nfsd4_probe_callback() should result in NFSD4_CB_DOWN getting set as > soon as the workqueue process it. Clearing SESSION4_BACK_CHAN squelches nfsd4_probe_callback(), so the server never checks in this case. > In theory there's a race there (I > should fix that) in practice I'd normally expect that to run before we > process a SEQUENCE request on the new session. >> Do you remember what this commit was trying to fix? > > It's not a bugfix, just a minor optimization. We could revert it if > need be. NP, I just don’t see how it helps to make that assumption, plus it doesn’t seem entirely safe. My initial question was "why does alloc_client() initialize cl_cb_state to NFSD4_CB_UNKNOWN instead of NFSD4_CB_DOWN?” -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
