On Thu 27-07-17 16:22:30, Ross Zwisler wrote:
> On Thu, Jul 27, 2017 at 03:12:42PM +0200, Jan Kara wrote:
> > Currently dax_insert_mapping() returns normal error code which is later
> > converted to VM_FAULT_ state. Since we will need to do more state
> > modifications specific to dax_insert_mapping() it does not make sense to
> > push them up to the caller of dax_insert_mapping(). Instead make
> > dax_insert_mapping() return VM_FAULT_ state the same way as
> > dax_pmd_insert_mapping() does that.
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > fs/dax.c | 45 +++++++++++++++++++++++++--------------------
> > 1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 0673efd72f53..9658975b926a 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -814,6 +814,15 @@ int dax_writeback_mapping_range(struct address_space *mapping,
> > }
> > EXPORT_SYMBOL_GPL(dax_writeback_mapping_range);
> >
> > +static int dax_fault_return(int error)
> > +{
> > + if (error == 0)
> > + return VM_FAULT_NOPAGE;
> > + if (error == -ENOMEM)
> > + return VM_FAULT_OOM;
> > + return VM_FAULT_SIGBUS;
> > +}
> > +
> > static sector_t dax_iomap_sector(struct iomap *iomap, loff_t pos)
> > {
> > return iomap->blkno + (((pos & PAGE_MASK) - iomap->offset) >> 9);
> > @@ -828,7 +837,7 @@ static int dax_insert_mapping(struct vm_fault *vmf, struct iomap *iomap,
> > unsigned long vaddr = vmf->address;
> > void *ret, *kaddr;
> > pgoff_t pgoff;
> > - int id, rc;
> > + int id, rc, vmf_ret;
> > pfn_t pfn;
> >
> > rc = bdev_dax_pgoff(iomap->bdev, sector, PAGE_SIZE, &pgoff);
> > @@ -850,9 +859,18 @@ static int dax_insert_mapping(struct vm_fault *vmf, struct iomap *iomap,
> >
> > trace_dax_insert_mapping(mapping->host, vmf, ret);
> > if (vmf->flags & FAULT_FLAG_WRITE)
> > - return vm_insert_mixed_mkwrite(vma, vaddr, pfn);
> > + rc = vm_insert_mixed_mkwrite(vma, vaddr, pfn);
> > else
> > - return vm_insert_mixed(vma, vaddr, pfn);
> > + rc = vm_insert_mixed(vma, vaddr, pfn);
> > +
> > + /* -EBUSY is fine, somebody else faulted on the same PTE */
> > + if (rc == -EBUSY)
> > + rc = 0;
> > +
> > + vmf_ret = dax_fault_return(rc);
>
> Prior to this point where we convert our 'rc' (which is a normal error code
> like 0, -ENOMEM, etc) to a 'vmf_ret' (which is a VM return code like
> VM_FAULT_NOPAGE, VM_FAULT_SIGBUS, etc), there are several paths in
> dax_insert_mapping() where we could still return a normal error code. I think
> this will confuse the fault handler because this code will get returned all
> the way up to do_fault() which expects to get a VM return code.
>
> So, I think we either need to:
>
> a) Make sure all return paths through dax_insert_mapping() end up going
> through dax_fault_return(), as we do in the PMD case, or
Yeah, this was the intention (as my eventually I'd like to make
dax_insert_mapping() and dax_pmd_insert_mapping() one function - they are
already currently very similar). I'll fix the missed cases.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
