Re: [PATCH] mm: support anonymous stable page

[Hugh Dickins]

On Fri, 11 Nov 2016, Minchan Kim wrote:
> Sorry for sending a wrong version. Here is new one.
> 
> From 2d42ead9335cde51fd58d6348439ca03cf359ba2 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@xxxxxxxxxx>
> Date: Fri, 11 Nov 2016 15:02:57 +0900
> Subject: [PATCH] mm: support anonymous stable page
> 
> For developemnt for zram-swap asynchronous writeback, I found
> strange corruption of compressed page. With investigation, it
> reveals currently stable page doesn't support anonymous page.
> IOW, reuse_swap_page can reuse the page without waiting
> writeback completion so that it can corrupt data during
> zram compression. It can affect every swap device which supports
> asynchronous writeback and CRC checking as well as zRAM.
> 
> Unfortunately, reuse_swap_page should be atomic so that we
> cannot wait on writeback in there so the approach in this patch
> is simply return false if we found it needs stable page.
> Although it increases memory footprint temporarily, it happens
> rarely and it should be reclaimed easily althoug it happened.
> Also, It would be better than waiting of IO completion, which
> is critial path for application latency.
> 
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>

Ack to your intention (we discussed this together years ago, but saw
no actual demand for it before now), and I like what you're doing;
but it has to be NAK to this implementation.

I sensed there was an problem when you posted; but only now, after
searching through the uses of mapping->host, do I see that problem.

You're setting swap's mapping->host = inode when it used to be NULL:
which seems like a very good way to get what you need, but I'm afraid
it's a change which goes way beyond your intention.

See inode_to_bdi(): for ordinary disk-based swap, it will now pick
up the bdi of the block device instead of noop_backing_dev_info, so
swap would then pass the mapping_cap_account_dirty() and similar
tests (mostly in mm/page-writeback.c), and go down codepaths it
has never gone down before.

It's possible that swap (and shmem) would be better off going down
those paths, to be throttled in a similar way to files; but that's
debatable, and a much bigger change than you want to get into for
zram stable pages.

Maybe add SWP_STABLE_WRITES in include/linux/swap.h, and set that
in swap_info->flags according to bdi_cap_stable_pages_required(),
leaving mapping->host itself NULL as before?

Hugh

> ---
>  mm/swapfile.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2210de290b54..ea591435d8e0 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -943,11 +943,21 @@ bool reuse_swap_page(struct page *page, int *total_mapcount)
>  	count = page_trans_huge_mapcount(page, total_mapcount);
>  	if (count <= 1 && PageSwapCache(page)) {
>  		count += page_swapcount(page);
> -		if (count == 1 && !PageWriteback(page)) {
> +		if (count != 1)
> +			goto out;
> +		if (!PageWriteback(page)) {
>  			delete_from_swap_cache(page);
>  			SetPageDirty(page);
> +		} else {
> +			struct address_space *mapping;
> +
> +			mapping = page_mapping(page);
> +			if (bdi_cap_stable_pages_required(
> +					inode_to_bdi(mapping->host)))
> +				return false;
>  		}
>  	}
> +out:
>  	return count <= 1;
>  }
>  
> @@ -2180,6 +2190,7 @@ static struct swap_info_struct *alloc_swap_info(void)
>  static int claim_swapfile(struct swap_info_struct *p, struct inode *inode)
>  {
>  	int error;
> +	struct address_space *swapper_space;
>  
>  	if (S_ISBLK(inode->i_mode)) {
>  		p->bdev = bdgrab(I_BDEV(inode));
> @@ -2202,6 +2213,9 @@ static int claim_swapfile(struct swap_info_struct *p, struct inode *inode)
>  	} else
>  		return -EINVAL;
>  
> +	swapper_space = &swapper_spaces[p->type];
> +	swapper_space->host = inode;
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4