All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
	Kent Overstreet <kent.overstreet@gmail.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault
Date: Wed, 3 Mar 2021 13:26:40 +0000	[thread overview]
Message-ID: <20210303132640.GB2723601@casper.infradead.org> (raw)
In-Reply-To: <20210302220735.1f150f28323f676d2955ab49@linux-foundation.org>

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 ...

  reply	other threads:[~2021-03-04  0:21 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-26 14:00 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 [this message]
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

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20210303132640.GB2723601@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=hch@lst.de \
    --cc=kent.overstreet@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --subject='Re: [PATCH] mm/filemap: Use filemap_read_page in filemap_fault' \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.