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