All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
@ 2021-02-26 14:00 Matthew Wilcox (Oracle)
  2021-03-03  1:30 ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-02-26 14:00 UTC (permalink / raw)
  To: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig,
	Andrew Morton
  Cc: Matthew Wilcox (Oracle)

After splitting generic_file_buffered_read() into smaller parts, it
turns out we can reuse one of the parts in filemap_fault().  This fixes
an oversight -- waiting for the I/O to complete is now interruptible
by a fatal signal.  And it saves us a few bytes of text in an unlikely
path.

$ ./scripts/bloat-o-meter before.o after.o
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-207 (-207)
Function                                     old     new   delta
filemap_fault                               2187    1980    -207
Total: Before=37491, After=37284, chg -0.55%

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/filemap.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 46a8b9e82434..f7ab86d13692 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2748,7 +2748,6 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	struct file *file = vmf->vma->vm_file;
 	struct file *fpin = NULL;
 	struct address_space *mapping = file->f_mapping;
-	struct file_ra_state *ra = &file->f_ra;
 	struct inode *inode = mapping->host;
 	pgoff_t offset = vmf->pgoff;
 	pgoff_t max_off;
@@ -2835,14 +2834,8 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	 * because there really aren't any performance issues here
 	 * and we need to check for errors.
 	 */
-	ClearPageError(page);
 	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
-	error = mapping->a_ops->readpage(file, page);
-	if (!error) {
-		wait_on_page_locked(page);
-		if (!PageUptodate(page))
-			error = -EIO;
-	}
+	error = filemap_read_page(file, mapping, page);
 	if (fpin)
 		goto out_retry;
 	put_page(page);
@@ -2850,7 +2843,6 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	if (!error || error == AOP_TRUNCATED_PAGE)
 		goto retry_find;
 
-	shrink_readahead_size_eio(ra);
 	return VM_FAULT_SIGBUS;
 
 out_retry:
-- 
2.30.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-02-26 14:00 [PATCH] mm/filemap: Use filemap_read_page in filemap_fault Matthew Wilcox (Oracle)
@ 2021-03-03  1:30 ` Andrew Morton
  2021-03-03  1:33   ` Matthew Wilcox
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-03-03  1:30 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle)
  Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Fri, 26 Feb 2021 14:00:11 +0000 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:

> After splitting generic_file_buffered_read() into smaller parts, it
> turns out we can reuse one of the parts in filemap_fault().  This fixes
> an oversight -- waiting for the I/O to complete is now interruptible
> by a fatal signal.  And it saves us a few bytes of text in an unlikely
> path.

We also handle AOP_TRUNCATED_PAGE which the present code fails to do. 
Should this be in the changelog?

Did we handle AOP_TRUNCATED_PAGE in the pre-splitup code, or is this new?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03  1:30 ` Andrew Morton
@ 2021-03-03  1:33   ` Matthew Wilcox
  2021-03-03  6:07     ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2021-03-03  1:33 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Tue, Mar 02, 2021 at 05:30:39PM -0800, Andrew Morton wrote:
> On Fri, 26 Feb 2021 14:00:11 +0000 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
> 
> > After splitting generic_file_buffered_read() into smaller parts, it
> > turns out we can reuse one of the parts in filemap_fault().  This fixes
> > an oversight -- waiting for the I/O to complete is now interruptible
> > by a fatal signal.  And it saves us a few bytes of text in an unlikely
> > path.
> 
> We also handle AOP_TRUNCATED_PAGE which the present code fails to do. 
> Should this be in the changelog?

No, the present code does handle AOP_TRUNCATED_PAGE.  It's perhaps not
the clearest in the diff, but it's there.  Here's git show -U5:

-       ClearPageError(page);
        fpin = maybe_unlock_mmap_for_io(vmf, fpin);
-       error = mapping->a_ops->readpage(file, page);
-       if (!error) {
-               wait_on_page_locked(page);
-               if (!PageUptodate(page))
-                       error = -EIO;
-       }
+       error = filemap_read_page(file, mapping, page);
        if (fpin)
                goto out_retry;
        put_page(page);
 
        if (!error || error == AOP_TRUNCATED_PAGE)
                goto retry_find;
 
