mm: Don't bother dropping mmap_sem for zero size readahead

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.

PG_readahead flag is shared with PG_reclaim but PG_reclaim is only used in write context while PG_readahead is used for read context.

To make it clear, let's introduce PageReadahead wrapper with !PageWriteback so it could make code clear and we could drop PageWriteback check in page_cache_async_readahead, which removes pointless dropping mmap_sem.

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

Quoted 9 lines · Minchan Kim

PG_readahead flag is shared with PG_reclaim but PG_reclaim is only used in write context while PG_readahead is used for read context.

To make it clear, let's introduce PageReadahead wrapper with !PageWriteback so it could make code clear and we could drop PageWriteback check in page_cache_async_readahead, which removes pointless dropping mmap_sem.

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

...

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

Quoted 8 lines

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

+/* Clear PG_readahead only if it's PG_readahead, not PG_reclaim */
+static inline int TestClearPageReadahead(struct page *page)
+{
+	VM_BUG_ON_PGFLAGS(PageCompound(page), page);
+
+	return !PageWriteback(page) ||
+			test_and_clear_bit(PG_reclaim, &page->flags);
+}

I think this is still wrong - if PageWriteback is not set, it will never clear PG_reclaim bit so effectively the page will stay in PageReadahead state!

The logic you really want to implement is:

	if (PageReadahead(page)) { <- this is your new PageReadahead
				    implementation
		clear_bit(PG_reclaim, &page->flags);
		return 1;
	}
	return 0;

Now this has the problem that it is not atomic. The only way I see to make this fully atomic is using cmpxchg(). If we wanted to make this kinda-sorta OK, the proper condition would look like:

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

Which is similar to what you originally had but different because in C '&&' operator is not commutative due to side-effects committed at sequence points.

BTW: I share Andrew's view that we are piling hacks to fix problems caused by older hacks. But I don't see any good option how to unalias PG_readahead and PG_reclaim.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR