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;
References: <632b8470d34a6_34962946d@dwillia2-xfh.jf.intel.com.notmuch>
 <YyuLLsindwo0prz4@nvidia.com>
 <632ba8eaa5aea_349629422@dwillia2-xfh.jf.intel.com.notmuch>
 <YyurdXnW7SyEndHV@nvidia.com>
 <632bc5c4363e9_349629486@dwillia2-xfh.jf.intel.com.notmuch>
 <YyyhrTxFJZlMGYY6@nvidia.com>
 <632cd9a2a023_3496294da@dwillia2-xfh.jf.intel.com.notmuch>
 <Yy2ziac3GdHrpxuh@nvidia.com>
 <632ddeffd86ff_33d629490@dwillia2-xfh.jf.intel.com.notmuch>
 <Yy3wA7/bkza7NO1J@nvidia.com>
 <632e031958740_33d629428@dwillia2-xfh.jf.intel.com.notmuch>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Dan Williams <dan.j.williams@intel.com>
Cc:     Jason Gunthorpe <jgg@nvidia.com>, 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
Date:   Tue, 27 Sep 2022 16:07:05 +1000
In-reply-to: <632e031958740_33d629428@dwillia2-xfh.jf.intel.com.notmuch>
Message-ID: <8735cdl4pk.fsf@nvdebian.thelocal>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TAH10BDh5iSFUab6wUF8By6PKcLhxwiyyDpPNPP6z6FDlK5I4/j/g4z/AWWw?=
 =?us-ascii?Q?g57dDWeywTgn8j8qbWcz/aatKdm9ASiM+qE6wAUtxwZk4S/4xNJaDjELiBvL?=
 =?us-ascii?Q?u4qNbTr4Uv+Snh9RdGbi/ck1TZukACxL8+42mzA5kh58zFpjdhnhdVO0/ZTF?=
 =?us-ascii?Q?4EcNI1/EkSQVKObUbRQh0dHuvnv9JW6p+C7ACnA53A/Ey6Wq96fF12+r+UQx?=
 =?us-ascii?Q?pv5JBbWbzj5qCXgkH6ngwiJ7NEikAocbIT6leIU7GNc7xmROa2qLpgPSm/E6?=
 =?us-ascii?Q?oZP2pyVxlUzFTpDvy/PUZYxddkQsYP/+Y1qJ2UfWd8RbNt8RFzzijCVPdl5R?=
 =?us-ascii?Q?gyi+5OaUsr2P7MYQm0sSEJ7Be9LJVC2+7BqU4xlH4mY4YvNORE6JT51Vh+cp?=
 =?us-ascii?Q?rzfaCP/SgtMjyDe3510e4NVRYjXU2lrW9w2FtEdfVF8rGnW4qzQ0R00vkF96?=
 =?us-ascii?Q?IaU2rA1h4WivnD1ka+Qq1sBLv+WWgjQh+PLxuYtw/4PmGYv7+CHcHUUEwAnV?=
 =?us-ascii?Q?zsck1HWt+ggYHVcV+eiSr4dsjPt0wqMO7yRpobHXVZpWG8lbx/yQJkGveRnd?=
 =?us-ascii?Q?wxYPW7N65UZA092n6lFA/HWKhg619l0eIZAoPKEayKJK5rxcMVzbjthWe2KA?=
 =?us-ascii?Q?P55XRZGtlBEjaKN9TXF/zGfEefTJtX3iY0PgluKZtcXwLen7vpa3MrbapdMu?=
 =?us-ascii?Q?S4ZrwBoieq2MDMESMRS6WybZi6MrdHHKArahud7RolsSB1t/S4cOYCdt1X3b?=
 =?us-ascii?Q?OfKTYx5m9G6JxGKChTpaiVSmGuNduU/G+AYb9bLtNS5GNPP9RXQK1Pl/fvTQ?=
 =?us-ascii?Q?hfPO77he0wO5X6lLLyx9h6wUrBrDrf8VM47juEsOkrF5Zu7WKljXDTQq6GbS?=
 =?us-ascii?Q?T+nvFhEYjlZraVoLeAaajnr4tdqYs777Yk5PqXpylRSvZcNANEQ/eqVB23mP?=
 =?us-ascii?Q?3Baj4bZChtz2VebWyR2bsENKSPbnLQukS/FbZ7RiNkRv491qxINrPHPB0WRg?=
 =?us-ascii?Q?WvxLTpB3ik3Tj2NlQJ+51msye2lJT3kf/AVM8SK6WNURlBAY/diCuhBImMUJ?=
 =?us-ascii?Q?hxiZsYY8qjIb+YGHDcO+0++0HypoGuxkbffYYp6YbSGm2jx/E5K4w79vovPb?=
 =?us-ascii?Q?vqnxYaFPhL++A5fQXzBg45QRj9KWZxcl05W68LzFs3OdMR+rUTlnBSYi0oct?=
 =?us-ascii?Q?HqL2l9bdklr2bY0A+uZvVp0dCAU8qWFXX4s8rIa5pP1ZTT3ku1F9Z4RCGj6g?=
 =?us-ascii?Q?G0+rNR3yQp+2OSZWHd3ppZa/hrPLxKNG1/AO1RiQ/L6WUdM+EIm7BbORaN30?=
 =?us-ascii?Q?64r7vUArUkjJ87c0UiSZJluMYuME7hAZkXfyN0Miwl7WgtotGsC+jtC+Kus2?=
 =?us-ascii?Q?oCcatgU778+Mn0y6QLR/kP2jKZXfO/cbSsMt2f+Xz/g+aEMvnxRhL4YJXB84?=
 =?us-ascii?Q?KQXoKNNNAsO0Uw/dvB0UUrHSXFLuwc0EtpBY5pUOQns7OohgYQo0jZVlnHlM?=
 =?us-ascii?Q?6uNCD2KYNYO7QFJk3uuK+0zYp5AjUj/sIqhiZIfl5c7bzN1w4Ed/xnAsWLH3?=
 =?us-ascii?Q?sENdl2IPmoU98EoBd1XoZph8TROAkP8HrWRU2IPK?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5826157f-9fde-4ffb-d427-08daa0503964
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2022 06:19:24.8645
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2YDXtOYAG1RrKWkGKjftnOWfgkHwJvkQgJofFT1VVIlQoYCjpbbExSoyHllrRxvboWAZOYq7GT+GA/iUw2VBlw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4912
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no 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


