Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Sep 2022 17:14:34 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Jason Gunthorpe <jgg@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     <akpm@linux-foundation.org>, Matthew Wilcox <willy@infradead.org>,
        "Jan Kara" <jack@suse.cz>, "Darrick J. Wong" <djwong@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        John Hubbard <jhubbard@nvidia.com>,
        <linux-fsdevel@vger.kernel.org>, <nvdimm@lists.linux.dev>,
        <linux-xfs@vger.kernel.org>, <linux-mm@kvack.org>,
        <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH v2 10/18] fsdax: Manage pgmap references at entry
 insertion and deletion
Message-ID: <632ba8eaa5aea_349629422@dwillia2-xfh.jf.intel.com.notmuch>
References: <166329930818.2786261.6086109734008025807.stgit@dwillia2-xfh.jf.intel.com>
 <166329936739.2786261.14035402420254589047.stgit@dwillia2-xfh.jf.intel.com>
 <YysZrdF/BSQhjWZs@nvidia.com>
 <632b2b4edd803_66d1a2941a@dwillia2-xfh.jf.intel.com.notmuch>
 <632b8470d34a6_34962946d@dwillia2-xfh.jf.intel.com.notmuch>
 <YyuLLsindwo0prz4@nvidia.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YyuLLsindwo0prz4@nvidia.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9dj3OK97812DQpl9yXnlfpT+2oL/GSbHAmQAiWxh+FvMqnrkJHBLVssSzRae?=
 =?us-ascii?Q?l5WzQpCJ8Tb4DmBAceE9hym9OaI+agw0F/oHp7m/H8SnioTBEK5ZpqZf6Opq?=
 =?us-ascii?Q?+scTm+PEKVDlyV56UL5U02qSvu09psXNCkjwehFhBj5cdKSabG65zg4Lv8Gf?=
 =?us-ascii?Q?U4x0tyX1zM5Xi9iz7jCZ+NgOnFWfO6FNbw3Sam3JGlZm3pimPHR0lp7E6IM5?=
 =?us-ascii?Q?UGKEv9ii/sKo12/rGkYVthS1teu6h2/UpN2h3v7s2YNbFqjD9oE7DvAnP/0+?=
 =?us-ascii?Q?DjgHshOppl75NlAUMvRQ/BKNnBGK+7X+AsvaoremYdyO5GcWbjSIdO9BhBFs?=
 =?us-ascii?Q?PRAq3GAKp2SXNtdUELrBmN9UFElFG9MicQ3+U0Gd0jQLBUwlgbIKGGXKL0WU?=
 =?us-ascii?Q?p8bZvJTBwNBibt6z8H9UmlhNibEAct/sxhiznuCkolU7gnIhlZRukNNbfT6w?=
 =?us-ascii?Q?KDfotkPSYq/HrbY6Fg31jngLyDyKd5HhGeUgDcqJJSyBs/qzhhqwxG4r/tzk?=
 =?us-ascii?Q?BOfR2eMarGBVYhiYRb43+cTRAX19dvQUY7nJd0Cm5i6wFm+G14QSAhH1Op1H?=
 =?us-ascii?Q?OzTVjINuJT8adv4uAJkPpgYkiUF2AiQFDxjHjbfpcoUzS2IObn5xu3ln03wx?=
 =?us-ascii?Q?NNiiipjMtG9j2p2wBPDPIUK+VtOnUe6jam0EDKTW9NnXexaGAXodond5IITI?=
 =?us-ascii?Q?qnN3kAw1lNj1DdYNlr0eJJ2YRW/ZvSRDKNvKIc/eTzZfK0UXqUJr/FTIUU30?=
 =?us-ascii?Q?F7wAefQX36mLMrrwoyYIqvoPu8BsDDkJ8kWdqXy9IM+ZbKw622ga+wJGTu8+?=
 =?us-ascii?Q?LFqrDWmZa9SiEc7w6RdeiWgVB/Dilmf9e/2sQIKafM2BBRW4mAfNK5qDOma0?=
 =?us-ascii?Q?hDNv62gzagqB/iFSvkXWy2bAYqDji27lXfFjAs4SsdUx+mp+ymHTjOI24adB?=
 =?us-ascii?Q?Not5PBTJ+JQw/a0kHYBuB65bYks2K+R8+jooqiTTVHNYspmL3xQ9cfDBpLiz?=
 =?us-ascii?Q?wzZ4gaicj1aQp6PcHEAmdGTNbAJD8SMTw4nwV67q0MsTpjaMIjljDBFxMtTR?=
 =?us-ascii?Q?GF7VAVIka/Zl5/lqhS4qb8oWGxGiG+uOld/COYc8xm0T9mDMWxFJSWX0HJFz?=
 =?us-ascii?Q?7wjxY1Y0s/XE5Yv+vUWBwRihfRDQu7+2yXx9YT+H8WCYR7IXSx8qRu83jvRf?=
 =?us-ascii?Q?+xlynsLHvki/EEXu8ukr3tOwi4W/b+IHo+x/LMV7/+NnJ7ajB8U8nf7bnszs?=
 =?us-ascii?Q?FYwFs6oOSHBkcYfYbubH0K9F0nGI+SR+8D1RHQre3k+mneVbU5LMG3dK6fD8?=
 =?us-ascii?Q?jYxisV8YFxY/cp8QKBIwHxVs2OgJDcOCnDTtoXe5PCK9MD+jBiEQGuqOzeQy?=
 =?us-ascii?Q?Hiz955bImyq8TyVfAjQ2FKfctV/9qPYPvP5ho2Qw6FGL+hB6f1MSKXrd+uqY?=
 =?us-ascii?Q?prnJJrcc6FGeKOQToYYrWaDHAU3rpBwqANoRRrb4VhCHD4iKvdekwmpbILeM?=
 =?us-ascii?Q?mr1LSZWLP97VAMmE5Tji7w1dC86VqqblEdfNfA7B152GzBg6xmuUGssmkzZH?=
 =?us-ascii?Q?SwLS0Ggb1sgr+Os3dcH0m+WL9kuIUQ4WbnS1JFEBJuCTf/FFXXtjAjYqfUP0?=
 =?us-ascii?Q?iw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 668e8a77-7104-4f0f-388b-08da9c2f6fdb
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2022 00:14:38.1473
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9scufQYP3aOyhMw/NnJAkmxmNfpJYhEaqXqfChH6kJNaQhB7GHlMtjsIPiQe3sSVV1SMqAwWIkoGt5eu8BtHzITYsVRgwcamXX9QYyPIunY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6365
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