-       shrink_readahead_size_eio(ra);
        return VM_FAULT_SIGBUS;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03  1:33   ` Matthew Wilcox
@ 2021-03-03  6:07     ` Andrew Morton
  2021-03-03 13:26       ` Matthew Wilcox
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-03-03  6:07 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Wed, 3 Mar 2021 01:33:13 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Mar 02, 2021 at 05:30:39PM -0800, Andrew Morton wrote:
> > On Fri, 26 Feb 2021 14:00:11 +0000 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
> > 
> > > After splitting generic_file_buffered_read() into smaller parts, it
> > > turns out we can reuse one of the parts in filemap_fault().  This fixes
> > > an oversight -- waiting for the I/O to complete is now interruptible
> > > by a fatal signal.  And it saves us a few bytes of text in an unlikely
> > > path.
> > 
> > We also handle AOP_TRUNCATED_PAGE which the present code fails to do. 
> > Should this be in the changelog?
> 
> No, the present code does handle AOP_TRUNCATED_PAGE.  It's perhaps not
> the clearest in the diff, but it's there.  Here's git show -U5:
> 
> -       ClearPageError(page);
>         fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> -       error = mapping->a_ops->readpage(file, page);
> -       if (!error) {
> -               wait_on_page_locked(page);
> -               if (!PageUptodate(page))
> -                       error = -EIO;
> -       }
> +       error = filemap_read_page(file, mapping, page);
>         if (fpin)
>                 goto out_retry;
>         put_page(page);
>  
>         if (!error || error == AOP_TRUNCATED_PAGE)
>                 goto retry_find;
>  
> -       shrink_readahead_size_eio(ra);
>         return VM_FAULT_SIGBUS;

But ->readpage() doesn't check for !mapping (does it?).  So the
->readpage() cannot return AOP_TRUNCATED_PAGE.

However filemap_read_page() does check for !mapping.  So current -linus
doesn't check for !mapping, and post-willy does do this?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03  6:07     ` Andrew Morton
@ 2021-03-03 13:26       ` Matthew Wilcox
  2021-03-03 20:12         ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2021-03-03 13:26 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Tue, Mar 02, 2021 at 10:07:35PM -0800, Andrew Morton wrote:
> > > We also handle AOP_TRUNCATED_PAGE which the present code fails to do. 
> > > Should this be in the changelog?
> > 
> > No, the present code does handle AOP_TRUNCATED_PAGE.  It's perhaps not
> > the clearest in the diff, but it's there.  Here's git show -U5:
> > 
> > -       ClearPageError(page);
> >         fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> > -       error = mapping->a_ops->readpage(file, page);
> > -       if (!error) {
> > -               wait_on_page_locked(page);
> > -               if (!PageUptodate(page))
> > -                       error = -EIO;
> > -       }
> > +       error = filemap_read_page(file, mapping, page);
> >         if (fpin)
> >                 goto out_retry;
> >         put_page(page);
> >  
> >         if (!error || error == AOP_TRUNCATED_PAGE)
> >                 goto retry_find;
> >  
> > -       shrink_readahead_size_eio(ra);
> >         return VM_FAULT_SIGBUS;
> 
> But ->readpage() doesn't check for !mapping (does it?).  So the
> ->readpage() cannot return AOP_TRUNCATED_PAGE.

Filesystems can return AOP_TRUNCATED_PAGE from ->readpage.  I think
ocfs2 is the only one to do so currently.

> However filemap_read_page() does check for !mapping.  So current -linus
> doesn't check for !mapping, and post-willy does do this?

Oh!  I see what you mean now.  I can't come up with a sequence of events
where this check is going to do anything useful.  It may be left over from
earlier times.  Here's an earlier version of generic_file_buffered_read()
before Kent refactored it:

                /* Start the actual read. The read will unlock the page. */
                error = mapping->a_ops->readpage(filp, page);
[...]
                if (!PageUptodate(page)) {
                        error = lock_page_killable(page);
                        if (unlikely(error))
                                goto readpage_error;
                        if (!PageUptodate(page)) {
                                if (page->mapping == NULL) {
                                        /*
                                         * invalidate_mapping_pages got it
                                         */
                                        unlock_page(page);
                                        put_page(page);


But here's the thing ... invalidate_mapping_pages() doesn't
ClearPageUptodate.  The only places where we ClearPageUptodate is on an
I/O error.

So ... as far as I can tell, the only way to hit this is:

 - Get an I/O error during the wait
 - Have another thread cause the page to be removed from the page cache
   (eg do direct I/O to the file) before this thread is run.

and the consequence to this change is that we have another attempt to
read the page instead of returning an error immediately.  I'm OK with
that unintentional change, although I think the previous behaviour was
also perfectly acceptable (after all, there was an I/O error while trying
to read this page).

Delving into the linux-fullhistory tree, this code was introduced by ...

commit 56f0d5fe6851037214a041a5cb4fc66199256544
Author: Andrew Morton <akpm@osdl.org>
Date:   Fri Jan 7 22:03:01 2005 -0800

    [PATCH] readpage-vs-invalidate fix

    A while ago we merged a patch which tried to solve a problem wherein a
    concurrent read() and invalidate_inode_pages() would cause the read() to
    return -EIO because invalidate cleared PageUptodate() at the wrong time.

We no longer clear PageUptodate, so I think this is stale code?  Perhaps
you could check with the original author ...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03 13:26       ` Matthew Wilcox
@ 2021-03-03 20:12         ` Andrew Morton
  2021-03-03 20:57           ` Matthew Wilcox
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-03-03 20:12 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Wed, 3 Mar 2021 13:26:40 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> But here's the thing ... invalidate_mapping_pages() doesn't
> ClearPageUptodate.  The only places where we ClearPageUptodate is on an
> I/O error.

