Re: [PATCH 2/3] Memory management livelock

[Andrew Morton <akpm@linux-foundation.org>]

> Subject: [PATCH 2/3] Memory management livelock

Please don't send multiple patches with identical titles - think up a
good, unique, meaningful title for each patch.

On Wed, 24 Sep 2008 14:52:18 -0400 (EDT) Mikulas Patocka <mpatocka@redhat.com> wrote:

> Avoid starvation when walking address space.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 

Please include a full changelog with each iteration of each patch.

That changelog should explain the reason for playing games with
bitlocks so Linus doesn't have kittens when he sees it.

>  include/linux/pagemap.h |    1 +
>  mm/filemap.c            |   20 ++++++++++++++++++++
>  mm/page-writeback.c     |   37 ++++++++++++++++++++++++++++++++++++-
>  mm/truncate.c           |   24 +++++++++++++++++++++++-
>  4 files changed, 80 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.27-rc7-devel/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.27-rc7-devel.orig/include/linux/pagemap.h	2008-09-24 02:57:37.000000000 +0200
> +++ linux-2.6.27-rc7-devel/include/linux/pagemap.h	2008-09-24 02:59:04.000000000 +0200
> @@ -21,6 +21,7 @@
>  #define	AS_EIO		(__GFP_BITS_SHIFT + 0)	/* IO error on async write */
>  #define AS_ENOSPC	(__GFP_BITS_SHIFT + 1)	/* ENOSPC on async write */
>  #define AS_MM_ALL_LOCKS	(__GFP_BITS_SHIFT + 2)	/* under mm_take_all_locks() */
> +#define AS_STARVATION	(__GFP_BITS_SHIFT + 3)	/* an anti-starvation barrier */
>  
>  static inline void mapping_set_error(struct address_space *mapping, int error)
>  {
> Index: linux-2.6.27-rc7-devel/mm/filemap.c
> ===================================================================
> --- linux-2.6.27-rc7-devel.orig/mm/filemap.c	2008-09-24 02:59:33.000000000 +0200
> +++ linux-2.6.27-rc7-devel/mm/filemap.c	2008-09-24 03:13:47.000000000 +0200
> @@ -269,10 +269,19 @@ int wait_on_page_writeback_range(struct 
>  	int nr_pages;
>  	int ret = 0;
>  	pgoff_t index;
> +	long pages_to_process;
>  
>  	if (end < start)
>  		return 0;
>  
> +	/*
> +	 * Estimate the number of pages to process. If we process significantly
> +	 * more than this, someone is making writeback pages under us.
> +	 * We must pull the anti-starvation plug.
> +	 */
> +	pages_to_process = bdi_stat(mapping->backing_dev_info, BDI_WRITEBACK);
> +	pages_to_process += (pages_to_process >> 3) + 16;

This sequence appears twice and it would probably be clearer to
implement it in a well-commented function.

>  	pagevec_init(&pvec, 0);
>  	index = start;
>  	while ((index <= end) &&
> @@ -288,6 +297,10 @@ int wait_on_page_writeback_range(struct 
>  			if (page->index > end)
>  				continue;
>  
> +			if (pages_to_process >= 0)
> +				if (!pages_to_process--)
> +					wait_on_bit_lock(&mapping->flags, AS_STARVATION, wait_action_schedule, TASK_UNINTERRUPTIBLE);

This is copied three times and perhaps also should be factored out.

Please note that an effort has been made to make mm/filemap.c look
presentable in an 80-col display.

>  			wait_on_page_writeback(page);
>  			if (PageError(page))
>  				ret = -EIO;
> @@ -296,6 +309,13 @@ int wait_on_page_writeback_range(struct 
>  		cond_resched();
>  	}
>  
> +	if (pages_to_process < 0) {
> +		smp_mb__before_clear_bit();
> +		clear_bit(AS_STARVATION, &mapping->flags);
> +		smp_mb__after_clear_bit();
> +		wake_up_bit(&mapping->flags, AS_STARVATION);
> +	}

This sequence is repeated three or four times and should be pulled out
into a well-commented function.  That comment should explain the logic
behind the use of these barriers, please.