Re: [PATCH 2/2] dax: Fix race between colliding PMD & PTE entries

[Ross Zwisler]

On Mon, May 22, 2017 at 04:37:48PM +0200, Jan Kara wrote:
> On Thu 18-05-17 15:29:39, Ross Zwisler wrote:
> > On Thu, May 18, 2017 at 09:50:37AM +0200, Jan Kara wrote:
> > > On Wed 17-05-17 11:16:39, Ross Zwisler wrote:
<>
> > > The first scenario seems to be possible. dax_iomap_pmd_fault() will create
> > > PMD entry in the radix tree. Then dax_iomap_pte_fault() will come, do
> > > grab_mapping_entry(), there it sees entry is PMD but we are doing PTE fault
> > > so I'd think that pmd_downgrade = true... But actually the condition there
> > > doesn't trigger in this case. And that's a catch that although we asked
> > > grab_mapping_entry() for PTE, we've got PMD back and that screws us later.
> > 
> > Yep, it was a concious decision when implementing the PMD support to allow one
> > thread to use PMDs and another to use PTEs in the same range, as long as the
> > thread faulting in PMDs is the first to insert into the radix tree.  A PMD
> > radix tree entry will be inserted and used for locking and dirty tracking, and
> > each thread or process can fault in either PTEs or PMDs into its own address
> > space as needed.
> 
> Well, for *threads* it doesn't really make good sense to mix PMDs and PTEs
> as they share page tables. However for *processes* it makes some sense to
> allow one process to use PTEs and another process to use PMDs. And I
> remember we were discussing this in the past.

Ugh, I was super sloppy with my use of "thread" and "process" in my previous
email.  Sorry, and thanks for the clarifications.  I think we're on the same
page, even if I had trouble articulating it. :)

> So normal fault path uses alloc_set_pte() for installing new PTE. And that
> uses pte_alloc_one_map() which checks whether PMD is still suitable for
> inserting a PTE. If not, we return VM_FAULT_NOPAGE. Probably it would be
> cleanest to factor our common parts of PTE and PMD insertion so that we can
> use these functions both from DAX and generic fault paths.

Makes sense, thanks.