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

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

On Tue, Apr 24, 2018 at 10:17:51PM +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.
> 
> commit 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 new vmf_insert_mixed() this issue is addressed.
> 
> vm_insert_mixed_mkwrite has inefficiency when it returns
> an error value, driver has to convert it to vm_fault_t
> type. With new vmf_insert_mixed_mkwrite() this limitation
> will be addressed.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>

Sure, this looks correct.  You can add:

Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

I noticed that we have the following status translation now in 4 places in 2
files:

        if (err == -ENOMEM)
                return VM_FAULT_OOM;
        if (err < 0 && err != -EBUSY)
                return VM_FAULT_SIGBUS;
        return VM_FAULT_NOPAGE;


This happens in vmf_insert_mixed_mkwrite(), vmf_insert_page(),
vmf_insert_mixed() and vmf_insert_pfn().

I think it'd be a good idea to consolidate this translation into an inline
helper, in the spirit of dax_fault_return().  This will ensure that if/when we
start changing this status translation, we won't accidentally miss some of the
places which would make them get out of sync.  No need to fold this into this
patch - it should be a separate change.