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:   Fri, 23 Sep 2022 12:03:53 -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: <632e031958740_33d629428@dwillia2-xfh.jf.intel.com.notmuch>
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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yy3wA7/bkza7NO1J@nvidia.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?M3rYidYD6zWykUjoP56dAFZ7MtoB4e6N+nzSYNRdXuI8pLbP6xS+nNWtAjCP?=
 =?us-ascii?Q?vTfLsVmNqcoqEy/fTAvFMhTR7aO4jjuDHse7ECnPnsbNLzU5m8Zn2M+ZNtwd?=
 =?us-ascii?Q?sKw9IvkpEcr/diJVbH70hc+hfNf9897aKm3QppkQOLvgO9fZbYA3Zcc4llL4?=
 =?us-ascii?Q?uG5CnPE72jGw8NAwhfC46nXebJwFiaHvxy5030EapQmfSmCqNmq+CLXLeYbF?=
 =?us-ascii?Q?ZkcYrnGELzH89kMfIFyKaoo7Y3CUXXjDFHfyvddKHslSWblrdSDyUuwyOg/Y?=
 =?us-ascii?Q?5l4rd4+Z1nAI169lpXFmbPCHd2eB2RkmkHLsOJpZMwFDSK2PYXKMSVua4EXq?=
 =?us-ascii?Q?zRO4IEh/HMKXQ/6rJy+3lOqlxrIt7wHeHH5pjK8PlP2wJRIUFSthXIsX4uS6?=
 =?us-ascii?Q?tD+4DheF4izZZ1yO5ORmfO5VSxT2+FPzdyQGs24AtHuV8DeCrAqIPMP9KBPC?=
 =?us-ascii?Q?to/C7S/AXBdF0G7qIP3zent0RX/Ky588+eTvjXFckeml2aMazDTSjYDy9Vwr?=
 =?us-ascii?Q?N7jwU6NA0beJZsA9po73Qm6lmMPA7t89KrSDo5dUwMfiYWTJiZDqgMZCzlpk?=
 =?us-ascii?Q?+wbHfauY+HkTWb169hP4YiEsxxWjSlQiiD2y4WQSOXo7rosfpeV/HXNIvGdr?=
 =?us-ascii?Q?6w4iNue7YvRchkvQCdBhtt5A/6D5PBEF3Rk3Jux+D7RospZU26UYvDUdHpUK?=
 =?us-ascii?Q?ESi4cRGjiQYwhRtzc/HOQxAO1fUpudT7uif+JE8mofE6MQggbctQj446nlkS?=
 =?us-ascii?Q?CzVvZ41JFwLk220b5/6GbNsiH7ItdvRYzbHi5Ec62J3cJkhHbNfPJ0U49NrL?=
 =?us-ascii?Q?W6CqA50bFDRpQpr+kCLnG6VCNQ+rqJzezhimYVzCGtV+SqhnDyEkMKIJcRSM?=
 =?us-ascii?Q?uWOKyPuCjmckDDhDNn6cUaP7QWA7/1D1ho/P7V5Zf9cBcpudIhUN+hjBZ9yK?=
 =?us-ascii?Q?Dk3UUn/822B7mdmoiHqiySoBCOXJhyUR/jfTmpZ+fmtd7EVLo7WfJunjDGRm?=
 =?us-ascii?Q?Ix+qNriG8P+tY7Y16ELaIshenboJkNFuoB1Yb6zL4we1bi9YQRNE7+yNCE9R?=
 =?us-ascii?Q?xehHEqxvgfJf2V6DfMxP6/5B1JHMr8ahzQjQll6JRyoKcAx54YJDLC19Abx/?=
 =?us-ascii?Q?o+VQlsmtvhUfKtwwiv8WvV+QMiXXyrmowZAihRmGHh1S8hAtsOkYhnXJK6k9?=
 =?us-ascii?Q?k3PFV89oHgCgz+EvrVW3mxrVt13qNFU7K1zAp734edyAhsNVgbOtxpr5fcfa?=
 =?us-ascii?Q?jfN5ubel6SNSX+qP0M5qfM0QjTAU94YioWkag3dSHFgCRfVwm/YRxsizcKIk?=
 =?us-ascii?Q?XY5tCql9Ehkx4wpDDNENJZHABDmu6Sg0gfEc8eHv4a8ik/S6Ux1YLJrAZ7CX?=
 =?us-ascii?Q?LQX0F5km6Z7xgLvenRt4vmulyCs2Zp01vBD2WgInxbdX+oIN4l7vnZh6Zl9I?=
 =?us-ascii?Q?3anenjAruvfnk3JJjVfzRmgc9YQ/ro53gXjrbocwI4dJSldkFZj2ZoZN3+e8?=
 =?us-ascii?Q?neSLuC/Lmm8vx7ka+fmi13gPd+SBOYeNMNAiig34tj2q4p3CR1e4mKplZqDW?=
 =?us-ascii?Q?QdTQxSuFIJQ0WNko0IlRXGVUAMCsYlLz+dYuOGq04jyIVAOTr6ljg+vVQych?=
 =?us-ascii?Q?fA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 31e831da-eacb-43b1-b4c9-08da9d965d3b
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 19:03:56.2445
 (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: 8nj56Fw+c1Kl0FJqUjQzRT+wNiqH3ZLrCo2ImpOfWCCDNUAXD0By9q6ce6oGWDK32+f+Gr76dL5UUnW+3GHlrfk0Y/k0AbwsU8L0HGAp0pQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5199
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 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.
> 
> > > > 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.