From: Matthew Wilcox <willy@infradead.org>
To: Gao Xiang <hsiangkao@aol.com>
Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
linux-erofs@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 07/12] erofs: Convert uncompressed files from readpages to readahead
Date: Sat, 25 Jan 2020 11:09:51 -0800 [thread overview]
Message-ID: <20200125190951.GN4675@bombadil.infradead.org> (raw)
In-Reply-To: <20200125015323.GA9918@hsiangkao-HP-ZHAN-66-Pro-G1>
On Sat, Jan 25, 2020 at 09:53:29AM +0800, Gao Xiang wrote:
> > + /* all the page errors are ignored when readahead */
> > + if (IS_ERR(bio)) {
> > + pr_err("%s, readahead error at page %lu of nid %llu\n",
> > + __func__, page->index,
> > + EROFS_I(mapping->host)->nid);
> >
> > - bio = NULL;
> > - }
> > + bio = NULL;
> > + put_page(page);
>
> Out of curiously, some little question... Why we need put_page(page) twice
> if erofs_read_raw_page returns with error...
>
> One put_page(page) is used as a temporary reference count for this request,
> we could put_page(page) in advance since pages are still locked before endio.
>
> Another put_page(page) is used for page cache xarray. I think in this case
> the page has been successfully inserted to the page cache anyway, after erroring
> out it will trigger .readpage again... so probably we need to keep this
> refcount count for page cache xarray?
>
> If I'm missing something, kindly correct me if I'm wrong....
You're quite right. After readahead has completed, the page should have
a refcount of 1 and be unlocked. If we hit an error, the page should
be !uptodate. It doesn't matter whether we set PageError or not in this
case; filemap_fault() will ClearPageError() before retrying if the page
is !uptodate. This extra put_page() is wrong, and I'll remove it from
the next version. Thanks!
next prev parent reply other threads:[~2020-01-25 19:10 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-25 1:35 [PATCH 00/12] Change readahead API Matthew Wilcox
2020-01-25 1:35 ` [PATCH 03/12] readahead: Put pages in cache earlier Matthew Wilcox
2020-01-25 19:44 ` Matthew Wilcox
2020-01-25 1:35 ` [PATCH 04/12] mm: Add readahead address space operation Matthew Wilcox
2020-01-25 3:57 ` Randy Dunlap
2020-02-01 0:25 ` Matthew Wilcox
2020-01-29 0:24 ` Dave Chinner
2020-01-30 8:00 ` Matthew Wilcox
2020-01-25 1:35 ` [PATCH 07/12] erofs: Convert uncompressed files from readpages to readahead Matthew Wilcox
2020-01-25 1:53 ` Gao Xiang via Linux-erofs
2020-01-25 19:09 ` Matthew Wilcox [this message]
2020-01-29 0:57 ` Dave Chinner
2020-01-30 8:10 ` Matthew Wilcox
2020-01-25 1:35 ` [PATCH 08/12] erofs: Convert compressed " Matthew Wilcox
2020-02-13 4:38 ` [PATCH 00/12] Change readahead API Andrew Morton
2020-02-13 13:43 ` 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=20200125190951.GN4675@bombadil.infradead.org \
--to=willy@infradead.org \
--cc=hsiangkao@aol.com \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).