Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux