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

[Jan Kara <jack@suse.cz>]

On Wed 16-11-16 14:28:20, Ross Zwisler wrote:
> On Fri, Nov 04, 2016 at 05:25:06AM +0100, 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 and make locking functions
> > private to fs/dax.c.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> > @@ -1006,13 +1007,14 @@ int dax_iomap_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> >  
> >  		if (error)
> >  			goto finish_iomap;
> > -		if (!radix_tree_exceptional_entry(entry)) {
> > +
> > +		__SetPageUptodate(vmf->cow_page);
> > +		if (!radix_tree_exceptional_entry(entry))
> >  			vmf->page = entry;
> 
> I don't think we need to set vmf->page anymore.  We would clear it to NULL in
> a few lines anyway, and the only call in between is finish_fault(), which
> only cares about vmf->cow_page().  This allows us to remove the vmf->page =
> NULL line a few lines below as well.

Well, I would not like to depend too much on which fields of vm_fault
finish_fault() actually uses - we should fill in as much as we have
available. But the truth is we sometime have page to fill in into vmf->page
and sometimes we don't so in this case I agree filling it in is pointless.
Changed.

> > @@ -1051,7 +1053,7 @@ int dax_iomap_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> >  		}
> >  	}
> >   unlock_entry:
> > -	if (!locked_status || error)
> > +	if (vmf_ret != VM_FAULT_LOCKED || error)
> >  		put_locked_mapping_entry(mapping, vmf->pgoff, entry);
> 
> I don't think this is quite right.  For example, for dax_load_hole(), if we
> can't get a page we put_locked_mapping_entry() and return VM_FAULT_OOM.
> Previously this logic would have skipped the second call to
> put_locked_mapping_entry(), but now with the strict check against
> VM_FAULT_LOCKED put the entry twice.
> 
> Maybe the right thing to do is just fix dax_load_hole() so it never calls
> put_locked_mapping_entry(), and leave this check as you have it?

Yeah, good catch. Actually I have follow up patches which somewhat clean up
dax_iomap_fault() so that page fault is fully completed within
dax_iomap_fault() even when instantiating a hole page which makes error
handling simpler. But I didn't want to complicate this series with it. So
for now I'll do what you suggest. Thanks.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>