Re: [PATCH v2 6/9] iomap,xfs: Convert from readpages to readahead

[Christoph Hellwig <hch@infradead.org>]

On Tue, Jan 14, 2020 at 06:38:40PM -0800, Matthew Wilcox wrote:
>  static loff_t
> +iomap_readahead_actor(struct inode *inode, loff_t pos, loff_t length,
>  		void *data, struct iomap *iomap, struct iomap *srcmap)
>  {
>  	struct iomap_readpage_ctx *ctx = data;
> @@ -410,10 +381,8 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length,
>  			ctx->cur_page = NULL;
>  		}
>  		if (!ctx->cur_page) {
> -			ctx->cur_page = iomap_next_page(inode, ctx->pages,
> -					pos, length, &done);
> -			if (!ctx->cur_page)
> -				break;
> +			ctx->cur_page = readahead_page(inode->i_mapping,
> +					pos / PAGE_SIZE);

Don't we at least need a sanity check for a NULL cur_page here?
Also the readahead_page version in your previous patch seems to expect
a byte offset, so the division above would not be required. (and should
probably be replaced with a right shift anyway no matter where it ends
up)

> +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;

Any good reason not to pass byte offsets for start and length?

> +	return length / PAGE_SIZE;

Same for the return value?

For the file systems that would usually be a more natural interface than
a page index and number of pages.