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 5CDACC43334 for ; Fri, 8 Jul 2022 02:50:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 648A16B0071; Thu, 7 Jul 2022 22:50:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 56320900002; Thu, 7 Jul 2022 22:50:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 403376B0071; Thu, 7 Jul 2022 22:50:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 24B556B0071 for ; Thu, 7 Jul 2022 22:50:34 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id EA5C933E24 for ; Fri, 8 Jul 2022 02:50:33 +0000 (UTC) X-FDA: 79662404346.15.F4CCE37 Received: from mail-qt1-f180.google.com (mail-qt1-f180.google.com [209.85.160.180]) by imf11.hostedemail.com (Postfix) with ESMTP id 7F0A640040 for ; Fri, 8 Jul 2022 02:50:33 +0000 (UTC) Received: by mail-qt1-f180.google.com with SMTP id bs20so25589545qtb.11 for ; Thu, 07 Jul 2022 19:50:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:in-reply-to:message-id:references :mime-version; bh=Ad97sT16le5gjY7PgDU6gmeetvBzm9wlE8bQkILVjNc=; b=tCcxRqjITVsiKb1O2qN38oOtehuQYEQmQlnA5EmpLLTcqVHGWkBNn5r6VjhB95uftz oBhwab0Wa2p2hfzUf6HitqoN4V8My4lx3Sr3LHdaGn7TrTI27nd6mE0wsQOIBv87ES7u uwlTgHRa2iafBHAxVTfOiHdPWVETU8CJjwRGskRUwQecY/1YbABG8fpWqJ0i5forgO17 n0R7m15mcAmskoYtfNfHUvAotB5BgPlp6Llxsn+Fa0qYPR44yx2cfMCeEIswKRQZAgPI NLOhCVeNcKdBjtuIw/nnF9iOSfGliNNxy/YqmZphg0HMl4dZzroFochhJCRrtxDrneBV hZ4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=Ad97sT16le5gjY7PgDU6gmeetvBzm9wlE8bQkILVjNc=; b=nO+7Xy75200gtR3gawgJaAsK+G+jnnTTXe9E8rP8SRfsw1S5boA79917X6c3AELKHB bErh8Dpgeq1mPkaF2nkuegIfTkcck7ER+4GmcW3KSc4by4oEbVs/8nHWQDGqQMsIpHwM 3AdODeyx7rcsKg+humelWP+jtSDACgybVr+KgAwymYT2NXhhSpFz6M5mJJ/SLERommyQ 3rdm9KHS0Jn+QZFCCHb74Ibsn8suBIHcEpcIPQLwSarcTu+Rktx5ym4q9orALAqrAjb7 7YmBtOmwFzVeNGqehUrwOPzN7MNA/cbQfGMunVN3KRTwzI/dPfUeNtfmIeZWmlg4UQZL j+yA== X-Gm-Message-State: AJIora9P8z0iOKEi1sObNkVeDWmKsNCATiMi2OVZ2rvnPGB1yVF1RPcp r8dHa5IJh6zeYNT5T9U8I65JNg== X-Google-Smtp-Source: AGRyM1tJqq+vsbrqVwAmw8UZQYXdEmN7Cq4ADeDpayYOUxqHMEno4PbopiKhNsYF91bJVygTm51HMQ== X-Received: by 2002:ac8:7d52:0:b0:319:51f0:e418 with SMTP id h18-20020ac87d52000000b0031951f0e418mr1088284qtb.481.1657248632632; Thu, 07 Jul 2022 19:50:32 -0700 (PDT) Received: from ripple.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id s7-20020a05620a254700b006a65c58db99sm35676841qko.64.2022.07.07.19.50.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Jul 2022 19:50:32 -0700 (PDT) Date: Thu, 7 Jul 2022 19:50:17 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@ripple.anvils To: "Matthew Wilcox (Oracle)" cc: Andrew Morton , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-aio@kvack.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, linux-mm@kvack.org, linux-xfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-ntfs-dev@lists.sourceforge.net, ocfs2-devel@oss.oracle.com, linux-mtd@lists.infradead.org, virtualization@lists.linux-foundation.org, Christoph Hellwig Subject: Re: [PATCH v2 07/19] mm/migrate: Convert expected_page_refs() to folio_expected_refs() In-Reply-To: <20220608150249.3033815-8-willy@infradead.org> Message-ID: <6e7599d1-8a5f-bf16-383c-febd753bd051@google.com> References: <20220608150249.3033815-1-willy@infradead.org> <20220608150249.3033815-8-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1657248633; 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=Ad97sT16le5gjY7PgDU6gmeetvBzm9wlE8bQkILVjNc=; b=wcTMGBvpo+GFIrtykIvrapAYibQ2BxMbeT+o3IycZt3vPY5pvvzLEFHeEW1c75gwMtw1Pb 8Z3kiatoS/PNyqlJdcgtOKLqEV8SvTHaIK5u+kEjyLuWmY+N0WmnIUU+3JiBopCitXT7Tp 6Lrq3UfgWd3lcsHEmo2eYTaTH8Zs1S0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1657248633; a=rsa-sha256; cv=none; b=Df+mjWa0HqDsB73/YBzOMXpL3om9m77lZUHoR+Be0pyqdRpTM8VXPSc2yLsTNiw4J4Cjzw We7+jrMYPCEzP95z8EifmjUjei8ExVn0GGJ0BBsqRzpxEQ4b5IUNX2SnOOC8j0MXJMutVi OdWD+EnbeS7hcVX5Y8/B08AT3KZXnCw= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=tCcxRqjI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf11.hostedemail.com: domain of hughd@google.com designates 209.85.160.180 as permitted sender) smtp.mailfrom=hughd@google.com Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=tCcxRqjI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf11.hostedemail.com: domain of hughd@google.com designates 209.85.160.180 as permitted sender) smtp.mailfrom=hughd@google.com X-Rspamd-Server: rspam02 X-Stat-Signature: r9xxb7ys4mtuogzytykijyhm47cj64rw X-Rspamd-Queue-Id: 7F0A640040 X-Rspam-User: X-HE-Tag: 1657248633-678593 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 Wed, 8 Jun 2022, Matthew Wilcox (Oracle) wrote: > Now that both callers have a folio, convert this function to > take a folio & rename it. > > Signed-off-by: Matthew Wilcox (Oracle) > Reviewed-by: Christoph Hellwig > --- > mm/migrate.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 2975f0c4d7cf..2e2f41572066 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -336,13 +336,18 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd) > } > #endif > > -static int expected_page_refs(struct address_space *mapping, struct page *page) > +static int folio_expected_refs(struct address_space *mapping, > + struct folio *folio) > { > - int expected_count = 1; > + int refs = 1; > + if (!mapping) > + return refs; > > - if (mapping) > - expected_count += compound_nr(page) + page_has_private(page); > - return expected_count; > + refs += folio_nr_pages(folio); > + if (folio_get_private(folio)) > + refs++; > + > + return refs; > } > > /* > @@ -359,7 +364,7 @@ int folio_migrate_mapping(struct address_space *mapping, > XA_STATE(xas, &mapping->i_pages, folio_index(folio)); > struct zone *oldzone, *newzone; > int dirty; > - int expected_count = expected_page_refs(mapping, &folio->page) + extra_count; > + int expected_count = folio_expected_refs(mapping, folio) + extra_count; > long nr = folio_nr_pages(folio); > > if (!mapping) { > @@ -669,7 +674,7 @@ static int __buffer_migrate_folio(struct address_space *mapping, > return migrate_page(mapping, &dst->page, &src->page, mode); > > /* Check whether page does not have extra refs before we do more work */ > - expected_count = expected_page_refs(mapping, &src->page); > + expected_count = folio_expected_refs(mapping, src); > if (folio_ref_count(src) != expected_count) > return -EAGAIN; > > -- > 2.35.1 This commit (742e89c9e352d38df1a5825fe40c4de73a5d5f7a in pagecache.git folio/for-next and recent linux-next) is dangerously wrong, at least for swapcache, and probably for some others. I say "dangerously" because it tells page migration a swapcache page is safe for migration when it certainly is not. The fun that typically ensues is kernel BUG at include/linux/mm.h:750! put_page_testzero() VM_BUG_ON_PAGE(page_ref_count(page) == 0, page), if CONFIG_DEBUG_VM=y (bisecting for that is what brought me to this). But I guess you might get silent data corruption too. I assumed at first that you'd changed the rules, and were now expecting any subsystem that puts a non-zero value into folio->private to raise its refcount - whereas the old convention (originating with buffer heads) is that setting PG_private says an extra refcount has been taken, please call try_to_release_page() to lower it, and maybe that will use data in page->private to do so; but page->private free for the subsystem owning the page to use as it wishes, no refcount implication. But that you had missed updating swapcache. So I got working okay with the patch below; but before turning it into a proper patch, noticed that there were still plenty of other places applying the test for PG_private: so now think that maybe you set out with intention as above, realized it wouldn't work, but got distracted before cleaning up some places you'd already changed. And patch below now goes in the wrong direction. Or maybe you didn't intend any change, but the PG_private test just got missed in a few places. I don't know, hope you remember, but current linux-next badly inconsistent. Over to you, thanks, Hugh --- a/mm/migrate.c 2022-07-06 14:24:44.499941975 -0700 +++ b/mm/migrate.c 2022-07-06 15:49:25.000000000 -0700 @@ -351,6 +351,10 @@ unlock: } #endif +static inline bool folio_counted_private(struct folio *folio) +{ + return !folio_test_swapcache(folio) && folio_get_private(folio); +} static int folio_expected_refs(struct address_space *mapping, struct folio *folio) { @@ -359,7 +363,7 @@ static int folio_expected_refs(struct ad return refs; refs += folio_nr_pages(folio); - if (folio_get_private(folio)) + if (folio_counted_private(folio)) refs++; return refs; --- a/mm/vmscan.c 2022-07-06 14:24:44.531942217 -0700 +++ b/mm/vmscan.c 2022-07-06 15:49:37.000000000 -0700 @@ -2494,6 +2494,10 @@ shrink_inactive_list(unsigned long nr_to * The downside is that we have to touch folio->_refcount against each folio. * But we had to alter folio->flags anyway. */ +static inline bool folio_counted_private(struct folio *folio) +{ + return !folio_test_swapcache(folio) && folio_get_private(folio); +} static void shrink_active_list(unsigned long nr_to_scan, struct lruvec *lruvec, struct scan_control *sc, @@ -2538,8 +2542,9 @@ static void shrink_active_list(unsigned } if (unlikely(buffer_heads_over_limit)) { - if (folio_get_private(folio) && folio_trylock(folio)) { - if (folio_get_private(folio)) + if (folio_counted_private(folio) && + folio_trylock(folio)) { + if (folio_counted_private(folio)) filemap_release_folio(folio, 0); folio_unlock(folio); }