> On Mar 26, 2017, at 10:38 PM, Chuck Lever <chucklever@xxxxxxxxx> wrote:
>
> Hey Jeff-
>
>
>>> On Mar 26, 2017, at 9:21 PM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
>>>
>>> On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote:
>>> Same change as Kinglong Mee's fix for the TCP backchannel service.
>>>
>>
>> Good catch. I guess I didn't do a good job of hunting down all of the
>> transports where this needed to be set. I'll give them another pass
>> again tomorrow to make sure I didn't miss any others.
>>
>>> Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...")
>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>> ---
>>> Some (perhaps late) review comments on 5283b03ee5cd:
>>>
>>> I have reservations about returning RPC_PROG_MISMATCH in this case.
>>> RPC_PROG_UNAVAIL is more sensible. But the use of UDP with NFSv4 is
>>> not an RPC-level error, thus reporting the problem here seems like a
>>> layering violation.
>>>
>>> I'm not sure why an explicit check is needed: if the server isn't
>>> listening on UDP, wouldn't clients see a transport-level rejection
>>> (like ECONNREFUSED)?
>>>
>>
>> Sure, if the server isn't listening on UDP...
>>
>> The point of that patch is to enforce not allowing v4 over UDP when the
>> server is listening on UDP to serve earlier versions.
>>
>> As far as the error...From RFC 5531:
>>
>> PROG_UNAVAIL = 1, /* remote hasn't exported program */
>> PROG_MISMATCH = 2, /* remote can't support version # */
>>
>> Consider the case where the server is listening on both TCP and UDP,
>> and is serving both v3 and v4. Someone tries to send a v4 RPC over UDP.
>>
>> The RPC program in that case (nfs) is supported over UDP, but the
>> version (v4) is not. So I disagree here. PROG_MISMATCH seems like the
>> better fit to me.
>
> Then the server should report the correct version range in the
> rejection. The RPC response I saw on the wire claimed that 4
> was the maximum supported version.
Of course, versions 2 and 3 do not make sense for
the backchannel. So I'm not sure what you would report
in that case.
>>> Are we certain that all client implementations (including
>>> backchannel clients) will do something useful when presented with
>>> such a rejection? At least in the backchannel case, the Linux server
>>> had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
>>> The workload stopped dead, no error report anywhere.
>>>
>>
>> Ouch. I think this would get translated into EPROTONOSUPPORT in the
>> client code. That should have ended up with nfsd4_mark_cb_down being
>> called with that error?...but I think that function may be effectively
>> neutered:
>>
>> static void warn_no_callback_path(struct nfs4_client *clp, int reason)
>> {
>> dprintk("NFSD: warning: no callback path to client %.*s: error %d\n",
>> (int)clp->cl_name.len, clp->cl_name.data, reason);
>> }
>>
>> Note that it emits a dprintk instead of a printk. Should we promote
>> that to something more visible?
>
> You don't want a warning if the client never provided a
> callback path. But if one was provided, and it disappears,
> that might be useful to know.
>
> OTOH some might blanch at the log flood, should something
> else go wrong.
>
> An error counter might be the least we can do, if not a
> one-shot pr_warn.
>
>
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> index c13a5c3..fc8f14c 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
>>> xprt = &cma_xprt->sc_xprt;
>>>
>>> svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
>>> + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
>>> serv->sv_bc_xprt = xprt;
>>>
>>> dprintk("svcrdma: %s(%p)\n", __func__, xprt);
>>>
>>> --
>>> 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
>>
>> --
>> 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
> --
> 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
--
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
