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

[Chuck Lever III <chuck.lever@oracle.com>]

> 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)


> 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