linux-kernel - Re: [PATCH] vmscan: remove wait_on_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100728091032.GD5300@csn.ul.ie>
Date:	Wed, 28 Jul 2010 10:10:33 +0100
From:	Mel Gorman <mel@....ul.ie>
To:	Wu Fengguang <fengguang.wu@...el.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan.kim@...il.com>,
	Andy Whitcroft <apw@...dowen.org>,
	Rik van Riel <riel@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Christoph Hellwig <hch@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Dave Chinner <david@...morbit.com>,
	Chris Mason <chris.mason@...cle.com>,
	Nick Piggin <npiggin@...e.de>,
	Johannes Weiner <hannes@...xchg.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Andreas Mohr <andi@...as.de>, Bill Davidsen <davidsen@....com>,
	Ben Gamari <bgamari.foss@...il.com>
Subject: Re: [PATCH] vmscan: remove wait_on_page_writeback() from pageout()

On Wed, Jul 28, 2010 at 04:46:54PM +0800, Wu Fengguang wrote:
> The wait_on_page_writeback() call inside pageout() is virtually dead code.
> 
>         shrink_inactive_list()
>           shrink_page_list(PAGEOUT_IO_ASYNC)
>             pageout(PAGEOUT_IO_ASYNC)
>           shrink_page_list(PAGEOUT_IO_SYNC)
>             pageout(PAGEOUT_IO_SYNC)
> 
> Because shrink_page_list/pageout(PAGEOUT_IO_SYNC) is always called after
> a preceding shrink_page_list/pageout(PAGEOUT_IO_ASYNC), the first
> pageout(ASYNC) converts dirty pages into writeback pages, the second
> shrink_page_list(SYNC) waits on the clean of writeback pages before
> calling pageout(SYNC). The second shrink_page_list(SYNC) can hardly run
> into dirty pages for pageout(SYNC) unless in some race conditions.
> 

It's possible for the second call to run into dirty pages as there is a
congestion_wait() call between the first shrink_page_list() call and the
second. That's a big window.

> And the wait page-by-page behavior of pageout(SYNC) will lead to very
> long stall time if running into some range of dirty pages.

True, but this is also lumpy reclaim which is depending on a contiguous
range of pages. It's better for it to wait on the selected range of pages
which is known to contain at least one old page than excessively scan and
reclaim newer pages.

> So it's bad
> idea anyway to call wait_on_page_writeback() inside pageout().
> 

I recognise that you are probably thinking of the stall-due-to-fork problem
but I'd expect the patch that raises the bar for <= PAGE_ALLOC_COSTLY_ORDER
to be sufficient. If not, I think it still makes sense to call
wait_on_page_writeback() for > PAGE_ALLOC_COSTLY_ORDER.


> CC: Andy Whitcroft <apw@...dowen.org>
> Signed-off-by: Wu Fengguang <fengguang.wu@...el.com>
> ---
>  mm/vmscan.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> --- linux-next.orig/mm/vmscan.c	2010-07-28 16:22:21.000000000 +0800
> +++ linux-next/mm/vmscan.c	2010-07-28 16:23:35.000000000 +0800
> @@ -324,8 +324,7 @@ typedef enum {
>   * pageout is called by shrink_page_list() for each dirty page.
>   * Calls ->writepage().
>   */
> -static pageout_t pageout(struct page *page, struct address_space *mapping,
> -						enum pageout_io sync_writeback)
> +static pageout_t pageout(struct page *page, struct address_space *mapping)
>  {
>  	/*
>  	 * If the page is dirty, only perform writeback if that write
> @@ -384,14 +383,6 @@ static pageout_t pageout(struct page *pa
>  			return PAGE_ACTIVATE;
>  		}
>  
> -		/*
> -		 * Wait on writeback if requested to. This happens when
> -		 * direct reclaiming a large contiguous area and the
> -		 * first attempt to free a range of pages fails.
> -		 */
> -		if (PageWriteback(page) && sync_writeback == PAGEOUT_IO_SYNC)
> -			wait_on_page_writeback(page);
> -
>  		if (!PageWriteback(page)) {
>  			/* synchronous write or broken a_ops? */
>  			ClearPageReclaim(page);
> @@ -727,7 +718,7 @@ static unsigned long shrink_page_list(st
>  				goto keep_locked;
>  
>  			/* Page is dirty, try to write it out here */
> -			switch (pageout(page, mapping, sync_writeback)) {
> +			switch (pageout(page, mapping)) {
>  			case PAGE_KEEP:
>  				goto keep_locked;
>  			case PAGE_ACTIVATE:
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/