Re: [PATCH 3/3] mm: make PageReadahead more strict

[Minchan Kim <minchan@kernel.org>]

On Mon, Feb 17, 2020 at 10:31:28AM +0100, Jan Kara wrote:
> On Wed 12-02-20 14:16:14, Minchan Kim wrote:
> > 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 <minchan@kernel.org>
> 
> ...
> 
> > +/* 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.

It's accurate. Thanks, Jan.

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

I believe it's okay to remove PG_writeback check in page_cache_async_readahead. 
It's merely optmization to accelerate page reclaim when the LRU victim page's
writeback is done by VM. If we removes the condition, we just lose few pages at
the moment for fast reclaim but finally they could be reclaimed in inactive LRU
and it would be *rare* in that that kinds of writeback from reclaim context
should be not common and doesn't affect system behavior a lot.


> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR