Message-ID: <0bc0bc150313d5cb91578f1b692bc62e066aa9e7.camel@kernel.org>
Date: Fri, 16 Aug 2024 12:34:16 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Chuck Lever <chuck.lever@...cle.com>
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>, Tom Haynes
 <loghyr@...il.com>, linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR
 encoding

On Fri, 2024-08-16 at 12:16 -0400, Chuck Lever wrote:
> On Fri, Aug 16, 2024 at 11:45:32AM -0400, Jeff Layton wrote:
> > On Fri, 2024-08-16 at 11:17 -0400, Chuck Lever wrote:
> > > On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> > > > This adds support for the "delstid" draft:
> > > > 
> > > >    
> > > > https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> > > > 
> > > > Most of this was autogenerated using Chuck's lkxdrgen tool with
> > > > some
> > > > by-hand tweaks to work around some symbol conflicts, and to add
> > > > some
> > > > missing pieces that were needed from nfs4_1.x.
> > > 
> > > I haven't read delstid closely enough to comment on the approach
> > > you've taken in 3/3, but I do have some thoughts about code
> > > organization here. I will try to study that draft soon.
> > > 
> > > And, I'm assuming you are continuing to evolve support for the
> > > draft
> > > and will be growing this series. So I will hold off on careful
> > > inspection of 3/3 for the moment.
> > > 
> > 
> > Yes. The client pieces are already in place, and I read I get is
> > that
> > the draft is all but done at this point. 3/3 is probably pretty
> > close
> > to what should go in. There are really 3 parts to the delstid
> > draft:
> > 
> > 1/ the OPEN_XOR_DELEGATION part, which allows the server to avoid
> > giving out an open stateid when giving out a deleg.
> > 
> > 2/ delegated timestamp support (which is the part I'm still looking
> > at)
> > 
> > 3/ FATTR4_OPEN_ARGUMENTS (which enables 1 and 2)
> > 
> > This patchset encompasses 1 & 3. Part 2 shouldn't have much bearing
> > on
> > this set. It's really a separate feature entirely that just happens
> > to
> > also depend on FATTR4_OPEN_ARGUMENTS support.
> > 
> > > First, I'm pleased that you found xdrgen useful for rapid
> > > prototyping. That's not something I had envisioned when I created
> > > the tool, but it's a good match, looks like.
> > > 
> > 
> > Yeah. It has some bugs that still need fixing, but it was certainly
> > better than having to roll all of that by hand.
> 
> I'm very interested to hear bug reports.
> 

I should have been more diligent about collecting them! I'll do that
soon.

> 
> > > Here you add a separate set of source files for delstid XDR... 
> > > That
> > > approach might not be scalable for adding subsequent new features
> > > in
> > > general, it occurs to me.
> > > 
> > > We might end up with a bunch of these little code blurbs with no
> > > clear understanding of how they inter-relate.  Thoughts about how
> > > to
> > > manage these are welcome: xdrgen could generate only header files
> > > and then we would #include them where needed, for example.
> > > 
> > > For now, I suggest folding the new generated XDR code into the
> > > existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you
> > > have
> > > some time for cleaning up the patches. An alternative would be to
> > > leave it and I can fold these together before committing.
> > > 
> > > (The long term, of course, will hopefully be generating all XDR
> > > code
> > > automatically, but we're a ways out from that, IMO).
> > > 
> > 
> > This is where I disagree.
> > 
> > The nice thing about lkxdrgen is that most of what it generates is
> > fairly self-contained. I think we ought to try to keep the new
> > (mostly
> > autogenerated) and old code (hand-rolled) separate for now.
> > 
> > I'm not sure what makes the most sense for a new naming scheme, but
> > I
> > really don't think we should paste all of this into nfs4xdr.c,
> > which is
> > just a huge jumble of hand-rolled XDR code. 
> 
> nfs4xdr.c is a mix of stuff that I constructed by rote, which is
> pretty clean, and stuff that mixes the "just serialize" logic with
> "do the proc part as well" logic, which is more ugly. I had thought
> that OPEN's XDR was in the former category, but I get it. 
> 
> So I still think there's a scalability problem with adding each new
> feature in its own XDR .c and .h, but I don't mind keeping the
> generated code separate from the legacy code. How about creating one
> new file to collect the mostly- or all-generated XDR code?
> 
> I've been using fs/nfsd/nfs[34]gen_xdr.[ch] in my testing.
> 

Sure, that sounds fine. I'll rename delstid_xdr.* to nfs4gen_xdr.*.
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists