Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Howells]

Hi Linus,

I've been looking at generic_perform_write() with an eye to adapting a version
for network filesystems in general.  I'm wondering if it's actually safe or
whether it needs 00a3d660cbac05af34cca149cb80fb611e916935 reverting, which is
itself a revert of 998ef75ddb5709bbea0bf1506cd2717348a3c647.

Anyway, I was looking at this bit:

	bytes = min_t(unsigned long, PAGE_SIZE - offset,
					iov_iter_count(i));
	...
	if (unlikely(iov_iter_fault_in_readable(i, bytes))) {
		status = -EFAULT;
		break;
	}

	if (fatal_signal_pending(current)) {
		status = -EINTR;
		break;
	}

	status = a_ops->write_begin(file, mapping, pos, bytes, flags,
					&page, &fsdata);
	if (unlikely(status < 0))
		break;

	if (mapping_writably_mapped(mapping))
		flush_dcache_page(page);

	copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);


and wondering if the iov_iter_fault_in_readable() is actually effective.  Yes,
it can make sure that the page we're intending to modify is dragged into the
pagecache and marked uptodate so that it can be read from, but is it possible
for the page to then get reclaimed before we get to
iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially take
a long time, say if it has to go and get a lock/lease from a server.

Also, I've been thinking about Willy's folio/THP stuff that allows bunches of
pages to be glued together into single objects for efficiency.  This is
problematic with the above code because the faultahead is limited to a maximum
of PAGE_SIZE, but we might be wanting to modify a larger object than that.

David

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Al Viro]

On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:

> and wondering if the iov_iter_fault_in_readable() is actually effective.  Yes,
> it can make sure that the page we're intending to modify is dragged into the
> pagecache and marked uptodate so that it can be read from, but is it possible
> for the page to then get reclaimed before we get to
> iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially take
> a long time, say if it has to go and get a lock/lease from a server.

Yes, it is.  So what?  We'll just retry.  You *can't* take faults while holding
some pages locked; not without shitloads of deadlocks.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

Note this part:

On Tue, Jun 22, 2021 at 8:20 AM David Howells <dhowells@redhat.com> wrote:
>
>         copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);

The "atomic" is the key thing.

The fault_in_readable is just an optimistic "let's make things be mapped".

But yes, it could get unmapped again before the actual copy happens
with the lock held. But that's why the copy is using that atomic
version, so if that happens, we'll end up repeating.

Honestly, the first part comment above the
iov_iter_fault_in_readable() is a bit misleading (the deadlock would
be real _except_ for the atomic part), and it would logically make
sense to only do this for when the actual atomic copy_from_user_atomic
fails. But then you'd have to fault things twice if you do fault.

            Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> and wondering if the iov_iter_fault_in_readable() is actually effective.  Yes,
> it can make sure that the page we're intending to modify is dragged into the
> pagecache and marked uptodate so that it can be read from, but is it possible
> for the page to then get reclaimed before we get to
> iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially take
> a long time, say if it has to go and get a lock/lease from a server.

It's certainly possible, but unlikely.  The page is going to go to the
head of the active queue, and so we'll have to burn our way through the
entire inactive and then active queue in order to bump this page out
of memory.

> Also, I've been thinking about Willy's folio/THP stuff that allows bunches of
> pages to be glued together into single objects for efficiency.  This is
> problematic with the above code because the faultahead is limited to a maximum
> of PAGE_SIZE, but we might be wanting to modify a larger object than that.

Just to be clear, it's not _currently_ a problem for the folio patchset.

Multi-page folios are only created during readahead.  Unless there's a
read error during readahead, a folio found during write() will either
be freshly created and order-0, or it'll be multi-order and uptodate.
I would like to create larger folios during write() eventually, but I'm
choosing to not burden the folio patchset with that enhancement yet.
It has enough performance improvement to not need that yet.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Al Viro]

On Tue, Jun 22, 2021 at 03:27:43PM +0000, Al Viro wrote:
> On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> 
> > and wondering if the iov_iter_fault_in_readable() is actually effective.  Yes,
> > it can make sure that the page we're intending to modify is dragged into the
> > pagecache and marked uptodate so that it can be read from, but is it possible
> > for the page to then get reclaimed before we get to
> > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially take
> > a long time, say if it has to go and get a lock/lease from a server.
> 
> Yes, it is.  So what?  We'll just retry.  You *can't* take faults while holding
> some pages locked; not without shitloads of deadlocks.

