On Tue, Apr 17, 2018 at 09:05:07PM +0530, Souptick Joarder wrote:
> Use new return type vm_fault_t for fault handler. For
> now, this is just documenting that the function returns
> a VM_FAULT value rather than an errno. Once all instances
> are converted, vm_fault_t will become a distinct type.
>
> Reference id -> 1c8f422059ae ("mm: change return type to
> vm_fault_t")
>
> There was an existing bug inside dax_load_hole()
> if vm_insert_mixed had failed to allocate a page table,
> we'd return VM_FAULT_NOPAGE instead of VM_FAULT_OOM.
> With vmf_insert_mixed this issue is addressed.
>
> vmf_insert_mixed_mkwrite() is the new wrapper function
> which will convert err returned from vm_insert_mixed_
> mkwrite() to vm_fault_t type.
This doesn't apply cleanly to v4.17-rc1, as it collides with patches from Dan,
and it doesn't work when applied to v4.16 because we're missing the
vmf_insert_mixed() wrapper. Generally when I have an odd baseline I provide a
link to a publicly accessible tree so people can grab a working version of my
patches, but in this case you probably just need to merge forward to the
latest linux/master.
> @@ -1094,6 +1094,18 @@ static bool dax_fault_is_synchronous(unsigned long flags,
> && (iomap->flags & IOMAP_F_DIRTY);
> }
>
> +static vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
> + unsigned long addr, pfn_t pfn)
> +{
> + int error;
> + vm_fault_t vmf_ret;
> +
> + error = vm_insert_mixed_mkwrite(vma, addr, pfn);
> + vmf_ret = dax_fault_return(error);
No need for all the temp variables and extra lines. This is simpler as:
error = vm_insert_mixed_mkwrite(vma, addr, pfn);
return dax_fault_return(error);
Obviating the need for vmf_ret, or just:
return dax_fault_return(vm_insert_mixed_mkwrite(vma, addr, pfn));
Though this may change once you address the -EBUSY handling mentioned below.
> @@ -1225,14 +1237,12 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
> }
> trace_dax_insert_mapping(inode, vmf, entry);
> if (write)
> - error = vm_insert_mixed_mkwrite(vma, vaddr, pfn);
> + vmf_ret = vmf_insert_mixed_mkwrite(vma, vaddr, pfn);
> else
> - error = vm_insert_mixed(vma, vaddr, pfn);
> + vmf_ret = vmf_insert_mixed(vma, vaddr, pfn);
> +
> + goto finish_iomap;
>
> - /* -EBUSY is fine, somebody else faulted on the same PTE */
> - if (error == -EBUSY)
> - error = 0;
In this rework you've lost this special case handling for -EBUSY, which means
that with your code users hitting -EBUSY (which is a non-error) will get a
VM_FAULT_SIGBUS.
> @@ -1568,8 +1577,8 @@ static int dax_insert_pfn_mkwrite(struct vm_fault *vmf,
> * stored persistently on the media and handles inserting of appropriate page
> * table entry.
> */
> -int dax_finish_sync_fault(struct vm_fault *vmf, enum page_entry_size pe_size,
> - pfn_t pfn)
> +vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf,
> + enum page_entry_size pe_size, pfn_t pfn)
> {
> int err;
> loff_t start = ((loff_t)vmf->pgoff) << PAGE_SHIFT;
> @@ -1581,7 +1590,8 @@ int dax_finish_sync_fault(struct vm_fault *vmf, enum page_entry_size pe_size,
> len = PMD_SIZE;
> else
> WARN_ON_ONCE(1);
> - err = vfs_fsync_range(vmf->vma->vm_file, start, start + len - 1, 1);
> + err = vfs_fsync_range(vmf->vma->vm_file,
> + start, start + len - 1, 1);
Unrelated and unneeded whitespace change.
