mm: fix long time stall from mm_populate

filemap_fault
find a page form page(PG_uptodate|PG_readahead|PG_writeback)

Quoted 2 lines · Minchan Kim

filemap_fault
find a page form page(PG_uptodate|PG_readahead|PG_writeback)

Uh ... That shouldn't be possible.

        /*
         * Same bit is used for PG_readahead and PG_reclaim.
         */
        if (PageWriteback(page))
                return;

        ClearPageReadahead(page);

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 8 lines

        /*
         * Same bit is used for PG_readahead and PG_reclaim.
         */
        if (PageWriteback(page))
                return;

        ClearPageReadahead(page);

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 9 lines

Quoted 9 lines

        /*
         * Same bit is used for PG_readahead and PG_reclaim.
         */
        if (PageWriteback(page))
                return;

        ClearPageReadahead(page);

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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.

Quoted 34 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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

Quoted 3 lines

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

+++ b/mm/filemap.c

(quoted diff) +1 -1

@@ -2420,7 +2420,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf,
                return fpin;
        if (ra->mmap_miss > 0)
                ra->mmap_miss--;
-       if (PageReadahead(page)) {
+       if (!PageWriteback(page) && PageReadahead(page)) {
                fpin = maybe_unlock_mmap_for_io(vmf, fpin);
                page_cache_async_readahead(mapping, ra, file,
                                           page, offset, ra->ra_pages);

Quoted 39 lines · Matthew Wilcox

Quoted 37 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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.

Quoted 1 line

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.

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.

Quoted 40 lines · Minchan Kim

Quoted 39 lines · Matthew Wilcox

Quoted 37 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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.

Quoted 7 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 49 lines · Matthew Wilcox

Quoted 47 lines · Minchan Kim

Quoted 35 lines · Matthew Wilcox

Quoted 34 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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

Quoted 5 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 20 lines

Quoted 7 lines

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();

Quoted 2 lines

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;

Quoted 50 lines · Minchan Kim

Quoted 49 lines · Matthew Wilcox

Quoted 47 lines · Minchan Kim

Quoted 35 lines · Matthew Wilcox

Quoted 34 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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

Quoted 5 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Well, as far as I'm reading the code, this shouldn't usually happen - PageReadahead is set on a page that the preread into memory. Once it is used for the first time (either by page fault or normal read), readahead logic triggers starting further readahead and PageReadahead gets cleared.

What could happen though is that the page gets written into (say just a few bytes). That would keep PageReadahead set although the page will become dirty and can later be written back. I don't find not triggering writeback in this case too serious though since it should be very rare.

So I'd be OK with the change Matthew suggested although I'd prefer if we had this magic "!PageWriteback && PageReadahead" test in some helper function (like page_should_trigger_readahead()?) with a good comment explaining the details.

Quoted 82 lines

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.

Quoted 20 lines

Quoted 7 lines

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();

Quoted 2 lines

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

Quoted 8 lines

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;

So this is definitively a bug which was already reported previously. I've just sent out a patch to fix this which has somehow fallen through the cracks.

Now I agree that regardless of this fix and the fix Matthew has proposed, mm_populate() would benefit from being more robust like you suggested so I'll check that separately (but I'm no expert in that area).

How about this?

include/linux/page-flags.h +4 -2

index 1bf83c8fcaa7..d07d602476df 100644
@@ -363,8 +363,28 @@ PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
 /* PG_readahead is only used for reads; PG_reclaim is only for writes */
 PAGEFLAG(Reclaim, reclaim, PF_NO_TAIL)
 	TESTCLEARFLAG(Reclaim, reclaim, PF_NO_TAIL)
-PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
-	TESTCLEARFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+
+SETPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+CLEARPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+

Quoted 16 lines · Minchan Kim

How about this?

include/linux/page-flags.h +4 -2

index 1bf83c8fcaa7..d07d602476df 100644
@@ -363,8 +363,28 @@ PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
 /* PG_readahead is only used for reads; PG_reclaim is only for writes */
 PAGEFLAG(Reclaim, reclaim, PF_NO_TAIL)
 	TESTCLEARFLAG(Reclaim, reclaim, PF_NO_TAIL)
-PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
-	TESTCLEARFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+
+SETPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+CLEARPAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
+

include/linux/page-flags.h PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL) · line 379

Quoted 10 lines

