Re: [PATCH 12/12] iomap: Convert from readpages to readahead

From: Matthew Wilcox
Date: Fri Jan 31 2020 - 04:44:20 EST


On Wed, Jan 29, 2020 at 12:38:39PM +1100, Dave Chinner wrote:
> On Fri, Jan 24, 2020 at 05:35:53PM -0800, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
> > Use the new readahead operation in XFS and iomap.
> > + if (ctx.cur_page && ctx.cur_page_in_bio)
> > put_page(ctx.cur_page);
> > - }
> >
> > - /*
> > - * Check that we didn't lose a page due to the arcance calling
> > - * conventions..
> > - */
> > - WARN_ON_ONCE(!ret && !list_empty(ctx.pages));
> > - return ret;
> > + return length / PAGE_SIZE;
>
> Took me quite some time to get my head around whether this was
> correct or not.

Yes. Unfortunately, this is the most complex of the conversions ;-(

> I'm still not certain in the cases where block size != page size and
> we've got an extent boundary in the middle of the page and had a
> read error on the second extent in the page. In this case,
> ctx.cur_page_in_bio is true so we drop the readahead reference to
> the page. Also, length is not a multiple of page size, and so the
> nr_pages value returned includes the partial page that we have IO
> underway on.
>
> That, I think, leads to both a double unlock and a double put_page()
> of the partial page in question.

But C division rounds down. So we neither unlock, nor put_page() the
page which was in the bio ... do we?