> 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) > 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. So, leave this patch as is, and I will deal with it later when we convert the callback XDR functions. -- Chuck Lever
