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!
