Re: [PATCH] io: adjust io provider for NFS tracepoint variants

[Kris Van Hees <kris.van.hees@oracle.com>]

On Wed, Jan 08, 2025 at 11:34:36AM +0000, Nick Alcock wrote:
> On 8 Jan 2025, Eugene Loh uttered the following:
> 
> > You already have a R-b, it seems the code is right (woo hoo! cool), and we're trying to get a release out the door.  But here are my
> > "late to the party" comments.  Feel free to ignore.
> 
> Ditto :)
> 
> > The names v1 and v2 strike me as funny;  they just make up new numbers.  Not a big deal, but ideally the names might more closely
> > reflect the actual version changes we're tracking.
> 
> Agreed.

See my rationale in my reply to Eugene.

> > Breaking the function out into a new "v1" version is some unneeded copy-and-paste code bloat, I think.  The function is kind of
> > large and the only difference between v1 and v2 is I guess in the two, relatively short, "start" sections.  So, keeping this stuff
> > as one function -- with extra logic in the two "start" sections -- might be more compact and clearer than having two rather similar
> > functions.
> 
> Agreed, but more generally it seems we are growing increasingly many
> fairly similar fake bio functions in the io provider: with this I think
> we're up to three. Maybe it's time to refactor some of this into some
> sort of shared fake bio machinery? Certainly if we grow more I think we
> should (and given the number of network filesystems, that seems quite
> likely to happen).

The trouble is that they are also sufficiently different, and with how we need
to generate code to trace down pointer chains etc in various ways, consolidated
code can get more convoluted and hard to read.  It will remain a balance
between sharing code with possible lack of clarity and more complexity, or
code duplication with more clarity and reduces complexity.  Sometimes it gets
hard to judge which is better, but this certainly can be looked at and we can
go from there.

> > On 1/7/25 17:44, Kris Van Hees wrote:
> >> Kernels prior to 5.6.0 pass 3 arguments (derived from the NFS hdr)
> >> to the nfs_initiate_read raw tracepoint, whereas kernels as of 5.6.0
> >> pass just the NFS hdr.
> >
> > Is the same true of write?  If so, then maybe say so and point out that what we're really doing is changing the handling of nfs
> > "start" while leaving nfs "done" alone...  that would correspond more closely to what is happening in the code, which talks of start
> > and done.
> 
> Good thinkiing. That would be commit
> 5bb2a7cb9fe58d2b1efedd6058d442c7871c45ec ("NFS: Clean up generic
> writeback tracepoints"), also by Trond, also first landing in 5.6-rc1.
> Same sort of thing.