Message-ID: <52C563DF-88D1-4AAC-B441-9B821A7B32FF@oracle.com>
Date: Wed, 4 Sep 2024 17:28:48 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>
CC: Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo
	<dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Trond Myklebust
	<trondmy@...nel.org>,
        Anna Schumaker <anna@...nel.org>,
        Olga Kornievskaia
	<okorniev@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christian Brauner
	<brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Jonathan Corbet
	<corbet@....net>, Tom Haynes <loghyr@...il.com>,
        Linux Kernel Mailing List
	<linux-kernel@...r.kernel.org>,
        Linux NFS Mailing List
	<linux-nfs@...r.kernel.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field



> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@...nel.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@...nel.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

Powered by blists - more mailing lists