From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0D10C433E3 for ; Fri, 21 Aug 2020 01:03:45 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6F1E2207DE for ; Fri, 21 Aug 2020 01:03:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6F1E2207DE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=fromorbit.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id E6C8F8D002E; Thu, 20 Aug 2020 21:03:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DF2C48D001E; Thu, 20 Aug 2020 21:03:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C93FB8D002E; Thu, 20 Aug 2020 21:03:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id AE6728D001E for ; Thu, 20 Aug 2020 21:03:44 -0400 (EDT) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 6EB741E0F for ; Fri, 21 Aug 2020 01:03:44 +0000 (UTC) X-FDA: 77172778368.26.pet07_260eac527035 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin26.hostedemail.com (Postfix) with ESMTP id 3691E1804A300 for ; Fri, 21 Aug 2020 01:03:44 +0000 (UTC) X-HE-Tag: pet07_260eac527035 X-Filterd-Recvd-Size: 5771 Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by imf14.hostedemail.com (Postfix) with ESMTP for ; Fri, 21 Aug 2020 01:03:43 +0000 (UTC) Received: from dread.disaster.area (pa49-181-146-199.pa.nsw.optusnet.com.au [49.181.146.199]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id AF65F823059; Fri, 21 Aug 2020 11:03:31 +1000 (AEST) Received: from dave by dread.disaster.area with local (Exim 4.92.3) (envelope-from ) id 1k8vSs-0007mS-Td; Fri, 21 Aug 2020 11:03:30 +1000 Date: Fri, 21 Aug 2020 11:03:30 +1000 From: Dave Chinner To: Gao Xiang Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Carlos Maiolino , Eric Sandeen , "Huang, Ying" , Yang Shi , Rafael Aquini , stable Subject: Re: [PATCH v2] mm, THP, swap: fix allocating cluster for swapfile by mistake Message-ID: <20200821010330.GC7728@dread.disaster.area> References: <20200820045323.7809-1-hsiangkao@redhat.com> <20200820233446.GB7728@dread.disaster.area> <20200821002145.GA28298@xiangao.remote.csb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200821002145.GA28298@xiangao.remote.csb> X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.3 cv=QIgWuTDL c=1 sm=1 tr=0 cx=a_idp_d a=GorAHYkI+xOargNMzM6qxQ==:117 a=GorAHYkI+xOargNMzM6qxQ==:17 a=kj9zAlcOel0A:10 a=y4yBn9ojGxQA:10 a=7-415B0cAAAA:8 a=6Q28Y-3DynG6Ewtw8AoA:9 a=CjuIK1q_8ugA:10 a=biEYGPWJfzWAr4FL6Ov7:22 X-Rspamd-Queue-Id: 3691E1804A300 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam02 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Aug 21, 2020 at 08:21:45AM +0800, Gao Xiang wrote: > Hi Dave, > > On Fri, Aug 21, 2020 at 09:34:46AM +1000, Dave Chinner wrote: > > 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? > > I think yes. > > > > > > 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? > > > > Yeah, I once think about some similiar thing as well. My thought for now is > > - First, SWAP THP doesn't claim to support such swapfile for now. > And the original author tried to explicitly avoid the whole thing in > > f0eea189e8e9 ("mm, THP, swap: Don't allocate huge cluster for file backed swap device") > > So such thing would be considered as some new feature and need > more testing at least. But for now I think we just need a quick > fix to fix the commit f0eea189e8e9 to avoid regression and for > backport use. Sure, a quick fix is fine for the current issue. I'm asking questions about the design/architecture of how THP_SWAP is supposed to work and whether swapfiles are violating some other undocumented assumption about swapping THP files... > - It is hard for users to control swapfile in > SWAPFILE_CLUSTER * PAGE_SIZE extents, especially users' > disk are fragmented or have some on-disk metadata limitation or > something. It's very hard for users to utilize this and arrange > their swapfile physical addr alignment and fragments for now. This isn't something users control. The swapfile extent mapping code rounds the swap extents inwards so that the parts of the on-disk extents that are not aligned or cannot hold a full page are omitted from the ranges of the file that can be swapped to. i.e. a file that extents aligned to 4kB is fine for a 4KB page size machine, but needs additional alignment to allow swapping to work on a 64kB page size machine. Hence the swap code rounds the file extents inwards to PAGE_SIZE to align them correctly. We really should be doing this for THP page size rather than PAGE_SIZE if THP_SWAP is enabled, regardless of whether swap clustering is enabled or not... Cheers, Dave. -- Dave Chinner david@fromorbit.com