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. > > 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. -- Chuck Lever
