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

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

[Dave Hansen]

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.

I started by just trying to deal with generic_perform_write() and
looked at a few more cases after Dave Chinner mentioned there was
some apparent proliferation of its pattern across the tree.

I think the first patch is probably OK for 6.14. If folks are OK
with other ones, perhaps they can just them up individually for
their trees.

--

More detailed cover letter below.

There are logically two pieces of data involved in a write operation:
a source that is read from and a target which is written to, like:

	sys_write(target_fd, &source, len);

This is implemented in generic VFS code and several filesystems
with loops that look something like this:

	do {
		fault_in_iov_iter_readable(source)
		// lock target folios
		copy_folio_from_iter_atomic()
		// unlock target folios
	} while(iov_iter_count(iter))

They fault in the source first and then proceed to do the write.  This
fault is ostensibly done for a few reasons:

 1. Deadlock avoidance if the source and target are the same
    folios.
 2. To check the user address that copy_folio_from_iter_atomic()
    will touch because atomic user copies do not check the address.
 3. "Optimization"

I'm not sure any of these are actually valid reasons.

The "atomic" user copy functions disable page fault handling because
page faults are not very atomic. This makes them naturally resistant
to deadlocking in page fault handling. They take the page fault
itself but short-circuit any handling.

copy_folio_from_iter_atomic() also *does* have user address checking.
I get a little lost in the iov_iter code, but it does know when it's
dealing with userspace versus kernel addresses and does seem to know
when to do things like copy_from_user_iter() (which does access_ok())
versus memcpy_from_iter().[1]

The "optimization" is for the case where 'source' is not faulted in.
It can avoid the cost of a "failed" page fault (it will fail to be
handled because of the atomic copy) and then needing to drop locks and
repeat the fault.

But the common case is surely one where 'source' *is* faulted in.
Usually, a program will put some data in a buffer and then write it to
a file in very short order. Think of something as simple as:

	sprintf(buf, "Hello world");
	write(fd, buf, len);

In this common case, the fault_in_iov_iter_readable() incurs the cost
of touching 'buf' in userspace twice.  On x86, that means at least an
extra STAC/CLAC pair.

Optimize for the case where the source buffer has already been faulted
in. Ensure forward progress by doing the fault in slow paths when the
atomic copies are not making progress.

That logically changes the above loop to something more akin to:

	do {
		// lock target folios
		copied = copy_folio_from_iter_atomic()
		// unlock target folios

		if (unlikely(!copied))
			fault_in_iov_iter_readable(source)
	} while(iov_iter_count(iter))

