Re: [PATCH 2/5] shmemfs: Use redirty_page_for_writepage()

[Hugh Dickins <hughd@google.com>]

On Wed, 5 Mar 2014, Chris Wilson wrote:

> "When we cannot write a page we should use redirty_page_for_writepage()
> instead of plain set_page_dirty(). That tells writeback code we have
> problems, redirties only the page (redirtying buffers is not needed),
> and updates mm accounting of failed page writes."

I didn't locate the origin of that quotation, but it's talking about
the usual filesystem cap_account_dirty/cap_account_writeback protocol.

shmem doesn't participate it that: it only writes out (to swap) under
memory pressure, not for sync, and follows a much simpler path.  Using
redirty_page_for_writepage() would lead it into complications, some of
which we prefer to avoid for efficiency, some of which would actually
be wrong (unless/until there's reason to convert shmem over to the
full protocol).

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Hugh Dickins <hughd@google.com>

So NAK.

But you didn't explain why you want to make this change.  I presume
it's for the 5/5 which you didn't Cc to me, but I've looked up on
lists.freedesktop.org/archives/intel-gfx.

That's a bigger change and worrying: I've not thought through
the consequences of shmem page writeback from generic_writepages()
called from within a slab shrinker: we're used to doing shmem page
writeback from pageout() in mm/vmscan.c and nowhere else.

One thing that would certainly be wrong (liable to deadlock) would
be to do that when shrink_control's gfp_mask does not have __GFP_IO.
But I'm not comfortable with page writeback from this level at all.

The shmem_truncate_range() (you've had for a long time) should be safe,
and the new invalidate_mapping_pages() too: neither of those gets into
I/O, and invalidate_mapping_pages() looks as if it does all that's
necessary for those pages to be put under writeback at the next
shrink_inactive_list() of the lruvec.

Or perhaps there's a gotcha or two, which we can fix up.
But please try to stick to truncation and invalidation.

Hugh

> ---
>  mm/shmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 142b0bc085e1..18aa88eff8e3 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -872,7 +872,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	mutex_unlock(&shmem_swaplist_mutex);
>  	swapcache_free(swap, NULL);
>  redirty:
> -	set_page_dirty(page);
> +	redirty_page_for_writepage(wbc, page);
>  	if (wbc->for_reclaim)
>  		return AOP_WRITEPAGE_ACTIVATE;	/* Return with page locked */
>  	unlock_page(page);
> -- 
> 1.9.0