include/linux/page-flags.h +10 -0

+/*
+ * Since PG_readahead is shared with PG_reclaim of the page flags,
+ * PageReadahead should double check whether it's readahead marker
+ * or PG_reclaim. It could be done by PageWriteback check because
+ * PG_reclaim is always with PG_writeback.
+ */
+static inline int PageReadahead(struct page *page)
+{
+	VM_BUG_ON_PGFLAGS(PageCompound(page), page);
+	return test_bit(PG_reclaim, &(page)->flags) && !PageWriteback(page);

Why not ...

	return page->flags & (1UL << PG_reclaim | 1UL << PG_writeback) ==
		(1UL << PG_reclaim);

include/linux/page-flags.h PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL) · line 384

Quoted 5 lines

include/linux/page-flags.h +5 -0

+static inline int TestClearPageReadahead(struct page *page)
+{
+	VM_BUG_ON_PGFLAGS(PageCompound(page), page);
+
+	return test_and_clear_bit(PG_reclaim, &page->flags) && !PageWriteback(page);

That's definitely wrong. It'll clear PageReclaim and then pretend it did nothing wrong.

	return !PageWriteback(page) ||
		test_and_clear_bit(PG_reclaim, &page->flags);

Quoted 6 lines

That's definitely wrong. It'll clear PageReclaim and then pretend it did nothing wrong.

	return !PageWriteback(page) ||
		test_and_clear_bit(PG_reclaim, &page->flags);

Much better, Thanks for the review, Matthew!
If there is no objection, I will send two patches to Andrew. One is PageReadahead strict, the other is limit retry from mm_populate.

Quoted 7 lines · On Wed, 12 Feb 2020 11:53:22 -0800 Minchan Kim <[email protected]> wrote:

Quoted 6 lines

That's definitely wrong. It'll clear PageReclaim and then pretend it did nothing wrong.

	return !PageWriteback(page) ||
		test_and_clear_bit(PG_reclaim, &page->flags);

Quoted 3 lines

Much better, Thanks for the review, Matthew!
If there is no objection, I will send two patches to Andrew. One is PageReadahead strict, the other is limit retry from mm_populate.

With much more detailed changelogs, please!

This all seems rather screwy. if a page is under writeback then it is uptodate and we should be able to fault it in immediately.

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Wed, 12 Feb 2020 11:53:22 -0800 Minchan Kim <[email protected]> wrote:

Quoted 6 lines

That's definitely wrong. It'll clear PageReclaim and then pretend it did nothing wrong.

	return !PageWriteback(page) ||
		test_and_clear_bit(PG_reclaim, &page->flags);

Quoted 8 lines

Quoted 3 lines

Much better, Thanks for the review, Matthew!
If there is no objection, I will send two patches to Andrew. One is PageReadahead strict, the other is limit retry from mm_populate.

With much more detailed changelogs, please!

This all seems rather screwy. if a page is under writeback then it is uptodate and we should be able to fault it in immediately.

Hi Andrew,

This description in cover-letter will work? If so, I will add each part below in each patch.

Subject: [PATCH 0/3] fixing mm_populate long stall

I got several reports major page fault takes several seconds sometime. When I review drop mmap_sem in page fault hanlder, I found several bugs.

   CPU 1							CPU 2

mm_populate
for ()
..
ret = populate_vma_page_range
__get_user_pages
faultin_page
handle_mm_fault
filemap_fault
do_async_mmap_readahead
shrink_page_list
pageout
SetPageReclaim(=SetPageReadahead)
writepage
SetPageWriteback
if (PageReadahead(page))
maybe_unlock_mmap_for_io
up_read(mmap_sem)
page_cache_async_readahead()
if (PageWriteback(page))
return;

here, since ret from populate_vma_page_range is zero, the loop continue to run with same address with previous iteration. It will repeat the loop until the page's writeout is done(ie, PG_writeback or PG_reclaim clear).

We could fix the above specific case via adding PageWriteback. IOW,

   ret = populate_vma_page_range
   	   ...
	   ...
	   filemap_fault
	     do_async_mmap_readahead
	       if (!PageWriteback(page) && PageReadahead(page))
	         maybe_unlock_mmap_for_io
		   up_read(mmap_sem)
		 page_cache_async_readahead()
		   if (PageWriteback(page))
		     return;

That's a thing [3/3] is fixing here. Even though it could fix the problem effectively, it has still livelock problem theoretically because the page of faulty address could be reclaimed and then allocated/become readahead marker on other CPUs during faulty process is retrying in mm_populate's loop.

[2/3] is fixing the
such livelock via limiting retry count.

There is another hole for the livelock or hang of the process as well as ageWriteback - ra_pages.

mm_populate
for ()
..
ret = populate_vma_page_range

     __get_user_pages
       faultin_page
         handle_mm_fault
	   filemap_fault
	     do_async_mmap_readahead
	       if (PageReadahead(page))
	         maybe_unlock_mmap_for_io
		   up_read(mmap_sem)
		 page_cache_async_readahead()
		   if (!ra->ra_pages)
		     return;

It will repeat the loop until ra->ra_pages become non-zero. [1/3] is fixing the problem.

Quoted 51 lines · On Wed, 12 Feb 2020 15:12:10 -0800 Minchan Kim <[email protected]> wrote:

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Wed, 12 Feb 2020 11:53:22 -0800 Minchan Kim <[email protected]> wrote:

Quoted 6 lines

That's definitely wrong. It'll clear PageReclaim and then pretend it did nothing wrong.

	return !PageWriteback(page) ||
		test_and_clear_bit(PG_reclaim, &page->flags);

Quoted 8 lines

Quoted 3 lines

Much better, Thanks for the review, Matthew!
If there is no objection, I will send two patches to Andrew. One is PageReadahead strict, the other is limit retry from mm_populate.

With much more detailed changelogs, please!

This all seems rather screwy. if a page is under writeback then it is uptodate and we should be able to fault it in immediately.

Hi Andrew,

This description in cover-letter will work? If so, I will add each part below in each patch.

Subject: [PATCH 0/3] fixing mm_populate long stall

I got several reports major page fault takes several seconds sometime. When I review drop mmap_sem in page fault hanlder, I found several bugs.

   CPU 1							CPU 2

mm_populate
for ()
..
ret = populate_vma_page_range

     __get_user_pages
       faultin_page
         handle_mm_fault
	   filemap_fault
	     do_async_mmap_readahead
	     						shrink_page_list
							  pageout
							    SetPageReclaim(=SetPageReadahead)
							      writepage
							        SetPageWriteback
	       if (PageReadahead(page))
	         maybe_unlock_mmap_for_io
		   up_read(mmap_sem)
		 page_cache_async_readahead()
		   if (PageWriteback(page))
		     return;

Quoted 4 lines

here, since ret from populate_vma_page_range is zero, the loop continue to run with same address with previous iteration. It will repeat the loop until the page's writeout is done(ie, PG_writeback or PG_reclaim clear).

The populate_vma_page_range() kerneldoc is wrong. "return 0 on success, negative error code on error". Care to fix that please?

Quoted 14 lines

We could fix the above specific case via adding PageWriteback. IOW,

   ret = populate_vma_page_range
   	   ...
	   ...
	   filemap_fault
	     do_async_mmap_readahead
	       if (!PageWriteback(page) && PageReadahead(page))
	         maybe_unlock_mmap_for_io
		   up_read(mmap_sem)
		 page_cache_async_readahead()
		   if (PageWriteback(page))
		     return;

Well yes, but the testing of PageWriteback() is a hack added in fe3cba17c49471 to permit the sharing of PG_reclaim and PG_readahead. If we didn't need that hack then we could avoid adding new hacks to hack around the old hack :(. Have you considered anything along those lines? Rework how we handle PG_reclaim/PG_readahead?

Quoted 6 lines

That's a thing [3/3] is fixing here. Even though it could fix the problem effectively, it has still livelock problem theoretically because the page of faulty address could be reclaimed and then allocated/become readahead marker on other CPUs during faulty process is retrying in mm_populate's loop.

Really? filemap_fault()'s

	if (!lock_page_maybe_drop_mmap(vmf, page, &fpin))
		goto out_retry;

	/* Did it get truncated? */
	if (unlikely(compound_head(page)->mapping != mapping)) {
		unlock_page(page);
		put_page(page);
		goto retry_find;
	}

should handle such cases?

Quoted 3 lines

[2/3] is fixing the
such livelock via limiting retry count.

I wouldn't call that "fixing" :(

Quoted 23 lines

There is another hole for the livelock or hang of the process as well as ageWriteback - ra_pages.

mm_populate
for ()
..
ret = populate_vma_page_range

     __get_user_pages
       faultin_page
         handle_mm_fault
	   filemap_fault
	     do_async_mmap_readahead
	       if (PageReadahead(page))
	         maybe_unlock_mmap_for_io
		   up_read(mmap_sem)
		 page_cache_async_readahead()
		   if (!ra->ra_pages)
		     return;

It will repeat the loop until ra->ra_pages become non-zero. [1/3] is fixing the problem.

Quoted 40 lines · Minchan Kim

Quoted 39 lines · Matthew Wilcox

Quoted 37 lines · Minchan Kim

Quoted 13 lines · Matthew Wilcox

Quoted 11 lines · Minchan Kim

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Minchan Kim

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.

Quoted 4 lines

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.

Quoted 7 lines

Quoted 1 line

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.

Quoted 6 lines

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.

Basically, fault handler releases mmap_sem before requesting readahead and then it is supposed to retry lookup the page from page cache with FAULT_FLAG_TRIED so that it avoids the live lock of infinite retry.

However, what happens if the fault handler find a page from page cache and the page has readahead marker but are waiting under writeback? Plus one more condition, it happens under mm_populate which repeats faulting unless it encounters error. So let's assemble conditions below.

__mm_populate
for (...)
get_user_pages(faluty_address)
handle_mm_fault
filemap_fault
find a page form page(PG_uptodate|PG_readahead|PG_writeback) it will return VM_FAULT_RETRY
continue with faulty_address

IOW, it will repeat fault retry logic until the page will be written back in the long run. It makes big spike latency of several seconds.

This patch solves the issue by turning off fault retry logic in second trial.

Signed-off-by Minchan Kim <[email protected]>

---
It was orignated from code review once I have seen several user reports but didn't confirm yet it's the root cause.

 mm/gup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

mm/gup.c +7 -2

index 1b521e0ac1de..b3f825092abf 100644
@@ -1196,6 +1196,7 @@ int __mm_populate(unsigned long start, unsigned long len, int ignore_errors)
 	struct vm_area_struct *vma = NULL;
 	int locked = 0;
 	long ret = 0;
+	bool tried = false;
 
 	end = start + len;
 

@@ -1226,14 +1227,18 @@ int __mm_populate(unsigned long start, unsigned long len, int ignore_errors)
 		 * double checks the vma flags, so that it won't mlock pages
 		 * if the vma was already munlocked.
 		 */
-		ret = populate_vma_page_range(vma, nstart, nend, &locked);
+		ret = populate_vma_page_range(vma, nstart, nend,
+						tried ? NULL : &locked);
 		if (ret < 0) {
 			if (ignore_errors) {
 				ret = 0;
 				continue;	/* continue at next VMA */
 			}
 			break;
-		}
+		} else if (ret == 0)
+			tried = true;
+		else
+			tried = false;
 		nend = nstart + ret * PAGE_SIZE;
 		ret = 0;
 	}

