The patch titled
Subject: fs: do not prefault sys_write() user buffer pages
has been removed from the -mm tree. Its filename was
fs-do-not-prefault-sys_write-user-buffer-pages.patch
This patch was dropped because it was merged into mainline or a subsystem tree
------------------------------------------------------
From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Subject: fs: do not prefault sys_write() user buffer pages
=== Short summary ====
iov_iter_fault_in_readable() works around a really rare case and we can
avoid the deadlock it addresses in another way: disable page faults and
work around copy failures by faulting after the copy in a slow path
instead of before in a hot one.
I have a little microbenchmark that does repeated, small writes to tmpfs.
This patch speeds that micro up by 6.2%.
=== Long version ===
When doing a sys_write() we have a source buffer in userspace and then a
target file page.
If both of those are the same physical page, there is a potential deadlock
that we avoid. It would happen something like this:
1. We start the write to the file
2. Allocate page cache page and set it !Uptodate
3. Touch the userspace buffer to copy in the user data
4. Page fault (since source of the write not yet mapped)
5. Page fault code tries to lock the page and deadlocks
(more details on this below)
To avoid this, we prefault the page to guarantee that this fault does not
occur. But, this prefault comes at a cost. It is one of the most
expensive things that we do in a hot write() path (especially if we
compare it to the read path). It is working around a pretty rare case.
To fix this, it's pretty simple. We move the "prefault" code to run after
we attempt the copy. We explicitly disable page faults _during_ the copy,
detect the copy failure, then execute the "prefault" ouside of where the
page lock needs to be held.
iov_iter_copy_from_user_atomic() actually already has an implicit
pagefault_disable() inside of it (at least on x86), but we add an explicit
one. I don't think we can depend on every kmap_atomic() implementation to
pagefault_disable() for eternity.
===================================================
The stack trace when this happens looks like this:
[<ffffffff81172130>] wait_on_page_bit_killable+0xc0/0xd0
[<ffffffff811721c4>] __lock_page_or_retry+0x84/0xa0
[<ffffffff811723cd>] filemap_fault+0x1ed/0x3d0
[<ffffffff81197841>] __do_fault+0x41/0xc0
[<ffffffff8119b8db>] handle_mm_fault+0x9bb/0x1210
[<ffffffff8109945f>] __do_page_fault+0x17f/0x3d0
[<ffffffff810996bc>] do_page_fault+0xc/0x10
[<ffffffff8183ba92>] page_fault+0x22/0x30
[<ffffffff8116ff1a>] generic_perform_write+0xca/0x1a0
[<ffffffff81171e30>] __generic_file_write_iter+0x190/0x1f0
[<ffffffff8126d659>] ext4_file_write_iter+0xe9/0x460
[<ffffffff811d027a>] __vfs_write+0xaa/0xe0
[<ffffffff811d0ae6>] vfs_write+0xa6/0x1a0
[<ffffffff811d1726>] SyS_write+0x46/0xa0
[<ffffffff81839f17>] entry_SYSCALL_64_fastpath+0x12/0x6a
[<ffffffffffffffff>] 0xffffffffffffffff
(Note, this does *NOT* happen in practice today because
the kmap_atomic() does a pagefault_disable(). The trace
above was obtained by taking out the pagefault_disable().)
You can trigger the deadlock with this little code snippet:
fd = open("foo", O_RDWR);
fdmap = mmap(NULL, len, PROT_WRITE|PROT_READ, MAP_SHARED, fd, 0);
write(fd, &fdmap[0], 1);
Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxx>
Cc: Jens Axboe <axboe@xxxxxx>
Cc: Tejun Heo <tj@xxxxxxxxxx>
Cc: NeilBrown <neilb@xxxxxxx>
Cc: Matthew Wilcox <matthew.r.wilcox@xxxxxxxxx>
Cc: Paul Cassella <cassella@xxxxxxxx>
Cc: Greg Thelen <gthelen@xxxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
mm/filemap.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff -puN mm/filemap.c~fs-do-not-prefault-sys_write-user-buffer-pages mm/filemap.c
--- a/mm/filemap.c~fs-do-not-prefault-sys_write-user-buffer-pages
+++ a/mm/filemap.c
@@ -2473,21 +2473,6 @@ ssize_t generic_perform_write(struct fil
iov_iter_count(i));
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(iov_iter_fault_in_readable(i, bytes))) {
- status = -EFAULT;
- break;
- }
-
status = a_ops->write_begin(file, mapping, pos, bytes, flags,
&page, &fsdata);
if (unlikely(status < 0))
@@ -2495,8 +2480,17 @@ again:
if (mapping_writably_mapped(mapping))
flush_dcache_page(page);
-
+ /*
+ * 'page' is now locked. If we are trying to copy from a
+ * mapping of 'page' in userspace, the copy might fault and
+ * would need PageUptodate() to complete. But, page can not be
+ * made Uptodate without acquiring the page lock, which we hold.
+ * Deadlock. Avoid with pagefault_disable(). Fix up below with
+ * iov_iter_fault_in_readable().
+ */
+ pagefault_disable();
copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
+ pagefault_enable();
flush_dcache_page(page);
status = a_ops->write_end(file, mapping, pos, bytes, copied,
@@ -2519,6 +2513,14 @@ again:
*/
bytes = min_t(unsigned long, PAGE_CACHE_SIZE - offset,
iov_iter_single_seg_count(i));
+ /*
+ * This is the fallback to recover if the copy from
+ * userspace above faults.
+ */
+ if (unlikely(iov_iter_fault_in_readable(i, bytes))) {
+ status = -EFAULT;
+ break;
+ }
goto again;
}
pos += copied;
_
Patches currently in -mm which might be from dave.hansen@xxxxxxxxxxxxxxx are
--
To unsubscribe from this list: send the line "unsubscribe mm-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
