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..)