Dan Williams <dan.j.williams@intel.com> writes:

> Jason Gunthorpe wrote:
>> On Fri, Sep 23, 2022 at 09:29:51AM -0700, Dan Williams wrote:
>> > > > /**
>> > > >  * pgmap_get_folio() - reference a folio in a live @pgmap by @pfn
>> > > >  * @pgmap: live pgmap instance, caller ensures this does not race @pgmap death
>> > > >  * @pfn: page frame number covered by @pgmap
>> > > >  */
>> > > > struct folio *pgmap_get_folio(struct dev_pagemap *pgmap,
>> > > > unsigned long pfn)
>>
>> Maybe should be not be pfn but be 'offset from the first page of the
>> pgmap' ? Then we don't need the xa_load stuff, since it cann't be
>> wrong by definition.
>>
>> > > > {
>> > > >         struct page *page;
>> > > >
>> > > >         VM_WARN_ONCE(pgmap != xa_load(&pgmap_array, PHYS_PFN(phys)));
>> > > >
>> > > >         if (WARN_ONCE(percpu_ref_is_dying(&pgmap->ref)))
>> > > >                 return NULL;
>> > >
>> > > This shouldn't be a WARN?
>> >
>> > It's a bug if someone calls this after killing the pgmap. I.e.  the
>> > expectation is that the caller is synchronzing this. The only reason
>> > this isn't a VM_WARN_ONCE is because the sanity check is cheap, but I do
>> > not expect it to fire on anything but a development kernel.
>>
>> OK, that makes sense
>>
>> But shouldn't this get the pgmap refcount here? The reason we started
>> talking about this was to make all the pgmap logic self contained so
>> that the pgmap doesn't pass its own destroy until all the all the
>> page_free()'s have been done.

That sounds good to me at least. I just noticed we introduced this exact
bug for device private/coherent pages when making their refcounts zero
based. Nothing currently takes pgmap->ref when a private/coherent page
is mapped. Therefore memunmap_pages() will complete and pgmap destroyed
while pgmap pages are still mapped.

So I think we need to call put_dev_pagemap() as part of
free_zone_device_page().

 - Alistair

>> > > > This does not create compound folios, that needs to be coordinated with
>> > > > the caller and likely needs an explicit
>> > >
>> > > Does it? What situations do you think the caller needs to coordinate
>> > > the folio size? Caller should call the function for each logical unit
>> > > of storage it wants to allocate from the pgmap..
>> >
>> > The problem for fsdax is that it needs to gather all the PTEs, hold a
>> > lock to synchronize against events that would shatter a huge page, and
>> > then build up the compound folio metadata before inserting the PMD.
>>
>> Er, at this point we are just talking about acquiring virgin pages
>> nobody else is using, not inserting things. There is no possibility of
>> conurrent shattering because, by definition, nothing else can
>> reference these struct pages at this instant.
>>
>> Also, the caller must already be serializating pgmap_get_folio()
>> against concurrent calls on the same pfn (since it is an error to call
>> pgmap_get_folio() on an non-free pfn)
>>
>> So, I would expect the caller must already have all the necessary
>> locking to accept maximally sized folios.
>>
>> eg if it has some reason to punch a hole in the contiguous range
>> (shatter the folio) it must *already* serialize against
>> pgmap_get_folio(), since something like punching a hole must know with
>> certainty if any struct pages are refcount != 0 or not, and must not
>> race with something trying to set their refcount to 1.
>
> Perhaps, I'll take a look. The scenario I am more concerned about is
> processA sets up a VMA of PAGE_SIZE and races processB to fault in the
> same filesystem block with a VMA of PMD_SIZE. Right now processA gets a
> PTE mapping and processB gets a PMD mapping, but the refcounting is all
> handled in small pages. I need to investigate more what is needed for
> fsdax to support folio_size() > mapping entry size.