998ef75ddb and aio-dio-invalidate-failure w/ data=journal

What I think is going on is that when we do attempt the copy, we end up marking the page dirty before we notice that we need to page fault in the page, which ends up triggering the warning that jbd2 buffer_head that is supposed to be journaled has been marked dirty without calling ext4_handle_dirty_metadata() --- which is handled by ext4_journalled_write_end(), but which is now happening out of order given this commit.

Is it possible that we can change iov_iter_copy_from_user_atomic(), to check for the error case before it marks the page dirty?

Or can we
create a light-weight function which checks to see if the page needs to be faulted in which is lighter weight than iov_iter_fault_in_readable?

Dave, mind sharing the micro-benchmark or perhaps even just a kernel profile of it? How is that "iov_iter_fault_in_readable()" so noticeable? It really shouldn't be a big deal.

     7.45%  write1byte_proc  [kernel.kallsyms]     [k] copy_user_enhanced_fast_string 
     6.51%  write1byte_proc  [kernel.kallsyms]     [k] unlock_page                    
     6.04%  write1byte_proc  [kernel.kallsyms]     [k] iov_iter_fault_in_readable     
     5.23%  write1byte_proc  libc-2.20.so          [.] __GI___libc_write              
     4.86%  write1byte_proc  [kernel.kallsyms]     [k] entry_SYSCALL_64               

     4.48%  write1byte_proc  [kernel.kallsyms]     [k] iov_iter_copy_from_user_atomic 
     3.94%  write1byte_proc  [kernel.kallsyms]     [k] generic_perform_write          
     3.74%  write1byte_proc  [kernel.kallsyms]     [k] mutex_lock                     
     3.59%  write1byte_proc  [kernel.kallsyms]     [k] entry_SYSCALL_64_after_swapgs  
     3.55%  write1byte_proc  [kernel.kallsyms]     [k] find_get_entry                 
     3.53%  write1byte_proc  [kernel.kallsyms]     [k] vfs_write                      
     3.17%  write1byte_proc  [kernel.kallsyms]     [k] find_lock_entry                
     3.17%  write1byte_proc  [kernel.kallsyms]     [k] put_page                       

       │      stac
 24.57 │      mov    (%rcx),%sil
 15.70 │      clac
 28.77 │      test   %eax,%eax
  2.15 │      mov    %sil,-0x1(%rbp)
  8.93 │    ↓ jne    66
  2.31 │      movslq %edx,%rdx

One thing I've been noticing on Skylake is that barriers (implicit and explicit) are showing up more in profiles.

What we're seeing here
probably isn't actually stac/clac overhead, but the cost of finishing some other operations that are outstanding before we can proceed through here.

Quoted 2 lines · Dave Hansen

One thing I've been noticing on Skylake is that barriers (implicit and explicit) are showing up more in profiles.

Ahh, you're on skylake?

It's entirely possible that the issue is that the whole "stac/mov/clac" is much more expensive because skylake actually ends up supporting those AC instructions. That would make sense.

We could probably do them outside the loop, rather than tightly around the actual move instructions. Peter (hpa), is there some sane interface to try to do that?

Quoted 4 lines

What we're seeing here
probably isn't actually stac/clac overhead, but the cost of finishing some other operations that are outstanding before we can proceed through here.

I suspect it actually _is_ stac/clac overhead. It might well be that clac/stac ends up serializing loads some way. Last I heard, they were reasonably cheap but certainly not free - and when we're talking about something that just loops over bringing the line into cache, it might be relatively expensive.

How did you do the profile? Use "-e cycles:pp" to get the precise profile information, which should actually attribute the cost to the instruction that really causes it.

       │      stac
 24.57 │      mov    (%rcx),%sil
 15.70 │      clac
 28.77 │      test   %eax,%eax
  2.15 │      mov    %sil,-0x1(%rbp)
  8.93 │    ↓ jne    66
  2.31 │      movslq %edx,%rdx

       │      sub    $0x8,%rsp
 24.57 │      stac
 15.49 │      mov    (%rcx),%sil
 29.06 │      clac
  2.24 │      test   %eax,%eax
  8.77 │      mov    %sil,-0x1(%rbp)
  2.22 │    ↓ jne    66
       │      movslq %edx,%rdx

Although I was probably wrong about the source of the overhead, the point still remains that the prefaulting is eating cycles for no practical benefit.