Jason Gunthorpe wrote:
> On Wed, Sep 21, 2022 at 02:38:56PM -0700, Dan Williams wrote:
> > Dan Williams wrote:
> > > Jason Gunthorpe wrote:
> > > > On Thu, Sep 15, 2022 at 08:36:07PM -0700, Dan Williams wrote:
> > > > > The percpu_ref in 'struct dev_pagemap' is used to coordinate active
> > > > > mappings of device-memory with the device-removal / unbind path. It
> > > > > enables the semantic that initiating device-removal (or
> > > > > device-driver-unbind) blocks new mapping and DMA attempts, and waits for
> > > > > mapping revocation or inflight DMA to complete.
> > > > 
> > > > This seems strange to me
> > > > 
> > > > The pagemap should be ref'd as long as the filesystem is mounted over
> > > > the dax. The ref should be incrd when the filesystem is mounted and
> > > > decrd when it is unmounted.
> > > > 
> > > > When the filesystem unmounts it should zap all the mappings (actually
> > > > I don't think you can even unmount a filesystem while mappings are
> > > > open) and wait for all page references to go to zero, then put the
> > > > final pagemap back.
> > > > 
> > > > The rule is nothing can touch page->pgmap while page->refcount == 0,
> > > > and if page->refcount != 0 then page->pgmap must be valid, without any
> > > > refcounting on the page map itself.
> > > > 
> > > > So, why do we need pgmap refcounting all over the place? It seems like
> > > > it only existed before because of the abuse of the page->refcount?
> > > 
> > > Recall that this percpu_ref is mirroring the same function as
> > > blk_queue_enter() whereby every new request is checking to make sure the
> > > device is still alive, or whether it has started exiting.
> > > 
> > > So pgmap 'live' reference taking in fs/dax.c allows the core to start
> > > failing fault requests once device teardown has started. It is a 'block
> > > new, and drain old' semantic.
> 
> It is weird this email never arrived for me..
> 
> I think that is all fine, but it would be much more logically
> expressed as a simple 'is pgmap alive' call before doing a new mapping
> than mucking with the refcount logic. Such a test could simply
> READ_ONCE a bool value in the pgmap struct.
> 
> Indeed, you could reasonably put such a liveness test at the moment
> every driver takes a 0 refcount struct page and turns it into a 1
> refcount struct page.

I could do it with a flag, but the reason to have pgmap->ref managed at
the page->_refcount 0 -> 1 and 1 -> 0 transitions is so at the end of
time memunmap_pages() can look at the one counter rather than scanning
and rescanning all the pages to see when they go to final idle.