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=-5.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 C73D2C433E0 for ; Wed, 17 Mar 2021 02:12:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2AC3664F91 for ; Wed, 17 Mar 2021 02:12:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2AC3664F91 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=toxicpanda.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B11618D0001; Tue, 16 Mar 2021 22:12:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AC0816B0071; Tue, 16 Mar 2021 22:12:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 913C48D0001; Tue, 16 Mar 2021 22:12:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0193.hostedemail.com [216.40.44.193]) by kanga.kvack.org (Postfix) with ESMTP id 727CC6B0070 for ; Tue, 16 Mar 2021 22:12:25 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 3C687180AD822 for ; Wed, 17 Mar 2021 02:12:25 +0000 (UTC) X-FDA: 77927741850.12.C0713BE Received: from mail-qv1-f46.google.com (mail-qv1-f46.google.com [209.85.219.46]) by imf20.hostedemail.com (Postfix) with ESMTP id AD295138 for ; Wed, 17 Mar 2021 02:12:24 +0000 (UTC) Received: by mail-qv1-f46.google.com with SMTP id o19so751402qvu.0 for ; Tue, 16 Mar 2021 19:12:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=toxicpanda-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=KA0cQX0H+fedUmZSqotKHkFt9nfBS2lV4mSDduYC1Vo=; b=CfwgdSSKwnQ510ZsN0+CUYSCMnJvGTNOF0qgo3PkrJT/Da/ZELVOKWL/gvdjXbwGIJ +O24SS0hCf6GGdRpdBSk7N5NbLn2iWfl25SUnaP3oNJ7WiJXrG4UqOuQVtPZMqv1UvLq 1k9Y29haYQb7OJLIFpq9Ca/2lmePu+tE/YxOqCC7/pjN0ZJm/td3809+dpAOb2cIbaOB JIelDCwL+8ebDLqZ3W53s0y9QKt1RRSRn3jACZJpraJ6VtSZoPGPLIHKRuVw6WXLF8mu NSaPFQU3F6l0ReQyu9G5Yv+r+f2QbaVUKOhZkjsn1XroEHm2K1q3C9v7SyUY5WN5Z7ln hDeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KA0cQX0H+fedUmZSqotKHkFt9nfBS2lV4mSDduYC1Vo=; b=bt6Za5vfyhqAB8A39AcZqbxCMKqA3i2IqXwZvI8VxzrmWlMb4xl/BzJHQyugDdHvZT GGcxp13K0xO5BkVbyFNJtAmshAvCuuWyeUdvO1ArLi9aKSXtiw6j7xSMsIRaSf9EP2Tt 62PUczEZ6YJJKgKCBEuo01TNQIm0UW+PGYAA38x5vQt+ZD4FTxsxNHBYY9OStKUlGMXb tD+MHiXmmGYVaomoyKqoF+sRgF9eB9PwDTP6uz3t1vWa46ZzaqG7hZvi6wMqpfhzZILI ZtRKNrSwqCzP4Lel9Rax1yfBrpxt4v6UNUMUNOobdf+IGYcnMQ7XIY3hGusRGuBfGw9n QrdA== X-Gm-Message-State: AOAM532fStsJkyhHUx08digxPm7GIxGNMFWU2dcG8m00pZlwNZ70KAyt 9lJWzqM++rkF5HYCVEPGd5HyYA== X-Google-Smtp-Source: ABdhPJyCVi9KcQuRC+jk9afKmHcOxIGMxGHzjm2jLRzMICkswTbEAhqzEZ42z+xpCoNkmkSraZlwfQ== X-Received: by 2002:a05:6214:1744:: with SMTP id dc4mr2746704qvb.40.1615947143864; Tue, 16 Mar 2021 19:12:23 -0700 (PDT) Received: from [192.168.1.45] (cpe-174-109-172-136.nc.res.rr.com. [174.109.172.136]) by smtp.gmail.com with ESMTPSA id r3sm16393336qkm.129.2021.03.16.19.12.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Mar 2021 19:12:23 -0700 (PDT) Subject: Re: [PATCH v4 02/28] mm: Add an unlock function for PG_private_2/PG_fscache To: Linus Torvalds , Matthew Wilcox , Chris Mason , David Sterba Cc: David Howells , Trond Myklebust , Anna Schumaker , Steve French , Dominique Martinet , Christoph Hellwig , Alexander Viro , Linux-MM , linux-cachefs@redhat.com, linux-afs@lists.infradead.org, "open list:NFS, SUNRPC, AND..." , CIFS , ceph-devel@vger.kernel.org, v9fs-developer@lists.sourceforge.net, linux-fsdevel , Jeff Layton , David Wysochanski , Linux Kernel Mailing List References: <161539526152.286939.8589700175877370401.stgit@warthog.procyon.org.uk> <161539528910.286939.1252328699383291173.stgit@warthog.procyon.org.uk> <20210316190707.GD3420@casper.infradead.org> From: Josef Bacik Message-ID: <887b9eb7-2764-3659-d0bf-6a034a031618@toxicpanda.com> Date: Tue, 16 Mar 2021 22:12:21 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: AD295138 X-Stat-Signature: e74q9bz5wq1r9uf31mzwe9keuns6zt1s Received-SPF: none (toxicpanda.com>: No applicable sender policy available) receiver=imf20; identity=mailfrom; envelope-from=""; helo=mail-qv1-f46.google.com; client-ip=209.85.219.46 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1615947144-490426 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 3/16/21 8:43 PM, Linus Torvalds wrote: > [ Adding btrfs people explicitly, maybe they see this on the fs-devel > list, but maybe they don't react .. ] > > On Tue, Mar 16, 2021 at 12:07 PM Matthew Wilcox wrote: >> >> This isn't a problem with this patch per se, but I'm concerned about >> private2 and expected page refcounts. > > Ugh. You are very right. > > It would be good to just change the rules - I get the feeling nobody > actually depended on them anyway because they were _so_ esoteric. > >> static inline int is_page_cache_freeable(struct page *page) >> { >> /* >> * A freeable page cache page is referenced only by the caller >> * that isolated the page, the page cache and optional buffer >> * heads at page->private. >> */ >> int page_cache_pins = thp_nr_pages(page); >> return page_count(page) - page_has_private(page) == 1 + page_cache_pins; > > You're right, that "page_has_private()" is really really nasty. > > The comment is, I think, the traditional usage case, which used to be > about page->buffers. Obviously these days it is now about > page->private with PG_private set, pointing to buffers > (attach_page_private() and detach_page_private()). > > But as you point out: > >> #define PAGE_FLAGS_PRIVATE \ >> (1UL << PG_private | 1UL << PG_private_2) >> >> So ... a page with both flags cleared should have a refcount of N. >> A page with one or both flags set should have a refcount of N+1. > > Could we just remove the PG_private_2 thing in this context entirely, > and make the rule be that > > (a) PG_private means that you have some local private data in > page->private, and that's all that matters for the "freeable" thing. > > (b) PG_private_2 does *not* have the same meaning, and has no bearing > on freeability (and only the refcount matters) > > I _)think_ the btrfs behavior is to only use PagePrivate2() when it > has a reference to the page, so btrfs doesn't care? > > I think fscache is already happy to take the page count when using > PG_private_2 for locking, exactly because I didn't want to have any > confusion about lifetimes. But this "page_has_private()" math ends up > meaning it's confusing anyway. > > btrfs people? What are the semantics for PG_private_2? Is it just a > flag, and you really don't want it to have anything to do with any > page lifetime decisions? Or? > Yeah it's just a flag, we use it to tell that the page is part of a range that has been allocated for IO. The lifetime of the page is independent of the page, but is generally either dirty or under writeback, so either it goes through truncate and we clear PagePrivate2 there, or it actually goes through IO and is cleared before we drop the page in our endio. We _always_ have PG_private set on the page as long as we own it, and PG_private_2 is only set in this IO related context, so we're safe there because of the rules around PG_dirty/PG_writeback. We don't need it to have an extra ref for it being set. Thanks, Josef