With "-e cycles:pp":

Quoted 8 lines

       │      sub    $0x8,%rsp
 24.57 │      stac
 15.49 │      mov    (%rcx),%sil
 29.06 │      clac
  2.24 │      test   %eax,%eax
  8.77 │      mov    %sil,-0x1(%rbp)
  2.22 │    ↓ jne    66
       │      movslq %edx,%rdx

Your ext4 patch may well fix the issue, and be the right thing to do (_regardless_ of the revert, in fact - while it might make the revert unnecessary, it might also be a good idea even if we do revert).

Quoted 4 lines · Linus Torvalds

Your ext4 patch may well fix the issue, and be the right thing to do (_regardless_ of the revert, in fact - while it might make the revert unnecessary, it might also be a good idea even if we do revert).

Thinking a bit more about your patch, I actually am getting more and more convinced that it's the wrong thing to do.

Why?

Because the whole "Setting copied=0 will tell the upper layers to repeat the write" just seems a nasty layering violation, where the low-level filesystem uses a magic return code to introduce a special case at the upper layers. But the upper layers are actually already *aware* of the special case, and in fact have a comment about it.

The ext4 side still worries me, though. You made that "page_zero_new_buffers()" conditional on "copied" being non-zero, but I'm not convinced it can be conditional. Even if we retry, that retry may end up failing (for example, because the source isn't mapped, so we return -EFAULT rather than re-doing the write), but we have those new buffers that got allocated in write_begin(), and now nobody has ever written any data to them at all, so they have random stale contents.

So I do think this needs more thought. Or at least somebody should explain to me better why it's all ok.

I'm attaching the "copied = 0" special case thing at the generic_perform_write() level as a patch for comments. But for now I still think that reverting would seem to be the safer thing (which still possibly leaves things buggy with a racy unmap, but at least it's the old bug that we've never hit in practice).

mm/filemap.c +6 -0

@@ -2493,6 +2493,20 @@ again:
 		pagefault_enable();
 		flush_dcache_page(page);
 
