Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 21 Aug 2020 09:34:46 +1000
From:   Dave Chinner <david@fromorbit.com>
To:     Gao Xiang <hsiangkao@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Carlos Maiolino <cmaiolino@redhat.com>,
        Eric Sandeen <esandeen@redhat.com>,
        "Huang, Ying" <ying.huang@intel.com>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        Rafael Aquini <aquini@redhat.com>,
        stable <stable@vger.kernel.org>
Subject: Re: [PATCH v2] mm, THP, swap: fix allocating cluster for swapfile by
 mistake
Message-ID: <20200820233446.GB7728@dread.disaster.area>
References: <20200820045323.7809-1-hsiangkao@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200820045323.7809-1-hsiangkao@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 20, 2020 at 12:53:23PM +0800, Gao Xiang wrote:
> SWP_FS is used to make swap_{read,write}page() go through
> the filesystem, and it's only used for swap files over
> NFS. So, !SWP_FS means non NFS for now, it could be either
> file backed or device backed. Something similar goes with
> legacy SWP_FILE.
> 
> So in order to achieve the goal of the original patch,
> SWP_BLKDEV should be used instead.
> 
> FS corruption can be observed with SSD device + XFS +
> fragmented swapfile due to CONFIG_THP_SWAP=y.
> 
> I reproduced the issue with the following details:
> 
> Environment:
> QEMU + upstream kernel + buildroot + NVMe (2 GB)
> 
> Kernel config:
> CONFIG_BLK_DEV_NVME=y
> CONFIG_THP_SWAP=y

Ok, so at it's core this is a swap file extent versus THP swap
cluster alignment issue?

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 6c26916e95fd..2937daf3ca02 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1074,7 +1074,7 @@ int get_swap_pages(int n_goal, swp_entry_t swp_entries[], int entry_size)
>  			goto nextsi;
>  		}
>  		if (size == SWAPFILE_CLUSTER) {
> -			if (!(si->flags & SWP_FS))
> +			if (si->flags & SWP_BLKDEV)
>  				n_ret = swap_alloc_cluster(si, swp_entries);
>  		} else
>  			n_ret = scan_swap_map_slots(si, SWAP_HAS_CACHE,

IOWs, if you don't make this change, does the corruption problem go
away if you align swap extents in iomap_swapfile_add_extent() to
(SWAPFILE_CLUSTER * PAGE_SIZE) instead of just PAGE_SIZE?

I.e. if the swapfile extents are aligned correctly to huge page swap
cluster size and alignment, does the swap clustering optimisations
for swapping THP pages work correctly? And, if so, is there any
performance benefit we get from enabling proper THP swap clustering
on swapfiles?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com