yup.

> So ... as far as I can tell, the only way to hit this is:
> 
>  - Get an I/O error during the wait
>  - Have another thread cause the page to be removed from the page cache
>    (eg do direct I/O to the file) before this thread is run.
> 
> and the consequence to this change is that we have another attempt to
> read the page instead of returning an error immediately.  I'm OK with
> that unintentional change, although I think the previous behaviour was
> also perfectly acceptable (after all, there was an I/O error while trying
> to read this page).
> 
> Delving into the linux-fullhistory tree, this code was introduced by ...
> 
> commit 56f0d5fe6851037214a041a5cb4fc66199256544
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Fri Jan 7 22:03:01 2005 -0800
> 
>     [PATCH] readpage-vs-invalidate fix
> 
>     A while ago we merged a patch which tried to solve a problem wherein a
>     concurrent read() and invalidate_inode_pages() would cause the read() to
>     return -EIO because invalidate cleared PageUptodate() at the wrong time.
> 
> We no longer clear PageUptodate, so I think this is stale code?  Perhaps
> you could check with the original author ...

Which code do you think might be stale?  We need the !PageUptodate
check to catch IO errors and we need the !page->mapping check to catch
invalidates.  Am a bit confused.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03 20:12         ` Andrew Morton
@ 2021-03-03 20:57           ` Matthew Wilcox
  2021-03-03 21:51             ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2021-03-03 20:57 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Wed, Mar 03, 2021 at 12:12:53PM -0800, Andrew Morton wrote:
