Re: [PATCH 14/17] mm/filemap: Restructure filemap_get_pages

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 03, 2020 at 02:46:19PM +0000, Matthew Wilcox wrote:
> On Tue, Nov 03, 2020 at 08:57:36AM +0100, Christoph Hellwig wrote:
> > On Mon, Nov 02, 2020 at 06:43:09PM +0000, Matthew Wilcox (Oracle) wrote:
> > > Avoid a goto, and by the time we get to calling filemap_update_page(),
> > > we definitely have at least one page.
> > 
> > I find the error handling flow hard to follow and the existing but
> > heavily touched naming of the nr_got variable and the find_pages label
> > not helpful.  I'd do the following on top of this patch:
> 
> I've removed nr_got entirely in my current tree ...

Even better.  The pagevec usage looks pretty nice!

> +static void mapping_get_read_thps(struct address_space *mapping,

Maybe call this pagevec_get_read_thps?

> +	pgoff_t maxindex = DIV_ROUND_UP(iocb->ki_pos + iter->count, PAGE_SIZE);
>  	struct page *page;
> +	int err = 0;
>  
>  find_page:
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
>  
> +	pagevec_init(pvec);
> +	mapping_get_read_thps(mapping, index, maxindex, pvec);
> +	if (!pagevec_count(pvec)) {
>  		if (iocb->ki_flags & IOCB_NOIO)
>  			return -EAGAIN;
>  		page_cache_sync_readahead(mapping, ra, filp, index,
> +				maxindex - index);
> +		mapping_get_read_thps(mapping, index, maxindex, pvec);
>  	}
> +	if (!pagevec_count(pvec)) {
>  		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
>  			return -EAGAIN;
> +		page = filemap_create_page(filp, mapping,
>  				iocb->ki_pos >> PAGE_SHIFT);
> +		if (!page)
>  			goto find_page;
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +		pagevec_add(pvec, page);
> +		return 0;

I'd pass the pagevec to filemap_create_page and just return an errno,
which should be a little easier to follow.

> -	page = pages[nr_got - 1];
> +	page = pvec->pages[pagevec_count(pvec) - 1];

I wonder if a pagevec_last_page() helper would be neat for things
like this? (might be a bit of over engineering..)