Signature · 1 line

2.25.0.225.g125e21ebc7-goog

Quoted 22 lines · Minchan Kim

Basically, fault handler releases mmap_sem before requesting readahead and then it is supposed to retry lookup the page from page cache with FAULT_FLAG_TRIED so that it avoids the live lock of infinite retry.

However, what happens if the fault handler find a page from page cache and the page has readahead marker but are waiting under writeback? Plus one more condition, it happens under mm_populate which repeats faulting unless it encounters error. So let's assemble conditions below.

__mm_populate
for (...)
get_user_pages(faluty_address)
handle_mm_fault
filemap_fault
find a page form page(PG_uptodate|PG_readahead|PG_writeback) it will return VM_FAULT_RETRY
continue with faulty_address

IOW, it will repeat fault retry logic until the page will be written back in the long run. It makes big spike latency of several seconds.

Quoted 7 lines

This patch solves the issue by turning off fault retry logic in second trial.

Signed-off-by Minchan Kim <[email protected]>

---
It was orignated from code review once I have seen several user reports but didn't confirm yet it's the root cause.

Yes, I think the immediate problem is actually elsewhere but I agree that __mm_populate() should follow the general protocol of retrying only once so your change should make it more robust. The patch looks good to me, you can add:

Reviewed-by Jan Kara <[email protected]>