> On Jul 2, 2024, at 2:32 PM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > > On Tue, Jul 02, 2024 at 06:06:09PM +0000, Chuck Lever III wrote: >> >> >>> On Jul 2, 2024, at 12:28 PM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: >>> >>> Hi, >>> >>> There seems to be consensus that these changes worthwhile and >>> extensively iterated on. >> >> I don't see a public consensus about "extensively iterated >> on". The folks you talk to privately might believe that, >> though. >> >> >>> I'd very much like these changes to land upstream as-is (unless review >>> teases out some show-stopper). These changes have been tested fairly >>> extensively (via xfstests) at this point. >>> >>> Can we now please provide formal review tags and merge these changes >>> through the NFS client tree for 6.11? >> >> Contributors don't get to determine the kernel release where >> their code lands; maintainers make that decision. You've stated >> your preference, and we are trying to accommodate. But frankly, >> the (server) changes don't stand up to close inspection yet. >> >> One of the client maintainers has had years to live with this >> work. But the server maintainers had their first look at this >> just a few weeks ago, and this is not the only thing any of us >> have on our plates at the moment. So you need to be patient. >> >> >>> FYI: >>> - I do not intend to rebase this series ontop of NeilBrown's partial >>> exploration of simplifying away the need for a "fake" svc_rqst >>> (noble goals and happy to help those changes land upstream as an >>> incremental improvement): >>> https://marc.info/?l=linux-nfs&m=171980269529965&w=2 >> >> Sorry, rebasing is going to be a requirement. > > What? You're imposing a rebase on completely unfinished and untested > code? Any idea when Neil will post v2? Or am I supposed to take his > partial first pass and fix it? Don't be ridiculous. Wait for Neil to post a working version. >> Again, as with the dprintk stuff, this is code that would get >> reverted or replaced as soon as we merge. We don't knowingly >> merge that kind of code; we fix it first. > > Nice rule, except there is merit in tested code landing without it > having to see last minute academic changes. These aren't dprintk, > these are disruptive changes that aren't fully formed. If they were > fully formed I wouldn't be resisting them. It's your server patch that isn't fully formed. Allocating a fake svc_rqst outside of an svc thread context and adding a work-around to avoid the cache lookup deferral is nothing but a hacky smelly prototype. It's not merge-ready or -worthy. > If Neil completes his work I'll rebase. But last time I rebased to > his simplification of the localio protocol (to use array and not > lists, nice changes, appreciated but it took serious work on my part > to fold them in): the code immediately BUG_ON()'d in sunrpc trivially. You should be very grateful that Neil is writing your code for you. He's already contributed much more than you have any reason to expect from someone who is not employed by Hammerspace. And quite frankly, it is not reasonable to expect anyone's freshly written code to be completely free of bugs. I'm sorry it took you a little while to find the problem, but it will become easier when you become more familiar with the code base. > So please be considerate of my time and the requirement for code to > actually work. I'll be considerate when you are considerate of our time and stop patch bombing the list with tiny incremental changes, demanding we "get the review done and merge it" before it is ready. Honestly, the work is proceeding quite unremarkably for a new feature. The problem seems to be that you don't understand why we're asking for (actually quite small) changes before merging, and we're asking you to do that work. Why are we asking you to do it? It's because you are asking for /our/ time. But we don't work for Hammerspace and do not have any particular interest in localIO and have no real way to test the facility yet (no, running fstests does not count as a full test). It's your responsibility to get this code put together, it's got to be your time and effort. You are getting paid to deal with this. None of the rest of us are. No-one else is asking for this feature. >>> - In addition, tweaks to use nfsd_file_acquire_gc() instead of >>> nfsd_file_acquire() aren't a priority. >> >> The discussion has moved well beyond that now... IIUC the >> preferred approach might be to hold the file open until the >> local app is done with it. However, I'm still not convinced >> there's a benefit to using the NFSD file cache vs. a plain >> dentry_open(). > > Saving an nfs_file to open_context, etc. All incremental improvement > (that needs time to stick the landing). You are still missing the point. The phony svc_rqst is being passed into nfsd_file_acquire(). Either we have to fix nfsd_file_acquire (as Neil did) or replace it's use with fh_verify() / dentry_open(). This is not about garbage collection, and hasn't been for a while. It's about replacing unmergable prototype code. And sticking the landing? If a few good fstests results are supposed to be good enough for us to merge your code as it exists now, why aren't they good enough to verify your code is OK to merge after a rebase? -- Chuck Lever
