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