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

From: Dan Williams <dan.j.williams@intel.com>
To: Dan Williams <dan.j.williams@intel.com>,
	Jason Gunthorpe <jgg@nvidia.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 17:54:54 -0700	[thread overview]
Message-ID: <6317ebde620ec_166f29466@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <6317a26d3e1ed_166f2946e@dwillia2-xfh.jf.intel.com.notmuch>

Dan Williams wrote:
> 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.

This gets tricky with break_layouts(). For example xfs_break_layouts()
wants to ensure that the page is gup idle while holding the mmap lock.
If the page is not gup idle it needs to drop locks and retry. It is
possible the path to drop a page reference also needs to acquire
filesystem locs. Consider odd cases like DMA from one offset to another
in the same file. So waiting with filesystem locks held is off the
table, which also means that deferring the wait until
dax_delete_mapping_entry() time is also off the table.

That means that even after the conversion to make DAX page references
0-based it will still be the case that filesystem code will be waiting
for the 2 -> 1 transition to indicate "mapped DAX page has no more
external references".

Then dax_delete_mapping_entry() can validate that it is performing the 1
-> 0 transition since no more refernences should have been taken while
holding filesystem locks.