From: Andrew Morton <akpm@linux-foundation.org>
To: Matthew Wilcox <willy@infradead.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 12:12:53 -0800 [thread overview]
Message-ID: <20210303121253.9f44d8129f148b1e2e78cc81@linux-foundation.org> (raw)
In-Reply-To: <20210303132640.GB2723601@casper.infradead.org>
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.
next prev parent 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 [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 [this message]
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=20210303121253.9f44d8129f148b1e2e78cc81@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=hch@lst.de \
--cc=kent.overstreet@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=willy@infradead.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.