Note that the revert you propose is going to do fault-in anyway; we really can't
avoid it.  The only thing it does is optimistically trying without that the
first time around, which is going to be an overall loss exactly in "slow
write_begin" case.  If source pages are absent, you'll get copyin fail;
iov_iter_copy_from_user_atomic() (or its replacement) is disabling pagefaults
itself.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 8:32 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But yes, it could get unmapped again before the actual copy happens
> with the lock held. But that's why the copy is using that atomic
> version, so if that happens, we'll end up repeating.

Side note: search for "iov_iter_fault_in_writeable()" on lkml for a
gfs2 patch-series that is buggy, exactly because it does *not* use the
atomic user space accesses, and just tries to do the fault-in to hide
the real bug.

So you are correct that the fault-in is something people need to be
very wary of. Without the atomic side of the access, it's pure voodoo
programming.

You have two choices:

 - don't hold any filesystem locks (*) over a user space access

 - do the user space access with the atomic versions and repeat (with
pre-faulting to make the repeat work)

There's one special case of that "no filesystem locks" case that I put
that (*) for: you could do a read-recursive lock if the filesystem
page fault path can only ever take read locks. But none of our regular
locks are read-recursive apart from the very special case of the
spinning rwlock in interrupts (see comment in
queued_read_lock_slowpath()).

That special read-recursive model "works", but I would seriously
caution against it, simply because such locks can get very unfair very
quickly. So it's a DoS magnet. It's part of why none of the normal
locking models really have that (any more - rwlocks used to all be
that way).

                     Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Howells]

Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> 
> > and wondering if the iov_iter_fault_in_readable() is actually effective.
> > Yes, it can make sure that the page we're intending to modify is dragged
> > into the pagecache and marked uptodate so that it can be read from, but is
> > it possible for the page to then get reclaimed before we get to
> > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially
> > take a long time, say if it has to go and get a lock/lease from a server.
> 
> Yes, it is.  So what?  We'll just retry.  You *can't* take faults while
> holding some pages locked; not without shitloads of deadlocks.

