On Thu, Jan 30, 2025 at 08:04:49AM -0800, Dave Hansen wrote: > On 1/29/25 23:44, Kent Overstreet wrote: > > On Wed, Jan 29, 2025 at 10:17:49AM -0800, Dave Hansen wrote: > >> tl;dr: The VFS and several filesystems have some suspect prefaulting > >> code. It is unnecessarily slow for the common case where a write's > >> source buffer is resident and does not need to be faulted in. > >> > >> Move these "prefaulting" operations to slow paths where they ensure > >> forward progress but they do not slow down the fast paths. This > >> optimizes the fast path to touch userspace once instead of twice. > >> > >> Also update somewhat dubious comments about the need for prefaulting. > >> > >> This has been very lightly tested. I have not tested any of the fs/ > >> code explicitly. > > > > Q: what is preventing us from posting code to the list that's been > > properly tested? > > > > I just got another bcachefs patch series that blew up immediately when I > > threw it at my CI. > > > > This is getting _utterly ridiculous_. That's a bit of an over-reaction, Kent. IMO, the developers and/or maintainers of each filesystem have some responsibility to test changes like this themselves as part of their review process. That's what you have just done, Kent. Good work! However, it is not OK to rant about how the proposed change failed because it was not exhaustively tested on every filesytem before it was posted. I agree with Dave - it is difficult for someone to test widepsread changes in code outside their specific expertise. In many cases, the test infrastructure just doesn't exist or, if it does, requires specialised knowledge and tools to run. In such cases, we have to acknowledge that best effort testing is about as good as we can do without overly burdening the author of such a change. In these cases, it is best left to the maintainer of that subsystem to exhaustively test the change to their subsystem.... Indeed, this is the whole point of extensive post-merge integration testing (e.g. the testing that gets run on linux-next -every day-). It reduces the burden on individuals proposing changes created by requiring exhaustive testing before review by amortising the cost of that exhaustive testing over many peer reviewed changes.... > In this case, I started with a single patch for generic code that I knew > I could test. In fact, I even had the 9-year-old binary sitting on my > test box. > > Dave Chinner suggested that I take the generic pattern go look a _bit_ > more widely in the tree for a similar pattern. That search paid off, I > think. But I ended up touching corners of the tree I don't know well and > don't have test cases for. Many thanks for doing the search, identifying all the places where this pattern existed and trying to address them, Dave. > For bcachefs specifically, how should we move forward? If you're happy > with the concept, would you prefer that I do some manual bcachefs > testing? Or leave a branch sitting there for a week and pray the robots > test it? The public automated test robots are horribly unreliable with their coverage of proposed changes. Hence my comment above about the subsystem maintainers bearing some responsibility to test the code as part of their review process.... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
