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@xxxxxxx>
> > ---
>
> > @@ -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@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
