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:   Thu, 22 Sep 2022 19:01:56 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Dave Chinner <david@fromorbit.com>,
        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
Message-ID: <632d13949e113_4a6742947c@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>
 <20220923013634.GY3600936@dread.disaster.area>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220923013634.GY3600936@dread.disaster.area>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?roG1sgajdMGWo26HCP2/eC1dxpk2lGJGzxsl2ZT4nbMrnfH1JrrnOaAUTHhH?=
 =?us-ascii?Q?bDoYXwziyAGpIsmHAZkREaLVFr/iJXs7szS3O8wEUqCVkfsbmTtFVn+xVHJr?=
 =?us-ascii?Q?9IMwM5oHwe51+zLuhn80nypulC9qAlarwdc0mUVwoBg6x2h0n1uh+BSFp4N2?=
 =?us-ascii?Q?Ag8zuGM61X5y/pSFCyORYRwiiF6SPfbuheAI5y1mEsgDgag4WrWKkTb+UJUF?=
 =?us-ascii?Q?60OdPbvkEFgEX76MBCG5N6mkKwRUzUGIVtu66WaEvHFgcWH531Hj6jn4PWp3?=
 =?us-ascii?Q?9W8bHIXxRA1ut/PMrNajPTw5QPdlEFP3Jmx2AuZ3ab7U1gDF//5w6mVijB5P?=
 =?us-ascii?Q?HDq++5K3M5FU0RwnZTB6dLpQ8NlqoOCFVWJQt/keXkJIWr6PLgBFwKR+jZAS?=
 =?us-ascii?Q?wDS3p98EHvTThd3JAQR6l3QevRbd9zxq0t2QRfaZcShVjhKcLl3ux1MngOdv?=
 =?us-ascii?Q?042RtWBSorhxg7RnfG1A1qQAGgxZZYLi//CSmGS3Rh0brYJsrFrB/IbR+0an?=
 =?us-ascii?Q?V8DsP9p+5YR0fRBkOjv6wdzDg+DWV/szhnmfLtQO1D+vrctfAN81lJYQ5AKH?=
 =?us-ascii?Q?YQ5WzLHhB6fBA7CRq03eKFTmFASzVr/eV9tLwQCiV6roVEUzBDuunaLzynn4?=
 =?us-ascii?Q?ocT3ktE5hLYQYY5jO6mN04fUuOH9RYnX4Iynlr7026yBxeYIFgzCsVa/h5B8?=
 =?us-ascii?Q?0E6GY+IlCGW3PdZ6jqyjg+9HrLtIUmW9/lDBL47KxpZ/P4J3xyq7h0UFUOa2?=
 =?us-ascii?Q?DndLF20o6N6dO5LN54nzSsV6GhWjcXdFjyAwXnFo+uHl1A4AznaKASnG6I6X?=
 =?us-ascii?Q?HnIiHVpeoNLZUGseoMCuzdR5Z8JJqlrvKjcz6hoib8gyrH0j0SMSMoAwXW91?=
 =?us-ascii?Q?FVlrEW8onqd91E3cp0ziKApevkIPiSqgOqprN3CiA3rKekE23CA0nHilqtZ7?=
 =?us-ascii?Q?4jzRr7ZGQyTRE80Prw6LlPUHMF+FNDfQ0gg9PKJkqJyaYeJLNeyGBvSbD9N+?=
 =?us-ascii?Q?WWxtBQEyNVbptJaJAIGQKyj5E81kCfu6KelhxeTgCwi4ufmG+TRbX29LFR3M?=
 =?us-ascii?Q?RDtfUATww4XLD6e5k47858vxjSjFTdHS4uwfvZi/nTIdxZ8mFu8jD6xWEbwG?=
 =?us-ascii?Q?hZ9iuizHbSTTigIS4PBdvVRBrMooqZf7lp+JNJ5a5KIAFsfGRghXkF1/g0jz?=
 =?us-ascii?Q?AtnUxuAi4H+5e03PlJIyjBGI28Gvojchybzfr3S0B45PnaBYyw8KfZW+Etux?=
 =?us-ascii?Q?oxgznuMU11GP7YsUefKyAiQlV3wML4nKR7SJhraBm5CcsF4M267mt+SqYd48?=
 =?us-ascii?Q?ifNDyjDT2395b561I7DwJpMSejFEGqQKYaEXx/xOifXyD24EiA7XWtesvaIv?=
 =?us-ascii?Q?GBAZHBl3iKiIqxgzG+VD+4ipP+/8tph5kxk8X1WJk16yJK55zDpDJxhS1O7k?=
 =?us-ascii?Q?PFHyBamJh9ZjYolcTD6hTHmHqfzwVHzhbwGpP9BeONd8W5s00LZ8axdDjGeT?=
 =?us-ascii?Q?rHLSomp/u4xBcP7l/fcXoswK1z07qr+SSBb7LPpbj7i0ERj2+C2aL0qFcoH4?=
 =?us-ascii?Q?Efo2OXyp0YMCU4VHpMBkpJYhl7MjXjp7ynKDNcqsIFjkwm89ZQ7M0E8hCa6k?=
 =?us-ascii?Q?Cw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 75ebc668-bb77-4956-24df-08da9d079982
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 02:01:59.3683
 (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: JvgfUVQZY8JNQzKBVESJqovBLYLehW0lowB3wewpRJDgp6dyWWGaAzSAnyExyllvOqjCmYGJjBawphN/weNAVERhQlo88f0yZHXV4EPh9RY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5037
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

Dave Chinner wrote:
> On Thu, Sep 22, 2022 at 02:54:42PM -0700, Dan Williams wrote:
> > Jason Gunthorpe wrote:
> > > On Wed, Sep 21, 2022 at 07:17:40PM -0700, Dan Williams wrote:
> > > > Jason Gunthorpe wrote:
> > > > > On Wed, Sep 21, 2022 at 05:14:34PM -0700, Dan Williams wrote:
> > > > > 
> > > > > > > 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.
> > > > > 
> > > > > That makes some sense too, but the logical way to do that is to put some
> > > > > counter along the page_free() path, and establish a 'make a page not
> > > > > free' path that does the other side.
> > > > > 
> > > > > ie it should not be in DAX code, it should be all in common pgmap
> > > > > code. The pgmap should never be freed while any page->refcount != 0
> > > > > and that should be an intrinsic property of pgmap, not relying on
> > > > > external parties.
> > > > 
> > > > I just do not know where to put such intrinsics since there is nothing
> > > > today that requires going through the pgmap object to discover the pfn
> > > > and 'allocate' the page.
> > > 
> > > I think that is just a new API that wrappers the set refcount = 1,
> > > percpu refcount and maybe building appropriate compound pages too.
> > > 
> > > Eg maybe something like:
> > > 
> > >   struct folio *pgmap_alloc_folios(pgmap, start, length)
> > > 
> > > And you get back maximally sized allocated folios with refcount = 1
> > > that span the requested range.
> > > 
> > > > In other words make dax_direct_access() the 'allocation' event that pins
> > > > the pgmap? I might be speaking a foreign language if you're not familiar
> > > > with the relationship of 'struct dax_device' to 'struct dev_pagemap'
> > > > instances. This is not the first time I have considered making them one
> > > > in the same.
> > > 
> > > I don't know enough about dax, so yes very foreign :)
> > > 
> > > 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. The 'allocator' for fsdax is the
> > filesystem block allocator, and pgmap_folio_get() grants access to a
> 
> No, the "allocator" for fsdax is the inode iomap interface, not the
> filesystem block allocator. The filesystem block allocator is only
> involved in iomapping if we have to allocate a new mapping for a
> given file offset.
> 
> A better name for this is "arbiter", not allocator.  To get an
> active mapping of the DAX pages backing a file, we need to ask the
> inode iomap subsystem to *map a file offset* and it will return
> kaddr and/or pfns for the backing store the file offset maps to.
> 
> IOWs, for FSDAX, access to the backing store (i.e. the physical pages) is
> arbitrated by the *inode*, not the filesystem allocator or the dax
> device. Hence if a subsystem needs to pin the backing store for some
> use, it must first ensure that it holds an inode reference (direct
> or indirect) for that range of the backing store that will spans the
> life of the pin. When the pin is done, it can tear down the mappings
> it was using and then the inode reference can be released.
> 
> This ensures that any racing unlink of the inode will not result in
> the backing store being freed from under the application that has a
> pin. It will prevent the inode from being reclaimed and so
> potentially accessing stale or freed in-memory structures. And it
> will prevent the filesytem from being unmounted while the
> application using FSDAX access is still actively using that
> functionality even if it's already closed all it's fds....

Sounds so simple when you put it that way. I'll give it a shot and stop
the gymnastics of trying to get in front of truncate_inode_pages_final()
with a 'dax break layouts', just hold it off until final unpin.