+		/*
+		 * If we didn't successfully copy all the data from user space,
+		 * and the target page is not up-to-date, we will have to prefault
+		 * the source. And if the page wasn't up-to-date before the write,
+		 * the "write_end()" may need to *make* it up-to-date, and thus
+		 * overwrite our partial copy.

Did you mean that as a cleanup, or to fix the regression?

Since the page isn't faulted in yet, iov_iter_copy_from_user_atomic() had already set copied=0

We could probably do them outside the loop, rather than tightly around the actual move instructions. Peter (hpa), is there some sane interface to try to do that?

Quoted 4 lines · Linus Torvalds

We could probably do them outside the loop, rather than tightly around the actual move instructions. Peter (hpa), is there some sane interface to try to do that?

There are... in particular the get_user_try/catch mechanism, used among other things by the signal handing, as well as the copy functions (I don't have the original code in front of me so I don't have the context which would be appropriate.)

Yes, but note that those interfaces are x86 only at the moment, so they'd have to be factored out and generalized before we can use it in generic code.

Quoted 3 lines · Ingo Molnar

Yes, but note that those interfaces are x86 only at the moment, so they'd have to be factored out and generalized before we can use it in generic code.

ARM64 these days (as a part of ARM8.1) has "Privileged Access Never", which is their name for SMAP. They do it somewhat similarly with an instruction to clear/set the PAN bit in the pstate register.

So we really should strive to make this support generic, because by now it's not x86-specific, and x86 and ARM64 together aren't exactly some odd special case..

I do in fact wonder if we should aim (eventually) for the rule that the "__" versions of the user access functions should not do the SMAP/PAN thing, since they have to be explicitly checked for pointer being valid anyway. And just make the rule be that since you have to check for the pointer being valid, you might as well also have to do the SMAP/PAN thing too.

We really should try get rid of _all_ uses of the "__" versions unless they are very locally and obviously checked with access_ok(). We've had way too many cases where people thought they were clever, and weren't really.

Quoted 5 lines

We really should try get rid of _all_ uses of the "__" versions unless they are very locally and obviously checked with access_ok(). We've had way too many cases where people thought they were clever, and weren't really.

That's a good idea.

The logistics worries me a bit: it looks like a major undertaking, considering the widespread use of these APIs in 1400+ call sites:

From past experience, I don't think we have anything that really

Quoted 10 lines · Ingo Molnar

Quoted 5 lines

We really should try get rid of _all_ uses of the "__" versions unless they are very locally and obviously checked with access_ok(). We've had way too many cases where people thought they were clever, and weren't really.

That's a good idea.

The logistics worries me a bit: it looks like a major undertaking, considering the widespread use of these APIs in 1400+ call sites:

Well, quite frankly, I think I'd be ok with just a mass conversion of the "__" functions to non-underscore ones.

From past experience, I don't think we have anything that really cares. The one exception is probably the signal stack handling, because it really uses multiple individual accesses, and so it is much more noticeable.

And there should be *no* meaningful difference between the underscore version and the non-underscore one, unless somebody does something really odd and questionable (ie depends on a kernel pointer - which doesn't even work on all architectures!).

So I really think we could do a mass conversion of everything that isn't under "arch/" (and obviously asm-generic/uaccess.h) in just one single go.

I obviously wouldn't take that into 4.3, but I really don't think this would merit splitting up into multiple patches either.

Then, one by one, we might convert back to __get/put_user() when we've (a) added the SMAP/PAN infrastructure (b) verified that there's an access_ok() _right_there_ and (c) actually verified that it's performance-critical.

I see drivers doing __get/put_user(), and it just makes me go "no". Not only are drivers likely to get it wrong, I don't believe the extra couple of cycles is going to matter compared to the cost of the hardware access itself. And if the access_ok() isn't local and obviously in the *only* place that can possibly lead to that code, then the code shouldn't use the underscore versions.

I've bisected it down to commit 998ef75ddb: "fs: do not prefault sys_write() user buffer pages". I've confirmed that 4.3-rc2 fails as detailed below, but with 998ef75ddb reverted, the problem goes away.

Before commit 998ef75ddb, if we need to prefault in the page, we do so before we attempt the copy. After this commit, we attempt the copy and if it fails because pagefaults have been turned off, we call write_end(), the unlock the page, prefault in the pages, and then retry the commit.

What I think is going on is that when we do attempt the copy, we end up marking the page dirty before we notice that we need to page fault in the page, which ends up triggering the warning that jbd2 buffer_head that is supposed to be journaled has been marked dirty without calling ext4_handle_dirty_metadata() --- which is handled by ext4_journalled_write_end(), but which is now happening out of order given this commit.

Is it possible that we can change iov_iter_copy_from_user_atomic(), to check for the error case before it marks the page dirty? Or can we create a light-weight function which checks to see if the page needs to be faulted in which is lighter weight than iov_iter_fault_in_readable?

static int ext4_journalled_write_end(struct file *file, ... unsigned copied, ... {

        else {
                if (copied < len) {
                        if (!PageUptodate(page))
                                copied = 0;

                        page_zero_new_buffers(page, from+copied, to);
                }

                ret = ext4_walk_page_buffers(handle, page_buffers(page), from,
                                             to, &partial, write_end_fn);
                if (!partial)
                        SetPageUptodate(page);
        }

Quoted 31 lines

 2)  aio-dio-3204  |               |              ext4_journalled_write_end() {
 2)  aio-dio-3204  |               |                /* ext4_journalled_write_end: dev 8,1 ino 12 pos 0 len 4096 copied 0 */
 2)  aio-dio-3204  |               |                page_zero_new_buffers() {
 2)  aio-dio-3204  |               |                  mark_buffer_dirty() {
 2)  aio-dio-3204  |               |                    /* block_dirty_buffer: 8,1 sector=34356 size=4096 */
 2)  aio-dio-3204  |   0.025 us    |                    page_mapping();
 2)  aio-dio-3204  |               |                    __set_page_dirty.constprop.46() {
 2)  aio-dio-3204  |   0.026 us    |                      _raw_spin_lock_irqsave();
 2)  aio-dio-3204  |               |                      account_page_dirtied() {
 2)  aio-dio-3204  |               |                        /* writeback_dirty_page: bdi 8:0: ino=12 index=0 */
 2)  aio-dio-3204  |               |                        __inc_zone_page_state() {
 2)  aio-dio-3204  |   0.030 us    |                          __inc_zone_state();
 2)  aio-dio-3204  |   0.211 us    |                        }
 2)  aio-dio-3204  |               |                        __inc_zone_page_state() {
 2)  aio-dio-3204  |   0.024 us    |                          __inc_zone_state();
 2)  aio-dio-3204  |   0.211 us    |                        }
 2)  aio-dio-3204  |   1.085 us    |                      }
 2)  aio-dio-3204  |   0.033 us    |                      _raw_spin_unlock_irqrestore();
 2)  aio-dio-3204  |   1.727 us    |                    }
 2)  aio-dio-3204  |               |                    __mark_inode_dirty() {
 2)  aio-dio-3204  |               |                      /* writeback_mark_inode_dirty: bdi 8:0: ino=12 state=I_DIRTY_SYNC|I_DIRTY_DATASYNC|I_DIRTY_PAGES flags=I_DIRTY_PAGES */
 2)  aio-dio-3204  |   0.207 us    |                    }
 2)  aio-dio-3204  |   2.600 us    |                  }
 2)  aio-dio-3204  |   2.878 us    |                }
 2)  aio-dio-3204  |               |                ext4_walk_page_buffers() {
 2)  aio-dio-3204  |               |                  write_end_fn() {
 2)  aio-dio-3204  |               |                    __ext4_handle_dirty_metadata() {
 2)  aio-dio-3204  |   0.024 us    |                      _cond_resched();
 2)  aio-dio-3204  |               |                      jbd2_journal_dirty_metadata() {
 2)  aio-dio-3204  |   0.025 us    |                        _raw_spin_lock();
 2)  aio-dio-3204  |               |                        __jbd2_journal_file_buffer() {

Quoted 8 lines

 2)  aio-dio-3204  |               |                          warn_dirty_buffer.isra.10() {
 2)  aio-dio-3204  |               |                            bdevname() {
 2)  aio-dio-3204  | + 15.280 us   |                              disk_name();
 2)  aio-dio-3204  | + 15.661 us   |                            }
 2)  aio-dio-3204  |               |                            /* ^A4JBD2: Spotted dirty metadata buffer (dev = sda1, blocknr = 34356). There's a risk of filesystem corruption in case of system crash. */
 2)  aio-dio-3204  |               |                            bdevname() {
 2)  aio-dio-3204  |   0.080 us    |                              disk_name();
 2)  aio-dio-3204  |   0.298 us    |                            }

