Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[NeilBrown]

On Thu, 05 Sep 2024, Chuck Lever III wrote:
> 
> 
> > On Sep 4, 2024, at 1:39 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > 
> > On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> >> 
> >>> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >>> 
> >>> On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
> >>>> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> >>>>> This is always the same value, and in a later patch we're going to need
> >>>>> to set bits in WORD2. We can simplify this code and save a little space
> >>>>> in the delegation too. Just hardcode the bitmap in the callback encode
> >>>>> function.
> >>>>> 
> >>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >>>> 
> >>>> OK, lurching forward on this ;-)
> >>>> 
> >>>> - The first patch in v3 was applied to v6.11-rc.
> >>>> - The second patch is now in nfsd-next.
> >>>> - I've squashed the sixth and eighth patches into nfsd-next.
> >>>> 
> >>>> I'm replying to this patch because this one seems like a step
> >>>> backwards to me: the bitmap values are implementation-dependent,
> >>>> and this code will eventually be automatically generated based only
> >>>> on the protocol, not the local implementation. IMO, architecturally,
> >>>> bitmap values should be set at the proc layer, not in the encoders.
> >>>> 
> >>>> I've gone back and forth on whether to just go with it for now, but
> >>>> I thought I'd mention it here to see if this change is truly
> >>>> necessary for what follows. If it can't be replaced, I will suck it
> >>>> up and fix it up later when this encoder is converted to an xdrgen-
> >>>> manufactured one.
> >>> 
> >>> It's not truly necessary, but I don't see why it's important that we
> >>> keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
> >>> field in the delegation record, and it has to be carried around in
> >>> perpetuity. This value is always set by the server and there are only a
> >>> few legit bit combinations that can be set in it.
> >>> 
> >>> We certainly can keep this bitmap around, but as I said in the
> >>> description, the delstid draft grows this bitmap to 3 words, and if we
> >>> want to be a purists about storing a bitmap,
> >> 
> >> Fwiw, it isn't purism about storing the bitmap, it's
> >> purism about adopting machine-generated XDR marshaling/
> >> unmarshaling code. The patch adds non-marshaling logic
> >> in the encoder. Either we'll have to add a way to handle
> >> that in xdrgen eventually, or we'll have to exclude this
> >> encoder from machine generation. (This is a ways down the
> >> road, of course)
> >> 
> > 
> > Understood. I'll note that this function works with a nfs4_delegation
> > pointer too, which is not part of the protocol spec.
> > 
> > Once we move to autogenerated code, we will always have a hand-rolled
> > "glue" layer that morphs our internal representation of these sorts of
> > objects into a form that the xdrgen code requires. Storing this info as
> > a flag in the delegation makes more sense to me, as the glue layer
> > should massage that into the needed form.
> 
> My thought was the existing proc functions would do
> the massaging for us. But that's an abstract,
> architectural kind of thought. I'm just starting to
> integrate this idea into lockd.

So presumably xdrgen defines a bunch of structs.  The proc code fills
those in (on the stack?) and passes them to the generated xdr code which
encodes them to the network stream??  That seems reasonable.

We still don't want ncf_cb_bmap in struct nfs4_cb_fattr, we only need it
in the generated struct.

Thanks,
NeilBrown