In that case, can we amend the comment immediately above
iov_iter_fault_in_readable()?

	/*
	 * 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))) {

The first part suggests this is for deadlock avoidance.  If that's not true,
then this should perhaps be changed.

David

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 03:36:22PM +0000, Al Viro wrote:
> On Tue, Jun 22, 2021 at 03:27:43PM +0000, Al Viro wrote:
> > On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> > 
> > > and wondering if the iov_iter_fault_in_readable() is actually effective.  Yes,
> > > it can make sure that the page we're intending to modify is dragged into the
> > > pagecache and marked uptodate so that it can be read from, but is it possible
> > > for the page to then get reclaimed before we get to
> > > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially take
> > > a long time, say if it has to go and get a lock/lease from a server.
> > 
> > Yes, it is.  So what?  We'll just retry.  You *can't* take faults while holding
> > some pages locked; not without shitloads of deadlocks.
> 
> Note that the revert you propose is going to do fault-in anyway; we really can't
> avoid it.  The only thing it does is optimistically trying without that the
> first time around, which is going to be an overall loss exactly in "slow
> write_begin" case.  If source pages are absent, you'll get copyin fail;
> iov_iter_copy_from_user_atomic() (or its replacement) is disabling pagefaults
> itself.

Let's not overstate the case.  I think for the vast majority of write()
calls, the data being written has recently been accessed.  So this
userspace access is unnecessary.  From the commentary around commits
00a3d660cbac and 998ef75ddb57, it seems that Dave had a CPU which was
particularly inefficient at accessing userspace.  I assume Intel have
fixed that by now and the extra load is in the noise.  But maybe enough
CPU errata have accumulated that it's slow again?

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 10:26 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Jun 22, 2021 at 03:36:22PM +0000, Al Viro wrote:
> >
> > Note that the revert you propose is going to do fault-in anyway; we really can't
> > avoid it.  The only thing it does is optimistically trying without that the
> > first time around, which is going to be an overall loss exactly in "slow
> > write_begin" case.  If source pages are absent, you'll get copyin fail;
> > iov_iter_copy_from_user_atomic() (or its replacement) is disabling pagefaults
> > itself.
>
> Let's not overstate the case.  I think for the vast majority of write()
> calls, the data being written has recently been accessed.  So this
> userspace access is unnecessary.

Note that the fault_in_readable is very much necessary - the only
question is whether it happens before the actual access, or after it
in the "oh, it failed, need to retry" case.

There are two cases:

 (a) the user page is there and accessible, and fault_in_readable
isn't necessary

 (b) not

and as you say, case (a) is generally the common one by far, although
it will depend on the exact load (iow, (b) *could* be the common case:
you can have situations where you mmap() things only to then write the
mapping out, and then accesses will fault a lot).

But if it's case (a), then the fault_in_readable is going to be pretty
cheap. We're talking "tens of CPU cycles", unlikely to really be an
issue.

If the case is (b), then the cost is not actually the access at all,
it's the *fault* and the retry. Now we're talking easily thousands of
cycles.

And that's where it matters whether the fault_in_readable is before or
after. If it's before the actual access, then you'll have just _one_
fault, and it will handle the fault.

If the fault_in_readable is only done in the allegedly unlikely
faulting case and is _after_ the actual user space atomic access,
you'll have *two* faults. First the copy_from_user_atomic() will
fault, and return a partial result. But the page won't actually be
populated, so then the fault_in_readable will have to fault _again_,
in order to finally populate the page. And then we retry
(successfully, except for the unbelievably rare case of racing with
pageout) the actual copy_from_user_atomic().

End result: doing the fault_in_readable "unnecessarily" at the
beginning is likely the better optimization. It's basically free when
it's not necessary, and it avoids an extra fault (and extra
lock/unlock and retry) when it does end up faulting pages in.

               Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> End result: doing the fault_in_readable "unnecessarily" at the
> beginning is likely the better optimization. It's basically free when
> it's not necessary, and it avoids an extra fault (and extra
> lock/unlock and retry) when it does end up faulting pages in.

It may also cause the read in to happen in the background whilst write_begin
is being done.

David

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 06:55:33PM +0100, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > End result: doing the fault_in_readable "unnecessarily" at the
> > beginning is likely the better optimization. It's basically free when
> > it's not necessary, and it avoids an extra fault (and extra
> > lock/unlock and retry) when it does end up faulting pages in.
> 
> It may also cause the read in to happen in the background whilst write_begin
> is being done.

Huh?  Last I checked, the fault_in_readable actually read a byte from
the page.  It has to wait for the read to complete before that can
happen.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 11:05 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> Huh?  Last I checked, the fault_in_readable actually read a byte from
> the page.  It has to wait for the read to complete before that can
> happen.

Yeah, we don't have any kind of async fault-in model.

I'm not sure how that would even look. I don't think it would
necessarily be *impossible* (special marker in the exception table to
let the fault code know that this is a "prepare" fault), but it would
be pretty challenging.

            Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> > It may also cause the read in to happen in the background whilst write_begin
> > is being done.
> 
> Huh?  Last I checked, the fault_in_readable actually read a byte from
> the page.  It has to wait for the read to complete before that can
> happen.

Ah, good point.

David

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Nadav Amit]

> On Jun 22, 2021, at 11:07 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Tue, Jun 22, 2021 at 11:05 AM Matthew Wilcox <willy@infradead.org> wrote:
>> 
>> Huh?  Last I checked, the fault_in_readable actually read a byte from
>> the page.  It has to wait for the read to complete before that can
>> happen.
> 
> Yeah, we don't have any kind of async fault-in model.
> 
> I'm not sure how that would even look. I don't think it would
> necessarily be *impossible* (special marker in the exception table to
> let the fault code know that this is a "prepare" fault), but it would
> be pretty challenging.

I did send an RFC some time ago for “prepare” fault, but it the RFC
requires some more work.

https://lore.kernel.org/lkml/20210225072910.2811795-1-namit@vmware.com/

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 11:07:56AM -0700, Linus Torvalds wrote:
> On Tue, Jun 22, 2021 at 11:05 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > Huh?  Last I checked, the fault_in_readable actually read a byte from
> > the page.  It has to wait for the read to complete before that can
> > happen.
> 
> Yeah, we don't have any kind of async fault-in model.
> 
> I'm not sure how that would even look. I don't think it would
> necessarily be *impossible* (special marker in the exception table to
> let the fault code know that this is a "prepare" fault), but it would
> be pretty challenging.

It wouldn't be _that_ bad necessarily.  filemap_fault:

        page = find_get_page(mapping, offset);
...
        } else if (!page) {
                fpin = do_sync_mmap_readahead(vmf);

... and we could return at that point if the flag was set.  There'd be
some more details to fill in (if there's a !uptodate page in the page
cache, don't wait for it), but it might not be too bad.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I'm not sure how that would even look. I don't think it would
> necessarily be *impossible* (special marker in the exception table to
> let the fault code know that this is a "prepare" fault), but it would
> be pretty challenging.

Probably the most obvious way would be to set a flag in task_struct saying
what you're doing and have the point that would otherwise wait for the page to
become unlocked skip to the fault fixup code if the page is locked after
->readahead() has been invoked and the flag is set, then use get_user() in
iov_iter_fault_in_readable().

But, as Willy says, there's a reasonable chance that the source page is
present anyway (presumably you want to write out data you've just constructed
or modified), in which case it's probably not worth the complexity.

David

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 11:23 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> It wouldn't be _that_ bad necessarily.  filemap_fault:

It's not actually the mm code that is the biggest problem. We
obviously already have readahead support.

It's the *fault* side.

In particular, since the fault would return without actually filling
in the page table entry (because the page isn't ready yet, and you
cannot expose it to other threads!), you also have to jump over the
instruction that caused this all.

Doable? Oh, absolutely. But you basically need a new inline asm thing
for each architecture, so that the architecture knows how to skip the
instruction that caused the page fault, and the new exception table
entry type to say 'this is that magic VM prefetch instruction".

So filemap_fault() is the least of the problems.

Honestly, it doesn't look _complicated_, but it does look like a fair
number of small details..

               Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 11:23 AM David Howells <dhowells@redhat.com> wrote:
>
> Probably the most obvious way would be to set a flag in task_struct saying
> what you're doing and have the point that would otherwise wait for the page to
> become unlocked skip to the fault fixup code if the page is locked after
> ->readahead() has been invoked and the flag is set, then use get_user() in
> iov_iter_fault_in_readable().

Yeah, the existing user access exception handling code _almost_
handles it, except for one thing: you'd need to have some way to
distinguish between "prefetch successful" and "fault failed".

And yeah, I guess it could be a flag in task_struct, but at that point
my gag reflex starts acting up.

               Linus

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 11:28:30AM -0700, Linus Torvalds wrote:
> On Tue, Jun 22, 2021 at 11:23 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > It wouldn't be _that_ bad necessarily.  filemap_fault:
> 
> It's not actually the mm code that is the biggest problem. We
> obviously already have readahead support.
> 
> It's the *fault* side.
> 
> In particular, since the fault would return without actually filling
> in the page table entry (because the page isn't ready yet, and you
> cannot expose it to other threads!), you also have to jump over the
> instruction that caused this all.

Oh, I was assuming that it'd be a function call like
get_user_pages_fast(), not an instruction that was specially marked to
be jumped over.  Gag reflex diminishing now?

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Nadav Amit]

> On Jun 22, 2021, at 11:36 AM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Tue, Jun 22, 2021 at 11:28:30AM -0700, Linus Torvalds wrote:
>> On Tue, Jun 22, 2021 at 11:23 AM Matthew Wilcox <willy@infradead.org> wrote:
>>> 
>>> It wouldn't be _that_ bad necessarily.  filemap_fault:
>> 
>> It's not actually the mm code that is the biggest problem. We
>> obviously already have readahead support.
>> 
>> It's the *fault* side.
>> 
>> In particular, since the fault would return without actually filling
>> in the page table entry (because the page isn't ready yet, and you
>> cannot expose it to other threads!), you also have to jump over the
>> instruction that caused this all.
> 
> Oh, I was assuming that it'd be a function call like
> get_user_pages_fast(), not an instruction that was specially marked to
> be jumped over.  Gag reflex diminishing now?

Just reminding the alternative (in the RFC that I mentioned before):
a vDSO exception table entry for a memory accessing function in the
vDSO. It then behaves as a sort of MADV_WILLNEED for the faulting
page if an exception is triggered. Unlike MADV_WILLNEED it maps the
page if no IO is needed. It can return through a register whether
the page was present or not.

I once implemented (another) alternative, in which the ELF had a section
with an exception-table (holding all the “Async-#PF” instructions),
which described where to skip to if a #PF occurs, but this solution
seemed too heavy-weight/intrusive.

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Linus Torvalds]

On Tue, Jun 22, 2021 at 11:51 AM Nadav Amit <nadav.amit@gmail.com> wrote:
> Just reminding the alternative (in the RFC that I mentioned before):
> a vDSO exception table entry for a memory accessing function in the
> vDSO. It then behaves as a sort of MADV_WILLNEED for the faulting
> page if an exception is triggered. Unlike MADV_WILLNEED it maps the
> page if no IO is needed. It can return through a register whether
> the page was present or not.

Yeah, that looks like a user-space equivalent.

And thanks to the vdso, it doesn't need to support all architectures.
Unlike a kernel model would (but yes, a kernel model could then have a
fallback for the non-prefetching synchronous case instead, so I guess
we could just do one architecture at a time).

               Linus

RE: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Laight]

From: David Howells
> Sent: 22 June 2021 17:27
> 
> Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> > On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> >
> > > and wondering if the iov_iter_fault_in_readable() is actually effective.
> > > Yes, it can make sure that the page we're intending to modify is dragged
> > > into the pagecache and marked uptodate so that it can be read from, but is
> > > it possible for the page to then get reclaimed before we get to
> > > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially
> > > take a long time, say if it has to go and get a lock/lease from a server.
> >
> > Yes, it is.  So what?  We'll just retry.  You *can't* take faults while
> > holding some pages locked; not without shitloads of deadlocks.
> 
> In that case, can we amend the comment immediately above
> iov_iter_fault_in_readable()?
> 
> 	/*
> 	 * 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))) {
> 
> The first part suggests this is for deadlock avoidance.  If that's not true,
> then this should perhaps be changed.

I'd say something like:
	/*
	 * The actual copy_from_user() is done with a lock held
	 * so cannot fault in missing pages.
	 * So fault in the pages first.
	 * If they get paged out the inatomic usercopy will fail
	 * and the whole operation is retried.
	 *
	 * Hopefully there are enough memory pages available to
	 * stop this looping forever.
	 */

