From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com ([134.134.136.126]:18409 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752254AbeDQU0K (ORCPT ); Tue, 17 Apr 2018 16:26:10 -0400 Date: Tue, 17 Apr 2018 14:26:09 -0600 From: Ross Zwisler To: Souptick Joarder Cc: mawilcox@microsoft.com, ross.zwisler@linux.intel.com, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] fs: dax: Adding new return type vm_fault_t Message-ID: <20180417202609.GA19878@linux.intel.com> References: <20180417153507.GA31905@jordon-HP-15-Notebook-PC> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180417153507.GA31905@jordon-HP-15-Notebook-PC> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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.