On Mon 17-10-16 13:29:49, Ross Zwisler wrote: > On Tue, Sep 27, 2016 at 06:08:14PM +0200, Jan Kara wrote: > > Move final handling of COW faults from generic code into DAX fault > > handler. That way generic code doesn't have to be aware of peculiarities > > of DAX locking so remove that knowledge. > > > > Signed-off-by: Jan Kara <j...@suse.cz> > > --- > > fs/dax.c | 22 ++++++++++++++++------ > > include/linux/dax.h | 7 ------- > > include/linux/mm.h | 9 +-------- > > mm/memory.c | 14 ++++---------- > > 4 files changed, 21 insertions(+), 31 deletions(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 0dc251ca77b8..b1c503930d1d 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -876,10 +876,15 @@ int dax_fault(struct vm_area_struct *vma, struct > > vm_fault *vmf, > > goto unlock_entry; > > if (!radix_tree_exceptional_entry(entry)) { > > vmf->page = entry; > > - return VM_FAULT_LOCKED; > > + if (unlikely(PageHWPoison(entry))) { > > + put_locked_mapping_entry(mapping, vmf->pgoff, > > + entry); > > + return VM_FAULT_HWPOISON; > > + } > > } > > - vmf->entry = entry; > > - return VM_FAULT_DAX_LOCKED; > > + error = finish_fault(vmf); > > + put_locked_mapping_entry(mapping, vmf->pgoff, entry); > > + return error ? error : VM_FAULT_DONE_COW; > > } > > > > if (!buffer_mapped(&bh)) { > > @@ -1430,10 +1435,15 @@ int iomap_dax_fault(struct vm_area_struct *vma, > > struct vm_fault *vmf, > > goto unlock_entry; > > if (!radix_tree_exceptional_entry(entry)) { > > vmf->page = entry; > > In __do_fault() we explicitly clear vmf->page in the case where PageHWPoison() > is set. I think we can get the same behavior here by moving the call that > sets vmf->page after the PageHWPoison() check.
Actually, the whole HWPoison checking was non-sensical for DAX. We want to check for HWPoison to avoid reading from poisoned pages. However for DAX we either use copy_user_dax() which takes care of IO errors / poisoning itself or we use clear_user_highpage() which doesn't touch the source page. So we don't have to check for HWPoison at all. Fixed. > > - return VM_FAULT_LOCKED; > > + if (unlikely(PageHWPoison(entry))) { > > + put_locked_mapping_entry(mapping, vmf->pgoff, > > + entry); > > + return VM_FAULT_HWPOISON; > > + } > > } > > - vmf->entry = entry; > > - return VM_FAULT_DAX_LOCKED; > > I think we're missing a call to > > __SetPageUptodate(new_page); > before finish_fault()? This call currently lives in do_cow_fault(), and > is part of the path that we don't skip as part of the VM_FAULT_DAX_LOCKED > logic. Ah, great catch. I wonder how the DAX COW test could have passed with this? Maybe PageUptodate is not used much for anon pages... Anyway thanks for spotting this. Honza -- Jan Kara <j...@suse.com> SUSE Labs, CR _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm