On Thu, 2013-07-18 at 15:50 +0000, Adamson, Dros wrote:
> On Jul 18, 2013, at 11:21 AM, "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx>
> wrote:
>
> > On Thu, 2013-07-18 at 15:10 +0000, Adamson, Dros wrote:
> >> On Jul 18, 2013, at 10:57 AM, "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:
> >>
> >>> On Thu, 2013-07-18 at 14:46 +0000, Adamson, Dros wrote:
> >>>> Hey,
> >>>>
> >>>> I have a mostly functional client-side implementation of SP4_MACH_CRED! It still needs a lot of cleanup and testing.
> >>>>
> >>>> I have one style issue that I want to run by the list before I post a patchset:
> >>>>
> >>>> So, SP4_MACH_CRED negotiates two bitmaps in the EXCHANGE_ID (one "enforce", one "allow") for state protection. These bitmaps are indexed by the NFSv4 operation number. The state protect check must happen in the nfs4proc.c layer (or before), right before we call rpc_call_sync or equivalent, so that it can select the right cred and rpc_client.
> >>>>
> >>>> Here's the problem: we don't know what operations (opcodes) are actually in a compound until the XDR encode callback is called. The rpc_procinfo array doesn't have this mapping - in fact, it only lives in the xdr encode layer.
> >>>>
> >>>> One approach is to immediately translate the opcode bitmaps to "nfs4 procedure index" bitmaps, indexing into the rpc_procinfo array. This would mean there is a second mapping of NFSv4 procedure -> opcodes that must be updated when an XDR encode callback is changed.
> >>>>
> >>>> Another approach would be to add a callback to the XDR api so we could "ask" it if an NFSv4 procedure contains any of the opcodes in a bitmap. The nice thing about this approach is that the mapping of procedure to opcodes within will live right next to the XDR encode callback and make it obvious that both need to be changed together.
> >>>>
> >>>> I suppose I'm leaning toward a combination of both of these approaches - keep the mapping in XDR-land and translate the bitmaps immediately on negation for fast lookups during normal operation.
> >>>>
> >>>> Comments? Am I missing something?
> >>>
> >>> I'm not sure that I understand. We don't do dynamic creation of
> >>> compounds: we pretty much know in the nfs4_proc_* routine what the main
> >>> operation is (the one exception being CLOSE/OPEN_DOWNGRADE). So why
> >>> can't we work out the protection status at that time? Is the worry that
> >>> the server might reboot and come back with different MACH_CRED
> >>> protections?
> >>
> >> Sure, we know what the main operation is and we know what operations will end up in a compound - we can just look at the xdr encoder. My question is simply one of style. Do we want to have each nfs4proc procedure to have a list of operations that must be updated if an xdr encoder is updated? I'm fine with doing it this way, it just seemed wrong to have the same mapping in two different places. This is why I'm asking ;)
> >>
> >> Also, we can't just use the "main" operation, we must check every operation within the compound and if any are required to use SP4, then the whole compound does. I don't necessarily know why a server would do this, but if we follow the spec and a server informs the client that SETATTR must use state protection, then a WRITE with post-op SETATTR must use the state protection even though WRITE doesn't need it.
> >
> > We only need to check every _stateful_ operation, right? I can't think
> > of any compounds with more than 1 stateful operation. We've deliberately
> > avoided those due to the problems that arise when you get a
> > NFS4ERR_DELAY or something equivalent at the wrong moment.
>
> That may be the intention, but I see no such limitation in the spec. It's my understanding that the server could specify any operation as MUST or MAY use state protection.
Right, but what would state protection mean for a stateless operation
such as GETATTR? That operation isn't even allowed to return
NFS4ERR_WRONG_CRED, so I don't see how it would apply.
> >> I'm not following the reboot question - that case should be handled just fine. Once the EXCHANGE_ID happens, the clp has two bitmaps (enforce and allow) that reflect the current mode of SP4_MACH_CRED.
> >
> > I'm thinking something along the following scenario:
> >
> > A process queues up an OPEN. The server replies NFS4ERR_BAD_SESSION and
> > later replies with NFS4ERR_STALE_CLIENTID. So we start reboot recovery,
> > and the OPEN gets queued waiting for a slot.
> > We then send a new EXCHANGE_ID, and the server replies with a
> > _different_ MACH_CRED protection for OPEN.
> >
> > The problem above is that the OPEN has already been started, and we've
> > already assigned it a credential. How do we handle that?
> > Ditto question for READ, WRITE, LOCK, LAYOUTGET,...
> >
> > Do we have a problem for LOCKU, CLOSE and OPEN_DOWNGRADE? I think we end
> > up just skipping recovery in those cases…
>
> These are good points. I'm not sure *why* the sp4_mach_cred config would change, but I suppose it could, and yes, in my current implementation there is no way to back out once a rpc task is invoked. Any thoughts on this? I can see two ways of handling this:
> - restarting all tasks (with rpc_clnt and rpc_cred appropriately changed) when a change in sp4_mach_cred config is detected.
> - somehow having the RPC layer call out to check SP4 status before sending each RPC.
For now, I think we should make sure that we handle NFS4ERR_WRONG_CRED
correctly in these cases. I don't think we ever want to handle that in
nfs4_async_handle_error, but we should definitely handle it in
nfs4_handle_exception(), since that can drive a full retry of the RPC
call.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
