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 09:29:51 -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: <632ddeffd86ff_33d629490@dwillia2-xfh.jf.intel.com.notmuch>
References: <YysZrdF/BSQhjWZs@nvidia.com>
 <632b2b4edd803_66d1a2941a@dwillia2-xfh.jf.intel.com.notmuch>
 <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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yy2ziac3GdHrpxuh@nvidia.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NLdrMbpZFa1vGifbgaQnNIS0dv6rH53N4c4tik8LOW3eDwLxWul9q2GfDnxD?=
 =?us-ascii?Q?d/+5q7vjynaHVdrVG/moz/TfE+0CZND+Lzz3qLpCiUkbQHK0vpXI/tE+yOWC?=
 =?us-ascii?Q?iv2mairKwgjMYeNb+ommhFOwMXinU7YY/8zBCnaL0LEDPRNCPrO3pMFNgVLI?=
 =?us-ascii?Q?cBW2y00OCiX7GkiiOYdZbDL7aLai3nVZgRCIC8YD06f2mJgI2UFwSFNr+IUv?=
 =?us-ascii?Q?qN16TlSja2srS7NoVZQArJLLyK7KJ7Uwm3PXHryoHEfm84cIC9WHS0YB2CBD?=
 =?us-ascii?Q?mXn3X50n/6lV8J3D2LoMvr0mG7S4jsTU/u2lQumCI85K6aNqWcTmrAcHyztW?=
 =?us-ascii?Q?LyLH/d2j+mRzrEE2se+tyQO5fv2yVM3E+TzSqZHDh3lZ/K6gw/mG5G1nuIkE?=
 =?us-ascii?Q?o3dnQL345GqwSee3ijRIaEH8nmcfCAvsq5smvioh31+SKDc9ygGzKwSLbDta?=
 =?us-ascii?Q?GirJH4TX39oGNsJPw1AHAG7AWFDUVebDyI3gYUNMSqDxtGM2xwgzIC5li+GK?=
 =?us-ascii?Q?7wJdCgZfMwRJPe5IWnPkiAxf0X5wnEm/dw4icQH+tyiCOKFvRMO+fM131wi6?=
 =?us-ascii?Q?ZXBiw8T389ekoquTFseJZrIes4Qza96I6e99457IdF8p8eFXu3zK4cIwfs9k?=
 =?us-ascii?Q?lyiKAm4PBQP/I2P68Nlic9UlVyQN82Uh55veCazfHMiL+btZTQAMWzMmyAIc?=
 =?us-ascii?Q?8XhKKds5SMaOmHLpZUSkK5F5aNi8iY/Aa3APo7vLl7VyURBABvQUU8JMeFDp?=
 =?us-ascii?Q?1smP970Lpu2XfKLC0kzRurp5ux+WqvtQSHL+H4e5Gy3tXQvH4WmeEHKbHTG0?=
 =?us-ascii?Q?4AyiYx1jJman5q4dEAxvb++N/XLSo5weuymlAYTCAz1p3J5bq3grfhZlqLpl?=
 =?us-ascii?Q?ZP1JqwEjRXjwv6ud+peZNl6Op4QoBEeAwvNkUtAAuOMYL1LlTp3TBxLaAk0J?=
 =?us-ascii?Q?EagfEhLd+KL08DmqjRsCfpyLs97k1rPs255PHPHgynIbyKYKP1Ru0GUHyHIi?=
 =?us-ascii?Q?4xuiM4bq8sKbR0Za4t9E7M10iqWyk8KiAdyFoSuk4uSZ0wYSLaTB20QGuBdc?=
 =?us-ascii?Q?oz/2wdQ5gvgtw8DDWwGkQhcHRSemVGFSA3FsvkEKh3PKR9/foWfRiQI4BPku?=
 =?us-ascii?Q?qs8dckYVZrYU6kpnxjkSw9yE4tMHI/vK/E9pv22wSoZRUNaYwneuIvNSB/4Y?=
 =?us-ascii?Q?SeMssP/Anno3u22JZrVQMGg4ezeaH2pxFuec3GuZFRGFElQkw+1uYMi+T7Go?=
 =?us-ascii?Q?OfjKIMHYhcoRWIGWq8jJ7p4r4V+t/dMd/rFz7fkUA3UtgLUns5POpP7DUxuo?=
 =?us-ascii?Q?7PP+kT6tbVnbaep7Q/wrIuA/LKDFal9hoo1Mi9ypfn5imkuNM9yEtIKOL89y?=
 =?us-ascii?Q?aCQCdmHhFlqMd4+IcPwpjbmk4gXx8QUAn1HKuoagL+Esf2ktJxymNJ4iTwKx?=
 =?us-ascii?Q?2P+m0bwaQFMC3FzvItOfLIsetCDEEVYx5JpM2O8k1IJ9yaHYJfpDIp686u2y?=
 =?us-ascii?Q?m90N58MGcl9ssLgTzClDGkBvgVSqPTu0VyB4liPUvvcnd5V3TqWOKNtQ7xpU?=
 =?us-ascii?Q?gdTp2P4MmZ7MzeK5zC36voAmRozBL8SIN9wheBprgYtZsbcAj++ClimJ66Em?=
 =?us-ascii?Q?4g=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: efb33aa0-0229-4bf0-bc89-08da9d80d89d
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 16:29:54.3812
 (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: yOSSoLg1OHcDLgLN+HCiV1ENgCFrn9GraHnKQYnl+P/iKXYJ/5FaFvZ3O16DC1bTw3Obkr2/U1T8lcWk5mlPdALc13JDsCD7l85RTq9uiLY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6918
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        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 Thu, Sep 22, 2022 at 02:54:42PM -0700, Dan Williams wrote:
> 
> > > I'm thinking broadly about how to make pgmap usable to all the other
> > > drivers in a safe and robust way that makes some kind of logical sense.
> > 
> > I think the API should be pgmap_folio_get() because, at least for DAX,
> > the memory is already allocated. 
> 
> I would pick a name that has some logical connection to
> ops->page_free()
> 
> This function is starting a pairing where once it completes page_free
> will eventually be called.

Following Dave's note that this is an 'arbitration' mechanism I think
request/release is more appropriate than alloc/free for what this is doing.

> 
> > /**
> >  * 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)
> > {
> >         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.

> 
> >         page = pfn_to_page(pfn);
> >         return page_folio(page);
> > }
> 
> Yeah, makes sense to me, but I would do a len as well to amortize the
> cost of all these checks..
> 
> > 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. So I
think that flow is request all pfns, lock, fixup refcounts, build up
compound folio, insert huge i_pages entry, unlock and install the pmd.