On Mon, Jul 24, 2017 at 01:25:30PM +0200, Jan Kara wrote:
@@ -1658,14 +1658,28 @@ static int insert_pfn(struct vm_area_struct *vma, unsigned long addr, if (!pte) goto out; retval = -EBUSY;
- if (!pte_none(*pte))
goto out_unlock;
- if (!pte_none(*pte)) {
if (mkwrite) {if (WARN_ON_ONCE(pte_pfn(*pte) != pfn_t_to_pfn(pfn)))Is the WARN_ON_ONCE() really appropriate here? Your testcase with private mappings has triggered this situation if I'm right...
Yep, I think this WARN_ON_ONCE() is correct. The test with private mappings had collisions between read-only DAX mappings which were being faulted in via insert_pfn(), and read/write COW page cache mappings which were being faulted in by wp_page_copy().
I was hitting a false-positive warning when I had the WARN_ON_ONCE() in insert_pfn() outside of the mkwrite case, i.e.:
if (!pte_none(*pte)) { if (WARN_ON_ONCE(pte_pfn(*pte) != pfn_t_to_pfn(pfn))) goto out_unlock; if (mkwrite) { entry = *pte; goto out_mkwrite; } else goto out_unlock; }
This was triggering when one thread was faulting in a read-only DAX mapping when another thread had already faulted in a read-write COW page cache page.
The patches I sent out have the warning in the mkwrite case, which would mean that we were getting a fault for a read/write PTE in insert_pfn() and the PFN didn't match what was already in the PTE.
This can't ever happen in the private mapping case because we will never install a read/write PTE for normal storage, only for COW page cache pages. Essentially I don't think we should ever be able to hit this warning, and if we do I'd like to get the bug report so that I can track down how it was happening and make sure that it's safe. It is in the mkwrite path of insert_pfn() which is currently only used by the DAX code.
Does that make sense to you, or would you recommend leaving it out? (If so, why?)