On Mon, 21 Nov 2016, Minchan Kim wrote:
> From: Minchan Kim <minc...@kernel.org>
> Date: Fri, 11 Nov 2016 15:02:57 +0900
> Subject: [PATCH v2] 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 <hu...@google.com>
> Cc: Darrick J. Wong <darrick.w...@oracle.com>
> Signed-off-by: Minchan Kim <minc...@kernel.org>
Acked-by: Hugh Dickins <hu...@google.com>
Looks good, thanks: we can always optimize away that little overhead
in the PageWriteback case, if it ever shows up in someone's testing.
Andrew might ask if we should Cc stable (haha): I think we agree
that it's a defect we've been aware of ever since stable pages were
first proposed, but nobody has actually been troubled by it before
your async zram development: so, you're right to be fixing it ahead
of your zram changes, but we don't see a call for backporting.
> ---
> * from v1
> * use swap_info_struct instead of swapper_space->host inode - Hugh
>
> include/linux/swap.h | 3 ++-
> mm/swapfile.c | 20 +++++++++++++++++++-
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index a56523cefb9b..55ff5593c193 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -150,8 +150,9 @@ enum {
> SWP_FILE = (1 << 7), /* set after swap_activate success */
> SWP_AREA_DISCARD = (1 << 8), /* single-time swap area discards */
> SWP_PAGE_DISCARD = (1 << 9), /* freed swap page-cluster discards */
> + SWP_STABLE_WRITES = (1 << 10), /* no overwrite PG_writeback pages */
> /* add others here before... */
> - SWP_SCANNING = (1 << 10), /* refcount in scan_swap_map */
> + SWP_SCANNING = (1 << 11), /* refcount in scan_swap_map */
> };
>
> #define SWAP_CLUSTER_MAX 32UL
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2210de290b54..66bc330c0b65 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -943,11 +943,25 @@ 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 {
> + swp_entry_t entry;
> + struct swap_info_struct *p;
> +
> + entry.val = page_private(page);
> + p = swap_info_get(entry);
> + if (p->flags & SWP_STABLE_WRITES) {
> + spin_unlock(&p->lock);
> + return false;
> + }
> + spin_unlock(&p->lock);
> }
> }
> +out:
> return count <= 1;
> }
>
> @@ -2447,6 +2461,10 @@ SYSCALL_DEFINE2(swapon, const char __user *,
> specialfile, int, swap_flags)
> error = -ENOMEM;
> goto bad_swap;
> }
> +
> + if (bdi_cap_stable_pages_required(inode_to_bdi(inode)))
> + p->flags |= SWP_STABLE_WRITES;
> +
> if (p->bdev && blk_queue_nonrot(bdev_get_queue(p->bdev))) {
> int cpu;
>
> --
> 2.7.4