The warning comes out of ext4_walk_page_buffers() and the dirty state comes from page_zero_new_buffers(). That seems a _bit_ goofy that the filesystem is marking the page dirty and then so shortly warning about it.

So your patch looks good, but in addition to that, if copied is > 0 and less than len, we shouldn't be calling page_zero_new_buffers(). We're going to need our own version of it that doesn't call mark_buffer_dirty().

So if Linus wants to revert 998ef75ddb patch, we can do that, but I'm also happy applying your patch as a way of preventing the failure.

And none of *those* requirements change just because "copied" would be zero. If you avoid zeroing the buffers and marking them dirty, nothing will ever initialize them on disk, andn if the prefault then later fails during retry, no later write will happen either. So now eventually later, a read() can see stale data from disk.

I do think this is an ext4 bug, and you'll need to do something *like* that patch. Maybe Dave's patch is good as-is. It's the "I think you need to do more" that I worry about. Not at -rc4 time. Not with a core filesystem like ext4. Let's not hurry this too much.

If there is a error while copying data from userspace into the page cache during a write(2) system call, in data=journal mode, in ext4_journalled_write_end() were using page_zero_new_buffers() from fs/buffer.c. Unfortunately, this sets the buffer dirty flag, which is no good if journalling is enabled. This is a long-standing bug that goes back for years and years in ext3, but a combination of (a) data=journal not being very common, (b) in many case it only results in a warning message. and (c) only very rarely causes the kernel hang, means that we only really noticed this as a problem when commit 998ef75ddb caused this failure to happen frequently enough to cause generic/208 to fail when run in data=journal mode.

The fix is to have our own version of this function that doesn't call mark_dirty_buffer(), since we will end up calling ext4_handle_dirty_metadata() on the buffer head(s) in questions very shortly afterwards in ext4_journalled_write_end().

Thanks to Dave Hansen and Linus Torvalds for helping to identify the root cause of the problem.

Signed-off-by Theodore Ts'o <[email protected]>

---
 fs/ext4/inode.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

fs/ext4/inode.c +0 -0

index ae52e32..0a589bb 100644

fs/ext4/inode.c +29 -1

