Re: [PATCH 00/13] Fix the DAX-gup mistake

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>, Jan Kara <jack@suse.cz>,
	Christoph Hellwig <hch@lst.de>,
	"Darrick J. Wong" <djwong@kernel.org>,
	John Hubbard <jhubbard@nvidia.com>,
	Matthew Wilcox <willy@infradead.org>, <linux-mm@kvack.org>,
	<nvdimm@lists.linux.dev>, <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 00/13] Fix the DAX-gup mistake
Date: Tue, 6 Sep 2022 12:41:33 -0700	[thread overview]
Message-ID: <6317a26d3e1ed_166f2946e@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <YxeWQIxPZF0QJ/FL@nvidia.com>

Jason Gunthorpe wrote:
> On Tue, Sep 06, 2022 at 11:37:36AM -0700, Dan Williams wrote:
> > Jason Gunthorpe wrote:
> > > On Tue, Sep 06, 2022 at 10:23:41AM -0700, Dan Williams wrote:
> > > 
> > > > > Can we continue to have the weird page->refcount behavior and still
> > > > > change the other things?
> > > > 
> > > > No at a minimum the pgmap vs page->refcount problem needs to be solved
> > > > first.
> > > 
> > > So who will do the put page after the PTE/PMD's are cleared out? In
> > > the normal case the tlb flusher does it integrated into zap..
> > 
> > AFAICS the zap manages the _mapcount not _refcount. Are you talking
> > about page_remove_rmap() or some other reference count drop?
> 
> No, page refcount.
> 
> __tlb_remove_page() eventually causes a put_page() via
> tlb_batch_pages_flush() calling free_pages_and_swap_cache()
> 
> Eg:
> 
>  *  MMU_GATHER_NO_GATHER
>  *
>  *  If the option is set the mmu_gather will not track individual pages for
>  *  delayed page free anymore. A platform that enables the option needs to
>  *  provide its own implementation of the __tlb_remove_page_size() function to
>  *  free pages.

Ok, yes, that is a vm_normal_page() mechanism which I was going to defer
since it is incremental to the _refcount handling fix and maintain that
DAX pages are still !vm_normal_page() in this set.

> > > Can we safely have the put page in the fsdax side after the zap?
> > 
> > The _refcount is managed from the lifetime insert_page() to
> > truncate_inode_pages(), where for DAX those are managed from
> > dax_insert_dentry() to dax_delete_mapping_entry().
> 
> As long as we all understand the page doesn't become re-allocatable
> until the refcount reaches 0 and the free op is called it may be OK!

Yes, but this does mean that page_maybe_dma_pinned() is not sufficient for
when the filesystem can safely reuse the page, it really needs to wait
for the reference count to drop to 0 similar to how it waits for the
page-idle condition today.