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.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Cc: linux-xfs@xxxxxxxxxxxxxxx
....
> +unsigned
> +iomap_readahead(struct address_space *mapping, pgoff_t start,
> unsigned nr_pages, const struct iomap_ops *ops)
> {
> struct iomap_readpage_ctx ctx = {
> - .pages = pages,
> .is_readahead = true,
> };
> - loff_t pos = page_offset(list_entry(pages->prev, struct page, lru));
> - loff_t last = page_offset(list_entry(pages->next, struct page, lru));
> - loff_t length = last - pos + PAGE_SIZE, ret = 0;
> + loff_t pos = start * PAGE_SIZE;
> + loff_t length = nr_pages * PAGE_SIZE;
>
> - trace_iomap_readpages(mapping->host, nr_pages);
> + trace_iomap_readahead(mapping->host, nr_pages);
>
> while (length > 0) {
> - ret = iomap_apply(mapping->host, pos, length, 0, ops,
> - &ctx, iomap_readpages_actor);
> + loff_t ret = iomap_apply(mapping->host, pos, length, 0, ops,
> + &ctx, iomap_readahead_actor);
> if (ret <= 0) {
> WARN_ON_ONCE(ret == 0);
> - goto done;
> + break;
> }
> pos += ret;
> length -= ret;
> }
> - ret = 0;
> -done:
> +
> if (ctx.bio)
> submit_bio(ctx.bio);
> - if (ctx.cur_page) {
> - if (!ctx.cur_page_in_bio)
> - unlock_page(ctx.cur_page);
> + 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.
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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