It is perfectly possible for another application thread to
invalidate one of the buffer fragments after iov_iter_fault_in_readable()
return success - so it will then fail on the second pass.

The maximum number of pages required is twice the maximum number
of iov fragments.
If the system is crawling along with no available memory pages
the same physical page could get used for two user pages.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Matthew Wilcox]

On Tue, Jun 22, 2021 at 09:55:09PM +0000, David Laight wrote:
> From: David Howells
> > Sent: 22 June 2021 17:27
> > 
> > Al Viro <viro@zeniv.linux.org.uk> wrote:
> > 
> > > On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> > >
> > > > and wondering if the iov_iter_fault_in_readable() is actually effective.
> > > > Yes, it can make sure that the page we're intending to modify is dragged
> > > > into the pagecache and marked uptodate so that it can be read from, but is
> > > > it possible for the page to then get reclaimed before we get to
> > > > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially
> > > > take a long time, say if it has to go and get a lock/lease from a server.
> > >
> > > Yes, it is.  So what?  We'll just retry.  You *can't* take faults while
> > > holding some pages locked; not without shitloads of deadlocks.
> > 
> > In that case, can we amend the comment immediately above
> > iov_iter_fault_in_readable()?
> > 
> > 	/*
> > 	 * 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))) {
> > 
> > The first part suggests this is for deadlock avoidance.  If that's not true,
> > then this should perhaps be changed.
> 
> I'd say something like:
> 	/*
> 	 * The actual copy_from_user() is done with a lock held
> 	 * so cannot fault in missing pages.
> 	 * So fault in the pages first.
> 	 * If they get paged out the inatomic usercopy will fail
> 	 * and the whole operation is retried.
> 	 *
> 	 * Hopefully there are enough memory pages available to
> 	 * stop this looping forever.
> 	 */
> 
> It is perfectly possible for another application thread to
> invalidate one of the buffer fragments after iov_iter_fault_in_readable()
> return success - so it will then fail on the second pass.
> 
> The maximum number of pages required is twice the maximum number
> of iov fragments.
> If the system is crawling along with no available memory pages
> the same physical page could get used for two user pages.