+ */
+static void zero_new_buffers(struct page *page, unsigned from, unsigned to)
+{
+	unsigned int block_start = 0, block_end;
+	struct buffer_head *head, *bh;
+
+	bh = head = page_buffers(page);
+	do {
+		block_end = block_start + bh->b_size;
+		if (buffer_new(bh)) {
+			if (block_end > from && block_start < to) {
+				if (!PageUptodate(page)) {
+					unsigned start, size;
+
+					start = max(from, block_start);
+					size = min(to, block_end) - start;
+
+					zero_user(page, start, size);
+					set_buffer_uptodate(bh);
+				}
+				clear_buffer_new(bh);
+			}
+		}
+		block_start = block_end;
+		bh = bh->b_this_page;
+	} while (bh != head);
+}
+
 static int ext4_journalled_write_end(struct file *file,
 				     struct address_space *mapping,
 				     loff_t pos, unsigned len, unsigned copied,

@@ -1207,7 +1239,7 @@ static int ext4_journalled_write_end(struct file *file,
 		if (copied < len) {
 			if (!PageUptodate(page))
 				copied = 0;
-			page_zero_new_buffers(page, from+copied, to);
+			zero_new_buffers(page, from+copied, to);
 		}
 
 		ret = ext4_walk_page_buffers(handle, page_buffers(page), from,

If there is a error while copying data from userspace into the page cache during a write(2) system call, in data=journal mode, in ext4_journalled_write_end() were using page_zero_new_buffers() from fs/buffer.c. Unfortunately, this sets the buffer dirty flag, which is no good if journalling is enabled. This is a long-standing bug that goes back for years and years in ext3, but a combination of (a) data=journal not being very common, (b) in many case it only results in a warning message. and (c) only very rarely causes the kernel hang, means that we only really noticed this as a problem when commit 998ef75ddb caused this failure to happen frequently enough to cause generic/208 to fail when run in data=journal mode.

The fix is to have our own version of this function that doesn't call mark_dirty_buffer(), since we will end up calling ext4_handle_dirty_metadata() on the buffer head(s) in questions very shortly afterwards in ext4_journalled_write_end().

Thanks to Dave Hansen and Linus Torvalds for helping to identify the root cause of the problem.

Signed-off-by Theodore Ts'o <[email protected]>

---
 fs/ext4/inode.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

fs/ext4/inode.c +33 -1

index ae52e32..0a589bb 100644
@@ -1181,6 +1181,38 @@ errout:
 	return ret ? ret : copied;
 }
 
+/*
+ * This is a private version of page_zero_new_buffers() which doesn't
+ * set the buffer to be dirty, since in data=journalled mode we need
+ * to call ext4_handle_dirty_metadata() instad.
+ */
+static void zero_new_buffers(struct page *page, unsigned from, unsigned to)
+{
+	unsigned int block_start = 0, block_end;
+	struct buffer_head *head, *bh;
+
+	bh = head = page_buffers(page);
+	do {
+		block_end = block_start + bh->b_size;
+		if (buffer_new(bh)) {
+			if (block_end > from && block_start < to) {
+				if (!PageUptodate(page)) {
+					unsigned start, size;
+
+					start = max(from, block_start);
+					size = min(to, block_end) - start;
+
+					zero_user(page, start, size);
+					set_buffer_uptodate(bh);
+				}
+				clear_buffer_new(bh);
+			}
+		}
+		block_start = block_end;
+		bh = bh->b_this_page;
+	} while (bh != head);
+}
+
 static int ext4_journalled_write_end(struct file *file,
 				     struct address_space *mapping,
 				     loff_t pos, unsigned len, unsigned copied,

@@ -1207,7 +1239,7 @@ static int ext4_journalled_write_end(struct file *file,
 		if (copied < len) {
 			if (!PageUptodate(page))
 				copied = 0;
-			page_zero_new_buffers(page, from+copied, to);
+			zero_new_buffers(page, from+copied, to);
 		}
 
 		ret = ext4_walk_page_buffers(handle, page_buffers(page), from,

Signature · 6 lines

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

If there is a error while copying data from userspace into the page cache during a write(2) system call, in data=journal mode, in ext4_journalled_write_end() were using page_zero_new_buffers() from fs/buffer.c. Unfortunately, this sets the buffer dirty flag, which is no good if journalling is enabled. This is a long-standing bug that goes back for years and years in ext3, but a combination of (a) data=journal not being very common, (b) in many case it only results in a warning message. and (c) only very rarely causes the kernel hang, means that we only really noticed this as a problem when commit 998ef75ddb caused this failure to happen frequently enough to cause generic/208 to fail when run in data=journal mode.

The fix is to have our own version of this function that doesn't call mark_dirty_buffer(), since we will end up calling ext4_handle_dirty_metadata() on the buffer head(s) in questions very shortly afterwards in ext4_journalled_write_end().

Thanks to Dave Hansen and Linus Torvalds for helping to identify the root cause of the problem.

The commit itself does not appear to be buggy per se, but it is exposing a bug in ext4 (and Ted thinks ext3 too, but we solved that by getting rid of it). It's too late in the release cycle to really worry about this, even if Dave Hansen has a patch that may actually fix the underlying ext4 problem. We can (and should) revisit this for the next release.

Hello there, a blast from the past.

I see this has landed in b90197b655185a11640cce3a0a0bc5d8291b8ad2

I came here from looking at a pwrite vs will-it-scale and noticing that pre-faulting eats CPU (over 5% on my Sapphire Rapids) due to SMAP trips.

It used to be that pre-faulting was avoided specifically for that reason, but it got temporarily reverted due to bugs in ext4, to quote Linus (see 00a3d660cbac05af34cca149cb80fb611e916935):

Quoted 4 lines · Mateusz Guzik

Hello there, a blast from the past.

I see this has landed in b90197b655185a11640cce3a0a0bc5d8291b8ad2

Whee. What archeology are you doing to notice this decade-old issue?

Quoted 2 lines

I came here from looking at a pwrite vs will-it-scale and noticing that pre-faulting eats CPU (over 5% on my Sapphire Rapids) due to SMAP trips.

Ugh. Yeah, turning SMAP on/off is expensive on most cores (apparently fixed in AMD Zen 5).

Quoted 3 lines

It used to be that pre-faulting was avoided specifically for that reason, but it got temporarily reverted due to bugs in ext4, to quote Linus (see 00a3d660cbac05af34cca149cb80fb611e916935):

Yeah, I think we should revert the revert (except we've done other changes in the last decade - surprise surprise - so it would be a completely manual revert).

If you send me a tested revert of the revert (aka re-do) of the "don't pre-fault" patch, I'll apply it.

Note that the ext4 problem could exist in other filesystems, so we might have to revert (again). It's not necessarily that ext4 was _particularly_ buggy, it's quite possible that the problem was originally found on ext4 just because it was more widely used than others.

This being your revert I was lowkey hoping you would do the honors.

Quoted 2 lines · Mateusz Guzik

This being your revert I was lowkey hoping you would do the honors.

If it had been a plain revert, I would have - but while the general layout of the code is similar, the code around this area has changed enough that it really needs to be pretty much entirely "fix up by hand" and wants some care and testing.

Which I am unlikely to have time for during this merge window.

So if you don't get around to it, and _if_ I remember this when the merge window is open, I might do it in my local tree, but then it will end up being too late for this merge window.

Quoted 5 lines

So if you don't get around to it, and _if_ I remember this when the merge window is open, I might do it in my local tree, but then it will end up being too late for this merge window.

The to-be-unreverted change was written by Dave (cc'ed).

I had a brief chat with him on irc, he said he is going to submit an updated patch.

Quoted 9 lines · Mateusz Guzik

Quoted 5 lines

So if you don't get around to it, and _if_ I remember this when the merge window is open, I might do it in my local tree, but then it will end up being too late for this merge window.

The to-be-unreverted change was written by Dave (cc'ed).

I had a brief chat with him on irc, he said he is going to submit an updated patch.

I poked at it a bit today. There's obviously been the page=>folio churn and also iov_iter_fault_in_readable() got renamed and got some slightly new semantics.

Anyway, here's a patch that compiles, boots and doesn't immediately fall over on ext4 in case anyone else wants to poke at it. I'll do a real changelog, SoB, etc.... and send it out for real tomorrow if it holds up.

index 4f476411a9a2d..98b37e4c6d43c 100644

---

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

diff -puN mm/filemap.c~generic_perform_write-1 mm/filemap.c

mm/filemap.c renamed from mm/filemap.c~generic_perform_write-1 +7 -11

@@ -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,15 @@ retry:
 				bytes = copied;
 				goto retry;
 			}
+			/*
+			 * 'folio' is now unlocked and faults on it can be