Re: [PATCH 29/45] writeback: fix the shmem AOP_WRITEPAGE_ACTIVATE case

[Hugh Dickins <hugh.dickins@tiscali.co.uk>]

On Wed, 7 Oct 2009, Wu Fengguang wrote:

> When shmem returns AOP_WRITEPAGE_ACTIVATE, the inode pages cannot be
> synced in the near future. So write_cache_pages shall stop writting this
> inode, and shmem shall increase pages_skipped to instruct VFS not to
> busy retry.
> 
> CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

Okay, it embarrasses me to see AOP_WRITEPAGE_ACTIVATE (and its horrid
"in this one case the page is still locked" semantic) still around -
my patch to remove it vanished from mmotm (probably caused a temporary
conflict) and I've never chased it up (partly out of guilt that I'd not
yet kept my promise to contact the openAFS people about their use of it).

But that's orthogonal to your concern here: for so long as there has
been a wbc->pages_skipped, I guess shmem_writepage() should have been
incrementing it there - thanks.  But I don't believe the VFS will ever
have any interest in pages_skipped from shmem_writepage(): do you have
evidence that it does?  If so, I need to investigate.

And the accompanying change to write_cache_pages() seems irrelevant
and misguided.  Irrelevant because write_cache_pages() should never be
dealing with shmem_writepage() (its bdi should keep it well away), and
should never be dealing with reclaim, which is the only case in which
shmem_writepage() returns AOP_WRITEPAGE_ACTIVATE - or have your other
changes, or the bdi work, changed that?

And misguided because in your change to write_cache_pages() you're
taking AOP_WRITEPAGE_ACTIVATE to say that it should now give up, not
process more pages.  We just don't know that.  All it means is that
this one page couldn't be written and should be reactivated (if it
were under reclaim): it might be the case that every other page tried
after would get treated in the same way, or it might be the case that
the next page would get written successfully.  That info is just not
provided.

Hugh

> ---
>  mm/page-writeback.c |   23 +++++++++++------------
>  mm/shmem.c          |    1 +
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> --- linux.orig/mm/page-writeback.c	2009-10-06 23:39:28.000000000 +0800
> +++ linux/mm/page-writeback.c	2009-10-06 23:39:29.000000000 +0800
> @@ -851,19 +851,18 @@ continue_unlock:
>  				if (ret == AOP_WRITEPAGE_ACTIVATE) {
>  					unlock_page(page);
>  					ret = 0;
> -				} else {
> -					/*
> -					 * done_index is set past this page,
> -					 * so media errors will not choke
> -					 * background writeout for the entire
> -					 * file. This has consequences for
> -					 * range_cyclic semantics (ie. it may
> -					 * not be suitable for data integrity
> -					 * writeout).
> -					 */
> -					done = 1;
> -					break;
>  				}
> +				/*
> +				 * done_index is set past this page,
> +				 * so media errors will not choke
> +				 * background writeout for the entire
> +				 * file. This has consequences for
> +				 * range_cyclic semantics (ie. it may
> +				 * not be suitable for data integrity
> +				 * writeout).
> +				 */
> +				done = 1;
> +				break;
>   			}
>  
>  			if (nr_to_write > 0) {
> --- linux.orig/mm/shmem.c	2009-10-06 23:37:40.000000000 +0800
> +++ linux/mm/shmem.c	2009-10-06 23:39:29.000000000 +0800
> @@ -1103,6 +1103,7 @@ unlock:
>  	 */
>  	swapcache_free(swap, NULL);
>  redirty:
> +	wbc->pages_skipped++;
>  	set_page_dirty(page);
>  	if (wbc->for_reclaim)
>  		return AOP_WRITEPAGE_ACTIVATE;	/* Return with page locked */