On Sat, Oct 31, 2020 at 05:06:46PM +0000, Matthew Wilcox wrote:
> > +static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
> > + struct page *page, pgoff_t pg_index, bool first)
>
> I prefer "filemap_update_page".
That has the advantage of being shorter, while I find it less descriptive.
I can updated it and add a comment explaining what it does, as that is
probably warranted anyway.
> I don't understand why you pass in pg_index instead of using page->index.
> We dereferenced the page pointer already to check PageReadahead(), so
> there's no performance issue here.
Yes, we should do that.
> Also, if filemap_find_get_pages() stops on the first !Uptodate or
> Readahead page, as I had it in my patchset, then we don't need the loop
> at all -- filemap_read_pages() looks like:
>
> nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
> if (!nr_pages) {
> pages[0] = filemap_create_page(iocb, iter);
> if (!IS_ERR_OR_NULL(pages[0]))
> return 1;
> if (!pages[0])
> goto retry;
> return PTR_ERR(pages[0]);
> }
>
> page = pages[nr_pages - 1];
> if (PageUptodate(page) && !PageReadahead(page))
> return nr_pages;
> err = filemap_update_page(iocb, iter, page);
> if (!err)
> return nr_pages;
> nr_pages -= 1;
> if (nr_pages)
> return nr_pages;
> return err;
This looks sensible, but goes beyond the simple refactoring I had
intended. Let me take a more detailed look at your series (I had just
updated my existing series to to the latest linux-next) and see how
it can nicely fit in.