I would suggest reading the function before you suggest modifications
to it.

                offset = (pos & (PAGE_SIZE - 1));
                bytes = min_t(unsigned long, PAGE_SIZE - offset,
                                                iov_iter_count(i));

Re: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[Dave Chinner]

On Tue, Jun 22, 2021 at 09:55:09PM +0000, David Laight wrote:
> From: David Howells
> > Sent: 22 June 2021 17:27
> > 
> > Al Viro <viro@zeniv.linux.org.uk> wrote:
> > 
> > > On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> > >
> > > > and wondering if the iov_iter_fault_in_readable() is actually effective.
> > > > Yes, it can make sure that the page we're intending to modify is dragged
> > > > into the pagecache and marked uptodate so that it can be read from, but is
> > > > it possible for the page to then get reclaimed before we get to
> > > > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially
> > > > take a long time, say if it has to go and get a lock/lease from a server.
> > >
> > > Yes, it is.  So what?  We'll just retry.  You *can't* take faults while
> > > holding some pages locked; not without shitloads of deadlocks.
> > 
> > In that case, can we amend the comment immediately above
> > iov_iter_fault_in_readable()?
> > 
> > 	/*
> > 	 * 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))) {
> > 
> > The first part suggests this is for deadlock avoidance.  If that's not true,
> > then this should perhaps be changed.
> 
> I'd say something like:
> 	/*
> 	 * The actual copy_from_user() is done with a lock held
> 	 * so cannot fault in missing pages.
> 	 * So fault in the pages first.
> 	 * If they get paged out the inatomic usercopy will fail
> 	 * and the whole operation is retried.
> 	 *
> 	 * Hopefully there are enough memory pages available to
> 	 * stop this looping forever.
> 	 */

