Re: [PATCH 10/20] mm: Move handling of COW faults into DAX code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <jack@xxxxxxx>
> > ---
> >  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 <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux