Re: [patch 7/9] mm: write_cache_pages terminate quickly

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

On Wed, 29 Oct 2008 01:47:22 +1100
npiggin@suse.de wrote:

> Terminate the write_cache_pages loop upon encountering the first page past
> end, without locking the page. Pages cannot have their index change when we
> have a reference on them (truncate, eg truncate_inode_pages_range performs
> the same check without the page lock).
> 

Traditionally lock_page() is used to stabilise ->index and ->mapping. 
Here you introduce a new and very subtle sort-of-locking rule without
actually really introducing it at all.  OK, there's a little comment
buried way down in this function.  But there's a contradictory comment
over truncate_inode_pages_range() ("When looking at...").

How do we make this new locking rule maintainable?  How do we avoid
breaking it in the future?  How do we prevent accidental breakage from
slipping past developers' and reviewers' attention?

Given the additional maintenance burdens, is this change worth doing
at all?


> ---
> Index: linux-2.6/mm/page-writeback.c
> ===================================================================
> --- linux-2.6.orig/mm/page-writeback.c
> +++ linux-2.6/mm/page-writeback.c
> @@ -911,15 +911,24 @@ retry:
>  		for (i = 0; i < nr_pages; i++) {
>  			struct page *page = pvec.pages[i];
>  
> -			done_index = page->index + 1;
> -
>  			/*
> -			 * At this point we hold neither mapping->tree_lock nor
> -			 * lock on the page itself: the page may be truncated or
> -			 * invalidated (changing page->mapping to NULL), or even
> -			 * swizzled back from swapper_space to tmpfs file
> -			 * mapping
> +			 * At this point, the page may be truncated or
> +			 * invalidated (changing page->mapping to NULL), or
> +			 * even swizzled back from swapper_space to tmpfs file
> +			 * mapping. However, page->index will not change
> +			 * because we have a reference on the page.
>  			 */
> +			if (page->index > end) {
> +				/*
> +				 * can't be range_cyclic (1st pass) because
> +				 * end == -1 in that case.
> +				 */
> +				done = 1;
> +				break;
> +			}
> +
> +			done_index = page->index + 1;
> +
>  			lock_page(page);
>  
>  			/*
> @@ -936,15 +945,6 @@ continue_unlock:
>  				continue;
>  			}
>  
> -			if (page->index > end) {
> -				/*
> -				 * can't be range_cyclic (1st pass) because
> -				 * end == -1 in that case.
> -				 */
> -				done = 1;
> -				goto continue_unlock;
> -			}
> -
>  			if (!PageDirty(page)) {
>  				/* someone wrote it for us */
>  				goto continue_unlock;
> 
> --