1. The comment about atomic user copies not checking addresses seems
   to have originated in 08291429cfa6 ("mm: fix pagecache write
   deadlocks") circa 2007. It was true then, but is no longer true.

 fs/bcachefs/fs-io-buffered.c |   30 ++++++++++--------------------
 fs/btrfs/file.c              |   20 +++++++++++---------
 fs/fuse/file.c               |   14 ++++++++++----
 fs/iomap/buffered-io.c       |   24 +++++++++---------------
 fs/netfs/buffered_write.c    |   13 +++----------
 fs/ntfs3/file.c              |   17 ++++++++++++-----
 mm/filemap.c                 |   26 +++++++++++++++-----------
 7 files changed, 70 insertions(+), 74 deletions(-)

Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: ntfs3@lists.linux.dev
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: linux-bcachefs@vger.kernel.org
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: David Howells <dhowells@redhat.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: netfs@lists.linux.dev

[PATCH 1/7] filemap: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

There is a bit of a sordid history here. I originally wrote
998ef75ddb57 ("fs: do not prefault sys_write() user buffer pages")
to fix a performance issue that showed up on early SMAP hardware.
But that was reverted with 00a3d660cbac because it exposed an
underlying filesystem bug.

This is a reimplementation of the original commit along with some
simplification and comment improvements.

The basic problem is that the generic write path has two userspace
accesses: one to prefault the write source buffer and then another to
perform the actual write. On x86, this means an extra STAC/CLAC pair.
These are relatively expensive instructions because they function as
barriers.

Keep the prefaulting behavior but move it into the slow path that gets
run when the write did not make any progress. This avoids livelocks
that can happen when the write's source and destination target the
same folio. Contrary to the existing comments, the fault-in does not
prevent deadlocks. That's accomplished by using an "atomic" usercopy
that disables page faults.

The end result is that the generic write fast path now touches
userspace once instead of twice. That should speed things up.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Link: https://lore.kernel.org/all/yxyuijjfd6yknryji2q64j3keq2ygw6ca6fs5jwyolklzvo45s@4u63qqqyosy2/
Cc: Ted Ts'o <tytso@mit.edu>

---

 b/mm/filemap.c |   26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff -puN mm/filemap.c~generic_perform_write-1 mm/filemap.c
--- a/mm/filemap.c~generic_perform_write-1	2025-01-29 09:03:30.963260106 -0800
+++ b/mm/filemap.c	2025-01-29 09:03:30.971260772 -0800
@@ -4027,17 +4027,6 @@ retry:
 		bytes = min(chunk - offset, bytes);
 		balance_dirty_pages_ratelimited(mapping);
 
-		/*
-		 * Bring in the user page that we will copy from _first_.
-		 * Otherwise there's a nasty deadlock on copying from the
-		 * same page as we're writing to, without it being marked
-		 * up-to-date.
-		 */
-		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
-			status = -EFAULT;
-			break;
-		}
-
 		if (fatal_signal_pending(current)) {
 			status = -EINTR;
 			break;
@@ -4055,6 +4044,11 @@ retry:
 		if (mapping_writably_mapped(mapping))
 			flush_dcache_folio(folio);
 
+		/*
+		 * 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(folio, offset, bytes, i);
 		flush_dcache_folio(folio);
 
@@ -4080,6 +4074,16 @@ retry:
 				bytes = copied;
 				goto retry;
 			}
+
+			/*
+			 * 'folio' is now unlocked and faults on it can be
+			 * handled. Ensure forward progress by trying to
+			 * fault it in now.
+			 */
+			if (fault_in_iov_iter_readable(i, bytes) == bytes) {
+				status = -EFAULT;
+				break;
+			}
 		} else {
 			pos += status;
 			written += status;
_

[PATCH 2/7] iomap: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Prefaulting the write source buffer incurs an extra userspace access
in the common fast path. Make iomap_write_iter() consistent with
generic_perform_write(): only touch userspace an extra time when
copy_folio_from_iter_atomic() has failed to make progress.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
---

 b/fs/iomap/buffered-io.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff -puN fs/iomap/buffered-io.c~iomap-postfault fs/iomap/buffered-io.c
--- a/fs/iomap/buffered-io.c~iomap-postfault	2025-01-29 09:03:32.179361299 -0800
+++ b/fs/iomap/buffered-io.c	2025-01-29 09:03:32.187361965 -0800
@@ -937,21 +937,6 @@ retry:
 		if (bytes > length)
 			bytes = length;
 
-		/*
-		 * Bring in the user page that we'll copy from _first_.
-		 * Otherwise there's a nasty deadlock on copying from the
-		 * same page as we're writing to, without it being marked
-		 * up-to-date.
-		 *
-		 * For async buffered writes the assumption is that the user
-		 * page has already been faulted in. This can be optimized by
-		 * faulting the user page.
-		 */
-		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
-			status = -EFAULT;
-			break;
-		}
-
 		status = iomap_write_begin(iter, pos, bytes, &folio);
 		if (unlikely(status)) {
 			iomap_write_failed(iter->inode, pos, bytes);
@@ -1005,6 +990,15 @@ retry:
 				bytes = copied;
 				goto retry;
 			}
+			/*
+			 * 'folio' is now unlocked and faults on it can be
+			 * handled. Ensure forward progress by trying to
+			 * fault it in now.
+			 */
+			if (fault_in_iov_iter_readable(i, bytes) == bytes) {
+				status = -EFAULT;
+				break;
+			}
 		} else {
 			pos += written;
 			total_written += written;
_

Re: [PATCH 2/7] iomap: Move prefaulting out of hot write path

[Christoph Hellwig]

Only this patch out of 7 made it to linux-xfs.  I have no way to
review the patch without the proper context.  Please make sure you
always Cc all patches to all recipients.

[PATCH 3/7] ntfs3: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Prefaulting the write source buffer incurs an extra userspace access
in the common fast path. Make ntfs_compress_write() consistent with
generic_perform_write(): only touch userspace an extra time when
copy_page_from_iter_atomic() has failed to make progress.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: ntfs3@lists.linux.dev
---

 b/fs/ntfs3/file.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff -puN fs/ntfs3/file.c~ntfs-postfault fs/ntfs3/file.c
--- a/fs/ntfs3/file.c~ntfs-postfault	2025-01-29 09:03:33.371460504 -0800
+++ b/fs/ntfs3/file.c	2025-01-29 09:03:33.375460837 -0800
@@ -1092,11 +1092,6 @@ static ssize_t ntfs_compress_write(struc
 		frame_vbo = pos & ~(frame_size - 1);
 		index = frame_vbo >> PAGE_SHIFT;
 
-		if (unlikely(fault_in_iov_iter_readable(from, bytes))) {
-			err = -EFAULT;
-			goto out;
-		}
-
 		/* Load full frame. */
 		err = ntfs_get_frame_pages(mapping, index, pages,
 					   pages_per_frame, &frame_uptodate);
@@ -1172,6 +1167,18 @@ static ssize_t ntfs_compress_write(struc
 		 */
 		cond_resched();
 
+		if (unlikely(!copied)) {
+			/*
+			 * folios are now unlocked and faults on them can be
+			 * handled. Ensure forward progress by trying to
+			 * fault in 'from' in case it maps one of the folios.
+			 */
+			if (fault_in_iov_iter_readable(from, bytes)) {
+				err = -EFAULT;
+				goto out;
+			}
+		}
+
 		pos += copied;
 		written += copied;
 
_

[PATCH 4/7] fuse: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Prefaulting the write source buffer incurs an extra userspace access
in the common fast path. Make fuse_fill_write_pages() consistent with
generic_perform_write(): only touch userspace an extra time when
copy_folio_from_iter_atomic() has failed to make progress.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
---

 b/fs/fuse/file.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff -puN fs/fuse/file.c~fuse-postfault fs/fuse/file.c
--- a/fs/fuse/file.c~fuse-postfault	2025-01-29 09:03:34.515555724 -0800
+++ b/fs/fuse/file.c	2025-01-29 09:03:34.523556390 -0800
@@ -1234,10 +1234,6 @@ static ssize_t fuse_fill_write_pages(str
 		bytes = min_t(size_t, bytes, fc->max_write - count);
 
  again:
-		err = -EFAULT;
-		if (fault_in_iov_iter_readable(ii, bytes))
-			break;
-
 		folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
 					    mapping_gfp_mask(mapping));
 		if (IS_ERR(folio)) {
@@ -1254,6 +1250,16 @@ static ssize_t fuse_fill_write_pages(str
 		if (!tmp) {
 			folio_unlock(folio);
 			folio_put(folio);
+
+			/*
+			 * Ensure forward progress by faulting in
+			 * while not holding the folio lock:
+			 */
+			if (fault_in_iov_iter_readable(ii, bytes)) {
+				err = -EFAULT;
+				break;
+			}
+
 			goto again;
 		}
 
_

Re: [PATCH 4/7] fuse: Move prefaulting out of hot write path

[Miklos Szeredi]

On Wed, 29 Jan 2025 at 19:17, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> Prefaulting the write source buffer incurs an extra userspace access
> in the common fast path. Make fuse_fill_write_pages() consistent with
> generic_perform_write(): only touch userspace an extra time when
> copy_folio_from_iter_atomic() has failed to make progress.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>

Applied, thanks.

Miklos

[PATCH 5/7] bcachefs: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Prefaulting the write source buffer incurs an extra userspace access
in the common fast path. Make bch2_buffered_write() consistent with
generic_perform_write(): only touch userspace an extra time when
copy_page_from_iter_atomic() has failed to make progress.

This also zaps a comment. It referred to a possible deadlock and to
userspace address checks. Neither of those things are a concern when
using copy_folio_from_iter_atomic() for atomic usercopies. It
prevents deadlocks by disabling page faults and it leverages user
copy functions that have their own access_ok() checks.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: linux-bcachefs@vger.kernel.org

---

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

diff -puN fs/bcachefs/fs-io-buffered.c~bcachefs-postfault fs/bcachefs/fs-io-buffered.c
--- a/fs/bcachefs/fs-io-buffered.c~bcachefs-postfault	2025-01-29 09:03:35.727656612 -0800
+++ b/fs/bcachefs/fs-io-buffered.c	2025-01-29 09:03:35.731656945 -0800
@@ -970,26 +970,6 @@ static ssize_t bch2_buffered_write(struc
 		unsigned offset = pos & (PAGE_SIZE - 1);
 		unsigned bytes = iov_iter_count(iter);
 again:
-		/*
-		 * Bring in the user page that we will copy from _first_.
-		 * Otherwise there's a nasty deadlock on copying from the
-		 * same page as we're writing to, without it being marked
-		 * up-to-date.
-		 *
-		 * Not only is this an optimisation, but it is also required
-		 * to check that the address is actually valid, when atomic
-		 * usercopies are used, below.
-		 */
-		if (unlikely(fault_in_iov_iter_readable(iter, bytes))) {
-			bytes = min_t(unsigned long, iov_iter_count(iter),
-				      PAGE_SIZE - offset);
-
-			if (unlikely(fault_in_iov_iter_readable(iter, bytes))) {
-				ret = -EFAULT;
-				break;
-			}
-		}
-
 		if (unlikely(fatal_signal_pending(current))) {
 			ret = -EINTR;
 			break;
@@ -1012,6 +992,16 @@ again:
 			 */
 			bytes = min_t(unsigned long, PAGE_SIZE - offset,
 				      iov_iter_single_seg_count(iter));
+
+			/*
+			 * Faulting in 'iter' may be required for forward
+			 * progress. Do it here, out outside the fast path
+			 * and when not holding any folio locks.
+			 */
+			if (fault_in_iov_iter_readable(iter, bytes) == bytes) {
+				ret = -EFAULT;
+				break;
+			}
 			goto again;
 		}
 		pos += ret;
_

[PATCH 6/7] btrfs: Move prefaulting out of hot write path

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Prefaulting the write source buffer incurs an extra userspace access
in the common fast path. Make btrfs_buffered_write() consistent with
generic_perform_write(): only touch userspace an extra time when
copy_folio_from_iter_atomic() has failed to make progress.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
---

 b/fs/btrfs/file.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff -puN fs/btrfs/file.c~btrfs-postfault fs/btrfs/file.c
--- a/fs/btrfs/file.c~btrfs-postfault	2025-01-29 09:03:36.927756510 -0800
+++ b/fs/btrfs/file.c	2025-01-29 09:03:36.935757176 -0800
@@ -1164,15 +1164,6 @@ ssize_t btrfs_buffered_write(struct kioc
 		int extents_locked;
 		bool force_page_uptodate = false;
 
-		/*
-		 * Fault pages before locking them in prepare_one_folio()
-		 * to avoid recursive lock
-		 */
-		if (unlikely(fault_in_iov_iter_readable(i, write_bytes))) {
-			ret = -EFAULT;
-			break;
-		}
-
 		only_release_metadata = false;
 		sector_offset = pos & (fs_info->sectorsize - 1);
 
@@ -1314,6 +1305,17 @@ again:
 
 		cond_resched();
 
+		/*
+		 * Fault pages in a slow path after dropping folio
+		 * lock. This avoids the chance of deadlocking in
+		 * the fault handler.
+		 */
+		if (unlikely(copied == 0) &&
+		    (fault_in_iov_iter_readable(i, write_bytes) == write_bytes)) {
+			ret = -EFAULT;
+			break;
+		}
+
 		pos += copied;
 		num_written += copied;
 	}
_

[PATCH 7/7] netfs: Remove outdated comments about prefaulting

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

I originally set out to make netfs_perform_write() behavior more
consistent with generic_perform_write(). However, netfs currently
treats a failure to make forward progress as a hard error and does not
retry where the generic code will loop around and retry.

Instead of a major code restructuring, just focus on improving the
comments.

The comment refers to a possible deadlock and to userspace address
checks. Neither of those things are a concern when using
copy_folio_from_iter_atomic() for atomic usercopies. It prevents
deadlocks by disabling page faults and it leverages user copy
functions that have their own access_ok() checks.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: netfs@lists.linux.dev
---

 b/fs/netfs/buffered_write.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff -puN fs/netfs/buffered_write.c~netfs-postfault fs/netfs/buffered_write.c
--- a/fs/netfs/buffered_write.c~netfs-postfault	2025-01-29 09:03:38.167859749 -0800
+++ b/fs/netfs/buffered_write.c	2025-01-29 09:03:38.171860082 -0800
@@ -152,16 +152,9 @@ ssize_t netfs_perform_write(struct kiocb
 		offset = pos & (max_chunk - 1);
 		part = min(max_chunk - offset, iov_iter_count(iter));
 
-		/* Bring in the user pages that we will copy from _first_ lest
-		 * we hit a nasty deadlock on copying from the same page as
-		 * we're writing to, without it being marked uptodate.
-		 *
-		 * Not only is this an optimisation, but it is also required to
-		 * check that the address is actually valid, when atomic
-		 * usercopies are used below.
-		 *
-		 * We rely on the page being held onto long enough by the LRU
-		 * that we can grab it below if this causes it to be read.
+		/* Bring in the user folios that are copied from before taking
+		 * locks on the mapping folios. This helps ensure forward
+		 * progress if they are the same folios.
 		 */
 		ret = -EFAULT;
 		if (unlikely(fault_in_iov_iter_readable(iter, part) == part))
_

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

[Kent Overstreet]

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_.

I built multiuser test infrastructure with a nice dashboard that anyone
can use, and the only response I've gotten from the old guard is Ted
jumping in every time I talk about it to say "no, we just don't want to
rewrite our stuff on _your_ stuff!". Real helpful, that.

>  1. Deadlock avoidance if the source and target are the same
>     folios.
>  2. To check the user address that copy_folio_from_iter_atomic()
>     will touch because atomic user copies do not check the address.
>  3. "Optimization"
> 
> I'm not sure any of these are actually valid reasons.
> 
> The "atomic" user copy functions disable page fault handling because
> page faults are not very atomic. This makes them naturally resistant
> to deadlocking in page fault handling. They take the page fault
> itself but short-circuit any handling.

#1 is emphatically valid: the deadlock avoidance is in _both_ using
_atomic when we have locks held, and doing the actual faulting with
locks dropped... either alone would be a buggy incomplete solution.

This needs to be reflected and fully described in the comments, since
it's subtle and a lot of people don't fully grok what's going on.

I'm fairly certain we have ioctl code where this is mishandled and thus
buggy, because it takes some fairly particular testing for lockdep to
spot it.

> copy_folio_from_iter_atomic() also *does* have user address checking.
> I get a little lost in the iov_iter code, but it does know when it's
> dealing with userspace versus kernel addresses and does seem to know
> when to do things like copy_from_user_iter() (which does access_ok())
> versus memcpy_from_iter().[1]
> 
> The "optimization" is for the case where 'source' is not faulted in.
> It can avoid the cost of a "failed" page fault (it will fail to be
> handled because of the atomic copy) and then needing to drop locks and
> repeat the fault.

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.

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

[Dave Hansen]

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_.

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.

> I built multiuser test infrastructure with a nice dashboard that anyone
> can use, and the only response I've gotten from the old guard is Ted
> jumping in every time I talk about it to say "no, we just don't want to
> rewrite our stuff on _your_ stuff!". Real helpful, that.

Sounds pretty cool! Is this something that I could have and should have
used to test the bcachefs patch?  I see some trees in here:

	https://evilpiepirate.org/~testdashboard/ci

But I'm not sure how to submit patches to it. Do you need to add users
manually? I wonder, though, how we could make it easier to find. I
didn't see anything Documentation/filesystems/bcachefs/ about this.

>>  1. Deadlock avoidance if the source and target are the same
>>     folios.
>>  2. To check the user address that copy_folio_from_iter_atomic()
>>     will touch because atomic user copies do not check the address.
>>  3. "Optimization"
>>
>> I'm not sure any of these are actually valid reasons.
>>
>> The "atomic" user copy functions disable page fault handling because
>> page faults are not very atomic. This makes them naturally resistant
>> to deadlocking in page fault handling. They take the page fault
>> itself but short-circuit any handling.
> 
> #1 is emphatically valid: the deadlock avoidance is in _both_ using
> _atomic when we have locks held, and doing the actual faulting with
> locks dropped... either alone would be a buggy incomplete solution.

I was (badly) attempting to separate out the two different problems:

	1. Doing lock_page() twice, which I was mostly calling the
	   "deadlock"
	2. Retrying the copy_folio_from_iter_atomic() forever which I
	   was calling the "livelock"

Disabling page faults fixes #1.
Doing faulting outside the locks somewhere fixes #2.

So when I was talking about "Deadlock avoidance" in the cover letter, I
was trying to focus on the double lock_page() problem.

> This needs to be reflected and fully described in the comments, since
> it's subtle and a lot of people don't fully grok what's going on.

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.

> I'm fairly certain we have ioctl code where this is mishandled and thus
> buggy, because it takes some fairly particular testing for lockdep to
> spot it.

Yeah, I wouldn't be surprised. It was having a little chuckle thinking
about how many engineers have discovered and fixed this problem
independently over the years in all the file system code in all the OSes.

>> copy_folio_from_iter_atomic() also *does* have user address checking.
>> I get a little lost in the iov_iter code, but it does know when it's
>> dealing with userspace versus kernel addresses and does seem to know
>> when to do things like copy_from_user_iter() (which does access_ok())
>> versus memcpy_from_iter().[1]
>>
>> The "optimization" is for the case where 'source' is not faulted in.
>> It can avoid the cost of a "failed" page fault (it will fail to be
>> handled because of the atomic copy) and then needing to drop locks and
>> repeat the fault.
> 
> 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?

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

[Dave Chinner]

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@fromorbit.com

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

[Kent Overstreet]

On Fri, Jan 31, 2025 at 08:36:29AM +1100, Dave Chinner wrote:
> 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.

This is just tone policing.

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

No, it exists - I built it.

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

I keep having the same conversations in code review:
"Did you test this?"
"No really, did you test this?"
"See that error path you modified, that our automated tests definitely
don't cover? You need to test that manually".

Developers don't think enough about testing - and if the excuse is that
the tools suck, then we need to address that. I'm not going to babysit
and do a bunch of manual work - I spend quite enough of my day staring
at test dashboards already, thank you very much.

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

AFAIK the public test robots don't run xfstests at all.

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

[Kent Overstreet]

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_.
> 
> 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.

That's all well and good, but the testing thing is really coming to a
head. I have enough on my plate without back-and-forth 

> > I built multiuser test infrastructure with a nice dashboard that anyone
> > can use, and the only response I've gotten from the old guard is Ted
> > jumping in every time I talk about it to say "no, we just don't want to
> > rewrite our stuff on _your_ stuff!". Real helpful, that.
> 
> Sounds pretty cool! Is this something that I could have and should have
> used to test the bcachefs patch?  I see some trees in here:
> 
> 	https://evilpiepirate.org/~testdashboard/ci
> 
> But I'm not sure how to submit patches to it. Do you need to add users
> manually? I wonder, though, how we could make it easier to find. I
> didn't see anything Documentation/filesystems/bcachefs/ about this.

Yes, I give out user accounts and that gives you a config file you can
edit to specify branches to test and tests to run; it then automatically
watches those branch(es).

Here's the thing though, the servers cost real money. I give out
accounts to community members (and people working on bcachefs are using
it and it's working wel), but if you've got a big tech company email
address you (or your managers) will have to be pitching in. Not
subsidizing you guys :)

> 
> >>  1. Deadlock avoidance if the source and target are the same
> >>     folios.
> >>  2. To check the user address that copy_folio_from_iter_atomic()
> >>     will touch because atomic user copies do not check the address.
> >>  3. "Optimization"
> >>
> >> I'm not sure any of these are actually valid reasons.
> >>
> >> The "atomic" user copy functions disable page fault handling because
> >> page faults are not very atomic. This makes them naturally resistant
> >> to deadlocking in page fault handling. They take the page fault
> >> itself but short-circuit any handling.
> > 
> > #1 is emphatically valid: the deadlock avoidance is in _both_ using
> > _atomic when we have locks held, and doing the actual faulting with
> > locks dropped... either alone would be a buggy incomplete solution.
> 
> I was (badly) attempting to separate out the two different problems:
> 
> 	1. Doing lock_page() twice, which I was mostly calling the
> 	   "deadlock"
> 	2. Retrying the copy_folio_from_iter_atomic() forever which I
> 	   was calling the "livelock"
> 
> Disabling page faults fixes #1.
> Doing faulting outside the locks somewhere fixes #2.
> 
> So when I was talking about "Deadlock avoidance" in the cover letter, I
> was trying to focus on the double lock_page() problem.
> 
> > This needs to be reflected and fully described in the comments, since
> > it's subtle and a lot of people don't fully grok what's going on.
> 
> 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.

> > I'm fairly certain we have ioctl code where this is mishandled and thus
> > buggy, because it takes some fairly particular testing for lockdep to
> > spot it.
> 
> Yeah, I wouldn't be surprised. It was having a little chuckle thinking
> about how many engineers have discovered and fixed this problem
> independently over the years in all the file system code in all the OSes.

Oh, this is the easy one - mmap and dio is where it really gets into
"stories we tell young engineers to keep them awake at night".

> 
> >> copy_folio_from_iter_atomic() also *does* have user address checking.
> >> I get a little lost in the iov_iter code, but it does know when it's
> >> dealing with userspace versus kernel addresses and does seem to know
> >> when to do things like copy_from_user_iter() (which does access_ok())
> >> versus memcpy_from_iter().[1]
> >>
> >> The "optimization" is for the case where 'source' is not faulted in.
> >> It can avoid the cost of a "failed" page fault (it will fail to be
> >> handled because of the atomic copy) and then needing to drop locks and
> >> repeat the fault.
> > 
> > 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.

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

[Dave Hansen]

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.

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

[Kent Overstreet]

On Thu, Jan 30, 2025 at 05:34:18PM -0800, Dave Hansen wrote:
> 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.
> 		 */
> 
> ?

Yeah I like that better, but I would also include the bit about "redoing
the fault when locks are dropped" in the same comment, you want the
whole idea described in one place.

> >>> 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?

No - that's what CI access gets you, and it's not just for bcachefs
testing; it could have tested this series on every filesystem in
parallel for you.

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

Correct.

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

To me, as a filesystem guy, that sounds terrifying :)

I'm a hardass, even among filesystem people, but this really isn't just
about making my life easier, it's about getting us _all_ better tools.
If we can consolidate on a single testrunner for VM tests that people at
least don't hate, then life would get drastically easier for all sorts
of things, including cross subsystem work.

And pretty much exists now, but the younger guys (and engineers in
China) have been a lot quicker to start using it than people who are
used to "the way things have always been"...