Re: [PATCH v5 03/13] mm: Put readahead pages in cache earlier
On Thu, Feb 13, 2020 at 07:36:38PM -0800, John Hubbard wrote: > I see two distinct things happening in this patch, and I think they want to > each be > in their own patch: > > 1) A significant refactoring of the page loop, and > > 2) Changing the place where the page is added to the page cache. (Only this > one is >mentioned in the commit description.) > > We'll be more likely to spot any errors if these are teased apart. Thanks. I ended up splitting this patch into three, each hopefully easier to understand.
Re: [PATCH v5 04/13] mm: Add readahead address space operation
On Thu, Feb 13, 2020 at 09:36:25PM -0800, John Hubbard wrote:
> > +static inline struct page *readahead_page(struct readahead_control *rac)
> > +{
> > + struct page *page;
> > +
> > + if (!rac->nr_pages)
> > + return NULL;
> > +
> > + page = xa_load(>mapping->i_pages, rac->start);
>
>
> Is it worth asserting that the page was found:
>
> VM_BUG_ON_PAGE(!page || xa_is_value(page), page);
>
> ? Or is that overkill here?
It shouldn't be possible since they were just added in a locked state.
If it did happen, it'll be caught by the assert below -- dereferencing
a NULL pointer or a shadow entry is not going to go well.
> > + VM_BUG_ON_PAGE(!PageLocked(page), page);
> > + rac->batch_count = hpage_nr_pages(page);
> > + rac->start += rac->batch_count;
>
> The above was surprising, until I saw the other thread with Dave and you.
> I was reviewing this patchset in order to have a chance at understanding the
> follow-on patchset ("Large pages in the page cache"), and it seems like that
> feature has a solid head start here. :)
Right, I'll document that.
Re: [PATCH v5 01/13] mm: Fix the return type of __do_page_cache_readahead
On Mon, Feb 10, 2020 at 05:03:36PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > ra_submit() which is a wrapper around __do_page_cache_readahead() already > returns an unsigned long, and the 'nr_to_read' parameter is an unsigned > long, so fix __do_page_cache_readahead() to return an unsigned long, > even though I'm pretty sure we're not going to readahead more than 2^32 > pages ever. I was going through this and realised it's completely pointless -- the returned value from ra_submit() and __do_page_cache_readahead() is eventually ignored through all paths. So I'm replacing this patch with one that makes everything return void.