> On Wed, 3 Mar 2021 13:26:40 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > But here's the thing ... invalidate_mapping_pages() doesn't
> > ClearPageUptodate.  The only places where we ClearPageUptodate is on an
> > I/O error.
> 
> yup.
> 
> > So ... as far as I can tell, the only way to hit this is:
> > 
> >  - Get an I/O error during the wait
> >  - Have another thread cause the page to be removed from the page cache
> >    (eg do direct I/O to the file) before this thread is run.
> > 
> > and the consequence to this change is that we have another attempt to
> > read the page instead of returning an error immediately.  I'm OK with
> > that unintentional change, although I think the previous behaviour was
> > also perfectly acceptable (after all, there was an I/O error while trying
> > to read this page).
> > 
> > Delving into the linux-fullhistory tree, this code was introduced by ...
> > 
> > commit 56f0d5fe6851037214a041a5cb4fc66199256544
> > Author: Andrew Morton <akpm@osdl.org>
> > Date:   Fri Jan 7 22:03:01 2005 -0800
> > 
> >     [PATCH] readpage-vs-invalidate fix
> > 
> >     A while ago we merged a patch which tried to solve a problem wherein a
> >     concurrent read() and invalidate_inode_pages() would cause the read() to
> >     return -EIO because invalidate cleared PageUptodate() at the wrong time.
> > 
> > We no longer clear PageUptodate, so I think this is stale code?  Perhaps
> > you could check with the original author ...
> 
> Which code do you think might be stale?  We need the !PageUptodate
> check to catch IO errors and we need the !page->mapping check to catch
> invalidates.  Am a bit confused.

I think the check of !page->mapping here:

        if (PageUptodate(page))
                return 0;
        if (!page->mapping)     /* page truncated */
                return AOP_TRUNCATED_PAGE;

is no longer needed.  If we didn't see an error, the page will be Uptodate,
regardless of whether it's been removed from the page cache.  If we did
see an error, it's OK to return -EIO, even if the page has been removed
from the page cache in the interim.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03 20:57           ` Matthew Wilcox
@ 2021-03-03 21:51             ` Andrew Morton
  2021-03-03 22:31               ` Matthew Wilcox
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-03-03 21:51 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Wed, 3 Mar 2021 20:57:36 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Mar 03, 2021 at 12:12:53PM -0800, Andrew Morton wrote:
> > On Wed, 3 Mar 2021 13:26:40 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> > 
> > > But here's the thing ... invalidate_mapping_pages() doesn't
> > > ClearPageUptodate.  The only places where we ClearPageUptodate is on an
> > > I/O error.
> > 
> > yup.
> > 
> > > So ... as far as I can tell, the only way to hit this is:
> > > 
> > >  - Get an I/O error during the wait
> > >  - Have another thread cause the page to be removed from the page cache
> > >    (eg do direct I/O to the file) before this thread is run.
> > > 
> > > and the consequence to this change is that we have another attempt to
> > > read the page instead of returning an error immediately.  I'm OK with
> > > that unintentional change, although I think the previous behaviour was
> > > also perfectly acceptable (after all, there was an I/O error while trying
> > > to read this page).
> > > 
> > > Delving into the linux-fullhistory tree, this code was introduced by ...
> > > 
> > > commit 56f0d5fe6851037214a041a5cb4fc66199256544
> > > Author: Andrew Morton <akpm@osdl.org>
> > > Date:   Fri Jan 7 22:03:01 2005 -0800
> > > 
> > >     [PATCH] readpage-vs-invalidate fix
> > > 
> > >     A while ago we merged a patch which tried to solve a problem wherein a
> > >     concurrent read() and invalidate_inode_pages() would cause the read() to
> > >     return -EIO because invalidate cleared PageUptodate() at the wrong time.
> > > 
> > > We no longer clear PageUptodate, so I think this is stale code?  Perhaps
> > > you could check with the original author ...
> > 
> > Which code do you think might be stale?  We need the !PageUptodate
> > check to catch IO errors and we need the !page->mapping check to catch
> > invalidates.  Am a bit confused.
> 
> I think the check of !page->mapping here:
> 
>         if (PageUptodate(page))
>                 return 0;
>         if (!page->mapping)     /* page truncated */
>                 return AOP_TRUNCATED_PAGE;
> 
> is no longer needed.  If we didn't see an error, the page will be Uptodate,
> regardless of whether it's been removed from the page cache.  If we did
> see an error, it's OK to return -EIO, even if the page has been removed
> from the page cache in the interim.

OK.

Checking page->mapping of an unlocked page seems meaningless anyway -
what's to prevent it from being truncated just after we checked?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
  2021-03-03 21:51             ` Andrew Morton
@ 2021-03-03 22:31               ` Matthew Wilcox
  0 siblings, 0 replies; 9+ messages in thread
From: Matthew Wilcox @ 2021-03-03 22:31 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, linux-fsdevel, Kent Overstreet, Christoph Hellwig

On Wed, Mar 03, 2021 at 01:51:42PM -0800, Andrew Morton wrote:
> On Wed, 3 Mar 2021 20:57:36 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Wed, Mar 03, 2021 at 12:12:53PM -0800, Andrew Morton wrote:
> > > On Wed, 3 Mar 2021 13:26:40 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> > > 
> > > > But here's the thing ... invalidate_mapping_pages() doesn't
> > > > ClearPageUptodate.  The only places where we ClearPageUptodate is on an
> > > > I/O error.
> > > 
> > > yup.
> > > 
> > > > So ... as far as I can tell, the only way to hit this is:
> > > > 
> > > >  - Get an I/O error during the wait
> > > >  - Have another thread cause the page to be removed from the page cache
> > > >    (eg do direct I/O to the file) before this thread is run.
> > > > 
> > > > and the consequence to this change is that we have another attempt to
> > > > read the page instead of returning an error immediately.  I'm OK with
> > > > that unintentional change, although I think the previous behaviour was
> > > > also perfectly acceptable (after all, there was an I/O error while trying
> > > > to read this page).
> > > > 
> > > > Delving into the linux-fullhistory tree, this code was introduced by ...
> > > > 
> > > > commit 56f0d5fe6851037214a041a5cb4fc66199256544
> > > > Author: Andrew Morton <akpm@osdl.org>
> > > > Date:   Fri Jan 7 22:03:01 2005 -0800
> > > > 
> > > >     [PATCH] readpage-vs-invalidate fix
> > > > 
> > > >     A while ago we merged a patch which tried to solve a problem wherein a
> > > >     concurrent read() and invalidate_inode_pages() would cause the read() to
> > > >     return -EIO because invalidate cleared PageUptodate() at the wrong time.
> > > > 
> > > > We no longer clear PageUptodate, so I think this is stale code?  Perhaps
> > > > you could check with the original author ...
> > > 
> > > Which code do you think might be stale?  We need the !PageUptodate
> > > check to catch IO errors and we need the !page->mapping check to catch
> > > invalidates.  Am a bit confused.
> > 
> > I think the check of !page->mapping here:
> > 
> >         if (PageUptodate(page))
> >                 return 0;
> >         if (!page->mapping)     /* page truncated */
> >                 return AOP_TRUNCATED_PAGE;
> > 
> > is no longer needed.  If we didn't see an error, the page will be Uptodate,
> > regardless of whether it's been removed from the page cache.  If we did
> > see an error, it's OK to return -EIO, even if the page has been removed
> > from the page cache in the interim.
> 
> OK.
> 
> Checking page->mapping of an unlocked page seems meaningless anyway -
> what's to prevent it from being truncated just after we checked?

Originally it prevented the opposite race from being misinterpreted; we
didn't care if it was truncated after we saw it was Uptodate; we cared
if it was !Uptodate because it had been truncated.  Now, it's just pointless.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-04  0:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 14:00 [PATCH] mm/filemap: Use filemap_read_page in filemap_fault Matthew Wilcox (Oracle)
2021-03-03  1:30 ` Andrew Morton
2021-03-03  1:33   ` Matthew Wilcox
2021-03-03  6:07     ` Andrew Morton
2021-03-03 13:26       ` Matthew Wilcox
2021-03-03 20:12         ` Andrew Morton
2021-03-03 20:57           ` Matthew Wilcox
2021-03-03 21:51             ` Andrew Morton
2021-03-03 22:31               ` Matthew Wilcox

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.