Re: [PATCH] mm: fix long time stall from mm_populate

[Minchan Kim]

On Tue, Feb 11, 2020 at 09:28:03AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 11, 2020 at 08:34:04AM -0800, Minchan Kim wrote:
> > On Tue, Feb 11, 2020 at 04:23:23AM -0800, Matthew Wilcox wrote:
> > > On Mon, Feb 10, 2020 at 08:25:36PM -0800, Minchan Kim wrote:
> > > > On Mon, Feb 10, 2020 at 07:54:12PM -0800, Matthew Wilcox wrote:
> > > > > On Mon, Feb 10, 2020 at 07:50:04PM -0800, Minchan Kim wrote:
> > > > > > On Mon, Feb 10, 2020 at 05:10:21PM -0800, Matthew Wilcox wrote:
> > > > > > > On Mon, Feb 10, 2020 at 04:19:58PM -0800, Minchan Kim wrote:
> > > > > > > >       filemap_fault
> > > > > > > >         find a page form page(PG_uptodate|PG_readahead|PG_writeback)
> > > > > > > 
> > > > > > > Uh ... That shouldn't be possible.
> > > > > > 
> > > > > > Please see shrink_page_list. Vmscan uses PG_reclaim to accelerate
> > > > > > page reclaim when the writeback is done so the page will have both
> > > > > > flags at the same time and the PG reclaim could be regarded as
> > > > > > PG_readahead in fault conext.
> > > > > 
> > > > > What part of fault context can make that mistake?  The snippet I quoted
> > > > > below is from page_cache_async_readahead() where it will clearly not
> > > > > make that mistake.  There's a lot of code here; please don't presume I
> > > > > know all the areas you're talking about.
> > > > 
> > > > Sorry about being not clear. I am saying  filemap_fault ->
> > > > do_async_mmap_readahead
> > > > 
> > > > Let's assume the page is hit in page cache and vmf->flags is !FAULT_FLAG
> > > > TRIED so it calls do_async_mmap_readahead. Since the page has PG_reclaim
> > > > and PG_writeback by shrink_page_list, it goes to 
> > > > 
> > > > do_async_mmap_readahead
> > > >   if (PageReadahead(page))
> > > >     fpin = maybe_unlock_mmap_for_io();
> > > >     page_cache_async_readahead
> > > >       if (PageWriteback(page))
> > > >         return;
> > > >       ClearPageReadahead(page); <- doesn't reach here until the writeback is clear
> > > >       
> > > > So, mm_populate will repeat the loop until the writeback is done.
> > > > It's my just theory but didn't comfirm it by the testing.
> > > > If I miss something clear, let me know it.
> > > 
> > > Ah!  Surely the right way to fix this is ...
> > 
> > I'm not sure it's right fix. Actually, I wanted to remove PageWriteback check
> > in page_cache_async_readahead because I don't see corelation. Why couldn't we
> > do readahead if the marker page is PG_readahead|PG_writeback design PoV?
> > Only reason I can think of is it makes *a page* will be delayed for freeing
> > since we removed PG_reclaim bit, which would be over-optimization for me.
> 
> You're confused.  Because we have a shortage of bits in the page flags,
> we use the same bit for both PageReadahead and PageReclaim.  That doesn't
> mean that a page marked as PageReclaim should be treated as PageReadahead.

My point is why we couldn't do readahead if the marker page is under PG_writeback.
It was there for a long time and you were adding one more so I was curious what's
reasoning comes from. Let me find why PageWriteback check in
page_cache_async_readahead from the beginning.

	fe3cba17c4947, mm: share PG_readahead and PG_reclaim

The reason comes from the description

    b) clear PG_readahead => implicit clear of PG_reclaim
            one(and only one) page will not be reclaimed in time
            it can be avoided by checking PageWriteback(page) in readahead first

The goal was to avoid delay freeing of the page by clearing PG_reclaim.
I'm saying that I feel it's over optimization. IOW, it would be okay to
lose a page to be accelerated reclaim.

> 
> > Other concern is isn't it's racy? IOW, page was !PG_writeback at the check below
> > in your snippet but it was under PG_writeback in page_cache_async_readahead and
> > then the IO was done before refault reaching the code again. It could be repeated
> > *theoretically* even though it's very hard to happen in real practice.
> > Thus, I think it would be better to remove PageWriteback check from
> > page_cache_async_readahead if we really want to go the approach.
> 
> PageReclaim is always cleared before PageWriteback.  eg here:
> 
> void end_page_writeback(struct page *page)
> ...
>         if (PageReclaim(page)) {
>                 ClearPageReclaim(page);
>                 rotate_reclaimable_page(page);
>         }
> 
>         if (!test_clear_page_writeback(page))
>                 BUG();
> 
> so if PageWriteback is clear, PageReclaim must already be observable as clear.
> 

I'm saying live lock siutation below.
It would be hard to trigger since IO is very slow but isn't it possible
theoretically?


                 CPU 1                                                CPU 2
mm_populate
1st trial
  __get_user_pages
    handle_mm_fault
      filemap_fault
        do_async_mmap_readahead 
        if (!PageWriteback(page) && PageReadahead(page)) {
          fpin = maybe_unlock_mmap_for_io
          page_cache_async_readahead
                                                                    set_page_writeback here
            if (PageWriteback(page))
	      return; <- hit

                                                                     writeback completed and reclaimed the page
								     ..
								     ondemand readahead allocates new page and mark it to PG_readahead
2nd trial
 __get_user_pages
    handle_mm_fault
      filemap_fault
        do_async_mmap_readahead 
        if (!PageWriteback(page) && PageReadahead(page)) {
          fpin = maybe_unlock_mmap_for_io
          page_cache_async_readahead
                                                                    set_page_writeback here
            if (PageWriteback(page))
	      return; <- hit

                                                                     writeback completed and reclaimed the page
								     ..
								     ondemand readahead allocates new page and mark it to PG_readahead

3rd trial
..


Let's consider ra_pages, too as I mentioned. Isn't it another hole to make
such live lock if other task suddenly reset it to zero?

void page_cache_async_readahead(..)
{
        /* no read-ahead */
        if (!ra->ra_pages)
                return;