All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Hansen <dave.hansen@linux.intel.com>
To: linux-kernel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,Ted Ts'o
	<tytso@mit.edu>,Christian Brauner <brauner@kernel.org>,Darrick
	J. Wong <djwong@kernel.org>,Matthew Wilcox (Oracle)
	<willy@infradead.org>,Al Viro
	<viro@zeniv.linux.org.uk>,linux-fsdevel@vger.kernel.org,Dave
	Hansen <dave.hansen@linux.intel.com>
Subject: [PATCH 1/7] filemap: Move prefaulting out of hot write path
Date: Wed, 29 Jan 2025 10:17:51 -0800	[thread overview]
Message-ID: <20250129181751.5527308F@davehans-spike.ostc.intel.com> (raw)
In-Reply-To: <20250129181749.C229F6F3@davehans-spike.ostc.intel.com>


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

  reply	other threads:[~2025-01-29 18:17 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-29 18:17 [PATCH 0/7] Move prefaulting into write slow paths Dave Hansen
2025-01-29 18:17 ` Dave Hansen [this message]
2025-01-29 18:17 ` [PATCH 2/7] iomap: Move prefaulting out of hot write path Dave Hansen
2025-01-31  7:59   ` Christoph Hellwig
2025-01-29 18:17 ` [PATCH 3/7] ntfs3: " Dave Hansen
2025-01-29 18:17 ` [PATCH 4/7] fuse: " Dave Hansen
2025-04-15  8:43   ` Miklos Szeredi
2025-01-29 18:17 ` [PATCH 5/7] bcachefs: " Dave Hansen
2025-01-29 18:18 ` [PATCH 6/7] btrfs: " Dave Hansen
2025-01-29 18:18 ` [PATCH 7/7] netfs: Remove outdated comments about prefaulting Dave Hansen
2025-01-30  7:44 ` [PATCH 0/7] Move prefaulting into write slow paths Kent Overstreet
2025-01-30 16:04   ` Dave Hansen
2025-01-30 21:36     ` Dave Chinner
2025-01-31  1:06       ` Kent Overstreet
2025-01-31  0:56     ` Kent Overstreet
2025-01-31  1:34       ` Dave Hansen
2025-01-31  2:17         ` Kent Overstreet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250129181751.5527308F@davehans-spike.ostc.intel.com \
    --to=dave.hansen@linux.intel.com \
    --cc=brauner@kernel.org \
    --cc=djwong@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.