Re: [PATCH] svcrdma: Select NFSv4.1 backchannel transport based on forward channel

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Jul 18, 2014 at 03:27:10PM -0400, Chuck Lever wrote:
> On Jul 18, 2014, at 3:12 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 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?

No, that would definitely be a bug, if you see it failing to support
multiple sessions per client id, let me know.

BUT we definitely only use one backchannel connection at a time, even if
the client offers us multiple connections over multiple sessions.

> Seems like cl_cb_state really should be a per-session thing for
> NFSv4.1.

All we care about is whether some session has a working backchannel.

The only callback we currently use is CB_RECALL, and it doesn't matter
which session that's sent over.  If we needed to support e.g.
CB_RECALL_SLOT, I think this should change.

> And, while we’re at it, the server should assert
> SEQ4_STATUS_CB_PATH_DOWN_SESSION as well as SEQ4_STATUS_CB_PATH_DOWN.

Looking back at the spec, especially the language about retrying
callbacsk, I think you might be right.  (And then maybe we do need to
track callback status per-session.)  I'm not sure if this has much
practical effect for now.

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

OK.  I may not get to this very soon.

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

Oops, you're right.  May as well make that unconditional.

> > 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?”

I don't remember, it could be that we really only need two states.

--b.