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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> 
> > On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> 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@kernel.org>
> > > 
> > > 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.

> 
> > then that will also
> > require us to keep the bitmap size (in another 32-bit word), since we
> > don't always want to set anything in the third word. That's already 24
> > extra bits per delegation, and we'll be adding new fields for the
> > timestamps in a later patch.
> > 
> > I don't see the benefit here.
> 
> Understood, there's a memory scalability issue.
> 
> There are other ways to go about this that do not grow
> the size of the delegation data structure, I think. For
> instance, you could store the handful of actual valid
> bitmap values in read-only memory, and have the proc
> function select and reference one of them. IIRC the
> client already does this for certain GETATTR operations.
> 

Unfortunately, it turns out that the bitmap is a bit more complicated,
and we don't quite do it right today. I did a more careful read of
section 10.4.3 in RFC8881. It has this pseudocode:

    if (!modified) {
        do CB_GETATTR for change and size;

        if (cc != sc)
            modified = TRUE;
    } else {
        do CB_GETATTR for size;
    }

    if (modified) {
        sc = sc + 1;
        time_modify = time_metadata = current_time;
        update sc, time_modify, time_metadata into file's metadata;
    }

Note that if the thing is already considered to be modified, then it
says to not request FATTR4_CHANGE. I have a related question around
this on the IETF list too.

> So, leave this patch as is, and I will deal with it
> later when we convert the callback XDR functions.
> 

Thanks, will do.
-- 
Jeff Layton <jlayton@kernel.org>