Re: [PATCH] fs: dax: Adding new return type vm_fault_t

[Ross Zwisler <ross.zwisler@linux.intel.com>]

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.