Re: [PATCH 0/7] Move prefaulting into write slow paths

[Dave Hansen <dave.hansen@intel.com>]

On 1/30/25 16:56, Kent Overstreet wrote:
> On Thu, Jan 30, 2025 at 08:04:49AM -0800, Dave Hansen wrote:...
>> Any suggestions for fully describing the situation? I tried to sprinkle
>> comments liberally but I'm also painfully aware that I'm not doing a
>> perfect job of talking about the fs code.
> 
> The critical thing to cover is the fact that mmap means that page faults
> can recurse into arbitrary filesystem code, thus blowing a hole in all
> our carefully crafted lock ordering if we allow that while holding
> locks - you didn't mention that at all.

What I've got today is this:

              /*
               * This needs to be atomic because actually handling page
               * faults on 'i' can deadlock if the copy targets a
               * userspace mapping of 'folio'.
               */
	      copied = copy_folio_from_iter_atomic(...);

Are you saying you'd prefer that this be something more like:

		/*
		 * Faults here on mmap()s can recurse into arbitrary
		 * filesystem code. Lots of locks are held that can
		 * deadlock. Use an atomic copy to avoid deadlocking
		 * in page fault handling.
		 */

?

>>> I do agree on moving it to the slowpath - I think we can expect the case
>>> where the process's immediate workingset is faulted out while it's
>>> running to be vanishingly small.
>>
>> Great! I'm glad we're on the same page there.
>>
>> 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?
> 
> No to the sit and pray. If I see one more "testing? that's something
> other people do" conversation I'll blow another gasket.
> 
> xfstests supports bcachefs, and if you need a really easy way to run it
> locally on all the various filesystems, I have a solution for that:
> 
> https://evilpiepirate.org/git/ktest.git/
> 
> If you want access to my CI that runs all that in parallel across 120
> VMs with the nice dashboard - shoot me an email and I'll outline server
> costs and we can work something out.

That _sounds_ a bit heavyweight to me for this patch:

 b/fs/bcachefs/fs-io-buffered.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

Is that the the kind of testing (120 VMs) that is needed to get a patch
into bcachefs?

Or are you saying that running xfstests on bcachefs with this patch
applied would be sufficient?

On the x86 side, I'm usually pretty happy to know that someone has
compiled a patch and at least executed the code at runtime a time or
two. So this process is a bit unfamiliar to me.