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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2BCC6C6FD35 for ; Thu, 29 Aug 2024 08:07:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 975496B00BA; Thu, 29 Aug 2024 04:07:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8FCD76B00BC; Thu, 29 Aug 2024 04:07:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 775F26B00BE; Thu, 29 Aug 2024 04:07:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 579616B00BA for ; Thu, 29 Aug 2024 04:07:37 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 0A77F80A4D for ; Thu, 29 Aug 2024 08:07:37 +0000 (UTC) X-FDA: 82504553754.29.4301CE7 Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by imf08.hostedemail.com (Postfix) with ESMTP id 29091160006 for ; Thu, 29 Aug 2024 08:07:34 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=V0LwtIff; spf=pass (imf08.hostedemail.com: domain of hughd@google.com designates 209.85.214.176 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724918834; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=NXvClmAij1L1JVrPb3Y+ThrTxtkUcnsEqbcNJq45kWA=; b=5tVgUrOKTjTiMGYriJEyC/K/hP2i1Kwb+5v1YOJFjGDSJOxhFvQ6bacBtsKkAxeV3eIwGx t0M4oMasRLMXdxeO59ctnwaIwN3u7Z9xP6ibyAObqNJ5XLQAznRmj29s0hmFm5URJKjWRb ZwOJaSOc21nB+Q5AXmu0nOdMCKDzpgo= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=V0LwtIff; spf=pass (imf08.hostedemail.com: domain of hughd@google.com designates 209.85.214.176 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724918834; a=rsa-sha256; cv=none; b=R3jlv7uTvfUcjutjRSrQCFeHzT/7Za3SCTGA7ZRNRmrmbFznzVzjB/KDGPN8G87VJDdJr6 LIgfr+fAKcvI51IRdFfgpMiBNp5KTFlih+y5bAOLdzVJshHP9IXG5bYTMAw1gDdVR9wfRU +aDNsm1FUsErbci1b7mDR1MjJKoM3l0= Received: by mail-pl1-f176.google.com with SMTP id d9443c01a7336-20203988f37so3610505ad.1 for ; Thu, 29 Aug 2024 01:07:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1724918854; x=1725523654; darn=kvack.org; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=NXvClmAij1L1JVrPb3Y+ThrTxtkUcnsEqbcNJq45kWA=; b=V0LwtIffJHrtpztdldVbNaJlbM2QR7aG5mKgurl2dzgvYhfyMHLvy+43z4CzrziAFT NwwrmBjCdnyr6uJuL30NTE0wtPnZV5Br57gg5Zpuj5nZOUg9ynbULEhsWBe2uEJHQ1lA AINl2SvdDg5b6iShEEE3TgRlfyxygiRknX58f2qaQwYBhA8piqwLoA7KqPjGkTIXlX4C VCtmC1bLqwCySRTw6RfqtmDIXwP2yxlZHKQTybF816nG7MeaRdcnRvroWooN5E8JfF5T v1cCVM2RinV1adJd+b4B1CXx8CcOVL5H4W4CgiPjNxxauRJKdjDMQQMQOhxgLHAPq7vV fqVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724918854; x=1725523654; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=NXvClmAij1L1JVrPb3Y+ThrTxtkUcnsEqbcNJq45kWA=; b=p4o/TU2yrQ1y9SADog/CR6O9akhP18/Vgf8/maIgOR1DRgokL6UEvyxMNDCOeQRQjV V3e9LIh52ioTnZ1irkKxL5hy8CITedtsg2x1upd5zkprAtVlhGygZpdhpx7UfMFqCQCt Ypn3vv6lnmy1l+CXxkncRxCa9B/Ssp8G/kxXwfMkBLcky+xt5yx/jh4zErePYMewcPnR ce7lU5PSWKlhwhs16R+hQoqCDO22gW77OkkF4YD6xChkmehrCOELHzUNcGRDBqPIXR/C vl/thJTornJ9rhP9nRAzkzrCdhjriAFOECWCrqzo/H8htzZAKMVARapKOZVDKjnfQUOj Qfcw== X-Forwarded-Encrypted: i=1; AJvYcCVK7uDcLVLRc2F9No3iLLOVy74mg5pNvl22Nqh58vLoVWDhEiuWZJ2OTfvk/P87j+tHjyfvoSlfEQ==@kvack.org X-Gm-Message-State: AOJu0Yxn3iYvaldJ7ludRJSCn7KqjzzsBY055ERdOIau/d5fAtDNXq8w 4KiHiLS9TZuJD/mZ1jYTsme3WCpWe/wCyZ/08GoXq+XenB+ohzFIZME62b4PAw== X-Google-Smtp-Source: AGHT+IFUSVbVLMuBqYsYJmxwJ/WBiZsUfZKt1gLKSFrMdL/2uNQ2Dg4lweTg36h9vyeT+RpNkIVrzg== X-Received: by 2002:a17:902:e80c:b0:202:2f24:1445 with SMTP id d9443c01a7336-2050c3cd55fmr21900525ad.33.1724918853258; Thu, 29 Aug 2024 01:07:33 -0700 (PDT) Received: from darker.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2051554541fsm6229705ad.228.2024.08.29.01.07.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Aug 2024 01:07:32 -0700 (PDT) Date: Thu, 29 Aug 2024 01:07:17 -0700 (PDT) From: Hugh Dickins To: Baolin Wang , Andrew Morton cc: Hugh Dickins , willy@infradead.org, david@redhat.com, wangkefeng.wang@huawei.com, chrisl@kernel.org, ying.huang@intel.com, 21cnbao@gmail.com, ryan.roberts@arm.com, shy828301@gmail.com, ziy@nvidia.com, ioworker0@gmail.com, da.gomez@samsung.com, p.raghav@samsung.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 4/9] mm: filemap: use xa_get_order() to get the swap entry order In-Reply-To: <3c020874-4cf3-418c-b89b-4e6ed158e5b9@linux.alibaba.com> Message-ID: References: <6876d55145c1cc80e79df7884aa3a62e397b101d.1723434324.git.baolin.wang@linux.alibaba.com> <3c020874-4cf3-418c-b89b-4e6ed158e5b9@linux.alibaba.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Rspam-User: X-Stat-Signature: njz661ryipusyo6g38xz5myjq4py9efh X-Rspamd-Queue-Id: 29091160006 X-Rspamd-Server: rspam11 X-HE-Tag: 1724918854-362030 X-HE-Meta: U2FsdGVkX1851Fuj2MdDglocnX2AtyItdEnEpivr8utiOehZNZHYjTRtkA8QONltQ7apX0ksyq01825gqoDPLi+fLmzLopY2bVMdleMOMkn1KaCy/KbCX+1PxLjIzDS2t2hz2T4a4ImaplVHZuord6upkDmf7C/+p3NHroLFcCt8zhwC+zSAvoZGSXuYq7vTrDVhb/jCby9SUz1fCnGUSOBrCU3KFcDuC67JzgFJftVOF1zxwWT4MRlC2nTdWZ3zC4iRVhsIn9bdcV7+gE6ZdlanEIgY9zntdD7qBOf681/YvjdFolLuptI6R2Z1D/xeYPyhqn+maG6zvnNTy6aDFPwSIOvXhSF3N2bTEjb01E0TkcN5jvMvtAAhW1UXxDpfORrqJSCFTkeXW//GU7anz0hSMnvovFh/UnhCXz3we1nccDVsCZGCjq4X5juQI5tX6ko2b1q08UdJcYzM4ioKePas6KWXJa7LZr5VKNfDIZuv0eo1iWta57MCRd7tzF+R8f8+Q6PdHgup/IxTmzBKYWSvIhlRP34GHn+FGch4NRXA7GUydhhzgHTr9C99mf4+tUtYma+yJ9ABo0PVFk+F2MFGLWvzbYziaka8oCuJdQjcDv6ZRuika7+75mr0Zke/K+cHBFd6jDB3oini8EKfnEOmMfADS+mfllMaiuzSvWgQGZMZsXQRfdtqfqmBl2SJmt6Stmotl1INwgtQ7p0znLLDNmcZQ5l1pVcGeW62dENhOPQgAhDFI1LpjMMfpnUaX4ZvDqlUHmgL2oP7shSduxIXR7v50OuTOvQ8WicKOXgFwzOnPw6y/46jzmq4LGZtOW8jTBHF/jNpIaW+m0/wOXE0zIUzTw5ep3EK6WX5OTWJBj5qHZSBZRFY3YD8hyYQkLBOSdjHLiNi2atUmoOfsOaizSHwCGMeJm4EqT9pHXbm6wgyXpBxDGZwIn4ED8MV/3oqqWF3BMuAe0Jc5h5 GUt/au4/ toS8yMJG20c4M2Xe1ig/+gViB2QlU0Yy94PiGze/F5PHz6lQHihAuFgtCLIHqfzpdmV2oguPGPcjUO9kDKdOVueXSXtw3BbKUOCJDpTYtrrcYu5uMN5w9wD0rT4Kg6BTqGQxyzdhyDYQJMjdER2t5HU0MTYNBsodUsFhEkPoySfyHW66JlyUK5M5OULwHcTYefHQkUEcVbVjM13PTepWJCCSNiHt/jqeoT0N4SDbmdsikzscm+HbpWo90D7Yan5hkdkHRNe99nSgnaUx5NRaHw9V7R0DvlQZyhOByics3+92swMwC/+Bk8qQox/URqMKlD5WBj54WPFAjQxODCzatnUPvgZxy8cqGrbiVGbVGLCAkSyc7XDhNTxH+eW3MD2bztQyx+FZ+gLSRox/9bwgei9eqVWrs1ngV+kW6bNbATWV6iq0= 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: List-Subscribe: List-Unsubscribe: On Tue, 27 Aug 2024, Baolin Wang wrote: > On 2024/8/26 05:55, Hugh Dickins wrote: > > On Mon, 12 Aug 2024, Baolin Wang wrote: > > > >> In the following patches, shmem will support the swap out of large folios, > >> which means the shmem mappings may contain large order swap entries, so > >> using xa_get_order() to get the folio order of the shmem swap entry to > >> update the '*start' correctly. > >> > >> Signed-off-by: Baolin Wang > >> --- > >> mm/filemap.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/mm/filemap.c b/mm/filemap.c > >> index 4130be74f6fd..4c312aab8b1f 100644 > >> --- a/mm/filemap.c > >> +++ b/mm/filemap.c > >> @@ -2056,6 +2056,8 @@ unsigned find_get_entries(struct address_space > >> *mapping, pgoff_t *start, > >> folio = fbatch->folios[idx]; > >> if (!xa_is_value(folio)) > >> nr = folio_nr_pages(folio); > >> + else > >> + nr = 1 << xa_get_order(&mapping->i_pages, > >> indices[idx]); > >> *start = indices[idx] + nr; > >> } > >> return folio_batch_count(fbatch); > >> @@ -2120,6 +2122,8 @@ unsigned find_lock_entries(struct address_space > >> *mapping, pgoff_t *start, > >> folio = fbatch->folios[idx]; > >> if (!xa_is_value(folio)) > >> nr = folio_nr_pages(folio); > >> + else > >> + nr = 1 << xa_get_order(&mapping->i_pages, > >> indices[idx]); > >> *start = indices[idx] + nr; > >> } > >> return folio_batch_count(fbatch); > >> -- > > > > Here we have a problem, but I'm not suggesting a fix for it yet: I > > need to get other fixes out first, then turn to thinking about this - > > it's not easy. > > Thanks for raising the issues. > > > > > That code is almost always right, so it works well enough for most > > people not to have noticed, but there are two issues with it. > > > > The first issue is that it's assuming indices[idx] is already aligned > > to nr: not necessarily so. I believe it was already wrong in the > > folio_nr_pages() case, but the time I caught it wrong with a printk > > was in the swap (value) case. (I may not be stating this correctly: > > again more thought needed but I can't spend so long writing.) > > > > And immediately afterwards that kernel build failed with a corrupted > > (all 0s) .o file - I'm building on ext4 on /dev/loop0 on huge tmpfs while > > swapping, and happen to be using the "-o discard" option to ext4 mount. > > > > I've been chasing these failures (maybe a few minutes in, maybe half an > > hour) for days, then had the idea of trying without the "-o discard": > > whereupon these builds can be repeated successfully for many hours. > > IIRC ext4 discard to /dev/loop0 entails hole-punch to the tmpfs. > > > > The alignment issue can easily be corrected, but that might not help. > > (And those two functions would look better with the rcu_read_unlock() > > moved down to just before returning: but again, wouldn't really help.) > > > > The second issue is that swap is more slippery to work with than > > folios or pages: in the folio_nr_pages() case, that number is stable > > because we hold a refcount (which stops a THP from being split), and > > later we'll be taking folio lock too. None of that in the swap case, > > so (depending on how a large entry gets split) the xa_get_order() result > > is not reliable. Likewise for other uses of xa_get_order() in this series. > > Now we have 2 users of xa_get_order() in this series: > > 1) shmem_partial_swap_usage(): this is acceptable, since racy results are not > a problem for the swap statistics. Yes: there might be room for improvement, but no big deal there. > > 2) shmem_undo_range(): when freeing a large swap entry, it will use > xa_cmpxchg_irq() to make sure the swap value is not changed (in case the large > swap entry is split). If failed to cmpxchg, then it will use current index to > retry in shmem_undo_range(). So seems not too bad? Right, I was missing the cmpxchg aspect. I am not entirely convinced of the safety in proceeding in this way, but I shouldn't spread FUD without justification. Today, no yesterday, I realized what might be the actual problem, and it's not at all these entry splitting races I had suspected. Fix below. Successful testing on mm-everything-2024-08-24-07-21 (well, that minus the commit which spewed warnings from bootup) confirmed it. But testing on mm-everything-2024-08-28-21-38 very quickly failed: unrelated to this series, presumably caused by patch or patches added since 08-24, one kind of crash on one machine (some memcg thing called from isolate_migratepages_block), another kind of crash on another (some memcg thing called from __read_swap_cache_async), I'm exhausted by now but will investigate later in the day (or hope someone else has). [PATCH] mm: filemap: use xa_get_order() to get the swap entry order: fix find_lock_entries(), used in the first pass of shmem_undo_range() and truncate_inode_pages_range() before partial folios are dealt with, has to be careful to avoid those partial folios: as its doc helpfully says, "Folios which are partially outside the range are not returned". Of course, the same must be true of any value entries returned, otherwise truncation and hole-punch risk erasing swapped areas - as has been seen. Rewrite find_lock_entries() to emphasize that, following the same pattern for folios and for value entries. Adjust find_get_entries() slightly, to get order while still holding rcu_read_lock(), and to round down the updated start: good changes, like find_lock_entries() now does, but it's unclear if either is ever important. Signed-off-by: Hugh Dickins --- mm/filemap.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/mm/filemap.c b/mm/filemap.c index 885a8ed9d00d..88a2ed008474 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2047,10 +2047,9 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start, if (!folio_batch_add(fbatch, folio)) break; } - rcu_read_unlock(); if (folio_batch_count(fbatch)) { - unsigned long nr = 1; + unsigned long nr; int idx = folio_batch_count(fbatch) - 1; folio = fbatch->folios[idx]; @@ -2058,8 +2057,10 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start, nr = folio_nr_pages(folio); else nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]); - *start = indices[idx] + nr; + *start = round_down(indices[idx] + nr, nr); } + rcu_read_unlock(); + return folio_batch_count(fbatch); } @@ -2091,10 +2092,17 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start, rcu_read_lock(); while ((folio = find_get_entry(&xas, end, XA_PRESENT))) { + unsigned long base; + unsigned long nr; + if (!xa_is_value(folio)) { - if (folio->index < *start) + nr = folio_nr_pages(folio); + base = folio->index; + /* Omit large folio which begins before the start */ + if (base < *start) goto put; - if (folio_next_index(folio) - 1 > end) + /* Omit large folio which extends beyond the end */ + if (base + nr - 1 > end) goto put; if (!folio_trylock(folio)) goto put; @@ -2103,7 +2111,19 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start, goto unlock; VM_BUG_ON_FOLIO(!folio_contains(folio, xas.xa_index), folio); + } else { + nr = 1 << xa_get_order(&mapping->i_pages, xas.xa_index); + base = xas.xa_index & ~(nr - 1); + /* Omit order>0 value which begins before the start */ + if (base < *start) + continue; + /* Omit order>0 value which extends beyond the end */ + if (base + nr - 1 > end) + break; } + + /* Update start now so that last update is correct on return */ + *start = base + nr; indices[fbatch->nr] = xas.xa_index; if (!folio_batch_add(fbatch, folio)) break; @@ -2115,17 +2135,6 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start, } rcu_read_unlock(); - if (folio_batch_count(fbatch)) { - unsigned long nr = 1; - int idx = folio_batch_count(fbatch) - 1; - - folio = fbatch->folios[idx]; - if (!xa_is_value(folio)) - nr = folio_nr_pages(folio); - else - nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]); - *start = indices[idx] + nr; - } return folio_batch_count(fbatch); } -- 2.35.3