What about the other 4 or 5 copies of this loop in the kernel?

This is a pattern, not a one off implementation. Comments describing
how the pattern works belong in the API documentation, not on a
single implemenation of the pattern...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: Do we need to unrevert "fs: do not prefault sys_write() user buffer pages"?

[David Laight]

From: Matthew Wilcox
> Sent: 22 June 2021 23:04
> 
> On Tue, Jun 22, 2021 at 09:55:09PM +0000, David Laight wrote:
> > From: David Howells
> > > Sent: 22 June 2021 17:27
> > >
> > > Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > > On Tue, Jun 22, 2021 at 04:20:40PM +0100, David Howells wrote:
> > > >
> > > > > and wondering if the iov_iter_fault_in_readable() is actually effective.
> > > > > Yes, it can make sure that the page we're intending to modify is dragged
> > > > > into the pagecache and marked uptodate so that it can be read from, but is
> > > > > it possible for the page to then get reclaimed before we get to
> > > > > iov_iter_copy_from_user_atomic()?  a_ops->write_begin() could potentially
> > > > > take a long time, say if it has to go and get a lock/lease from a server.
> > > >
> > > > Yes, it is.  So what?  We'll just retry.  You *can't* take faults while
> > > > holding some pages locked; not without shitloads of deadlocks.
> > >
> > > In that case, can we amend the comment immediately above
> > > iov_iter_fault_in_readable()?
> > >
> > > 	/*
> > > 	 * 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))) {
> > >
> > > The first part suggests this is for deadlock avoidance.  If that's not true,
> > > then this should perhaps be changed.
> >
> > I'd say something like:
> > 	/*
> > 	 * The actual copy_from_user() is done with a lock held
> > 	 * so cannot fault in missing pages.
> > 	 * So fault in the pages first.
> > 	 * If they get paged out the inatomic usercopy will fail
> > 	 * and the whole operation is retried.
> > 	 *
> > 	 * Hopefully there are enough memory pages available to
> > 	 * stop this looping forever.
> > 	 */
> >
> > It is perfectly possible for another application thread to
> > invalidate one of the buffer fragments after iov_iter_fault_in_readable()
> > return success - so it will then fail on the second pass.
> >
> > The maximum number of pages required is twice the maximum number
> > of iov fragments.
> > If the system is crawling along with no available memory pages
> > the same physical page could get used for two user pages.
> 
> I would suggest reading the function before you suggest modifications
> to it.
> 
>                 offset = (pos & (PAGE_SIZE - 1));
>                 bytes = min_t(unsigned long, PAGE_SIZE - offset,
>                                                 iov_iter_count(i));

Right the transfer size is limited to PAGE_SIZE.
But the user supplied iov[] could be a lot of 2 byte buffers
all with base addresses (PAGE_SIZE * n - 1).
So you might need two pages for each iov[] fragment.
And you need not to recycle the earlier ones.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)