[PATCH] include: mm: Adding new inline function vmf_error

[PATCH] include: mm: Adding new inline function vmf_error

[Souptick Joarder]

Many places in drivers/ file systems error was handled
like below -
ret = (ret == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;

This new inline function vmf_error() will replace this
and return vm_fault_t type err.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 include/linux/mm.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a4d8853..e283dd8 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2453,6 +2453,18 @@ static inline vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma,
 	return VM_FAULT_NOPAGE;
 }
 
+static inline vm_fault_t vmf_error(int err)
+{
+	vm_fault_t ret;
+
+	if (err == -ENOMEM)
+		ret = VM_FAULT_OOM;
+	else
+		ret = VM_FAULT_SIGBUS;
+
+	return ret;
+}
+
 struct page *follow_page_mask(struct vm_area_struct *vma,
 			      unsigned long address, unsigned int foll_flags,
 			      unsigned int *page_mask);
-- 
1.9.1

Re: [PATCH] include: mm: Adding new inline function vmf_error

[Matthew Wilcox]

On Thu, Apr 05, 2018 at 09:52:25PM +0530, Souptick Joarder wrote:
> Many places in drivers/ file systems error was handled
> like below -
> ret = (ret == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
> 
> This new inline function vmf_error() will replace this
> and return vm_fault_t type err.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>

Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>

To elaborate a little more on the changelog above, a lot of drivers and
filesystems currently have a rather complex mapping of errno-to-VM_FAULT
code.  We've been able to eliminate a lot of it by just returning VM_FAULT
codes directly from functions which are called exclusively from the
fault handling path.

Some functions can be called both from the fault handler and other context
which are expecting an errno, so they have to continue to return an errno.
Some users still need to choose different behaviour for different errnos,
but vmf_error() captures the essential error translation that's common
to all users, and those that need to handle additional errors can handle
them first.

We'd like to get this into -rc1 so we can start trickling driver chanages
that depend on it into the maintainer trees.

Re: [PATCH] include: mm: Adding new inline function vmf_error

[Andrew Morton]

On Thu, 5 Apr 2018 21:52:25 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> Many places in drivers/ file systems error was handled
> like below -
> ret = (ret == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
> 
> This new inline function vmf_error() will replace this
> and return vm_fault_t type err.
> 
> ...
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2453,6 +2453,18 @@ static inline vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma,
>  	return VM_FAULT_NOPAGE;
>  }
>  
> +static inline vm_fault_t vmf_error(int err)
> +{
> +	vm_fault_t ret;
> +
> +	if (err == -ENOMEM)
> +		ret = VM_FAULT_OOM;
> +	else
> +		ret = VM_FAULT_SIGBUS;
> +
> +	return ret;
> +}
> +

That's a bit verbose.  Why not simply

	return (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;

Also, if would be nice to see some sites converted so we can see the
benefit of the patch and to actually test it.

Re: [PATCH] include: mm: Adding new inline function vmf_error

[Matthew Wilcox]

On Thu, Apr 05, 2018 at 12:53:22PM -0700, Andrew Morton wrote:
> > +static inline vm_fault_t vmf_error(int err)
> > +{
> > +	vm_fault_t ret;
> > +
> > +	if (err == -ENOMEM)
> > +		ret = VM_FAULT_OOM;
> > +	else
> > +		ret = VM_FAULT_SIGBUS;
> > +
> > +	return ret;
> > +}
> > +
> 
> That's a bit verbose.  Why not simply
> 
> 	return (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;

That's a little skimpy for my taste (although Souptick's is more verbose
than I like too) ... I suggested this:

> > @@ -8983,9 +8984,9 @@ int btrfs_page_mkwrite(struct vm_fault *vmf)
> >  	}
> >  	if (ret) {
> >  		if (ret == -ENOMEM)
> > -			ret = VM_FAULT_OOM;
> > +			retval = VM_FAULT_OOM;
> >  		else /* -ENOSPC, -EIO, etc */
> > -			ret = VM_FAULT_SIGBUS;
> > +			retval = VM_FAULT_SIGBUS;
> >  		if (reserved)
> >  			goto out;
> >  		goto out_noreserve;
> 
> I'm seeing this pattern _a lot_ in filesystems.  It gets written in a
> few different ways, but
> 
> 	ret = (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
> 
> is really common.  I think we should do a helper function as part of
> these cleanups ... maybe:
> 
> static inline vm_fault_t vmf_error(int errno)
> {
> 	if (err == -ENOMEM)
> 		return VM_FAULT_OOM;
> 	return VM_FAULT_SIGBUS;
> }
> 
> -		if (ret == -ENOMEM)
> -			ret = VM_FAULT_OOM;
> -		else /* -ENOSPC, -EIO, etc */
> -			ret = VM_FAULT_SIGBUS;
> +		ret = vmf_error(err);
> 
> I know we've mostly been deleting these errno-to-vm_fault converters,
> but those try to do too much -- they handle an errno of 0 (when there
> are at least three ways to return success -- 0, NOPAGE and LOCKED),
> and often they've encoded some other VM_FAULT code in a different
> errno, eg the way block_page_mkwrite() uses -EFAULT.
> 
> There are a few other error codes to handle under special conditions,
> but the caller can handle them first.  eg I see block_page_mkwrite()
> eventually looking like this:
> 
> 	err = __block_write_begin(page, 0, end, get_block);
> 	if (!err)
> 		err = block_commit_write(page, 0, end);
> 
> 	if (unlikely(err < 0))
> 		goto error;
> 	set_page_dirty(page);
> 	wait_for_stable_page(page);
> 	return 0;
> error:
> 	if (err == -EAGAIN)
> 		ret = VM_FAULT_NOPAGE;
> 	else
> 		ret = vmf_error(err);
> out_unlock:
> 	unlock_page(page);
> 	return ret;

Re: [PATCH] include: mm: Adding new inline function vmf_error

[Souptick Joarder]

Hi Andrew,

Any further comment on this patch ?
Around 10 drivers/file systems changes (vm_fault_t type changes)
depend on this patch.

On Fri, Apr 6, 2018 at 1:56 AM, Matthew Wilcox <willy@infradead.org> wrote:
> On Thu, Apr 05, 2018 at 12:53:22PM -0700, Andrew Morton wrote:
>> > +static inline vm_fault_t vmf_error(int err)
>> > +{
>> > +   vm_fault_t ret;
>> > +
>> > +   if (err == -ENOMEM)
>> > +           ret = VM_FAULT_OOM;
>> > +   else
>> > +           ret = VM_FAULT_SIGBUS;
>> > +
>> > +   return ret;
>> > +}
>> > +
>>
>> That's a bit verbose.  Why not simply
>>
>>       return (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
>
> That's a little skimpy for my taste (although Souptick's is more verbose
> than I like too) ... I suggested this:
>
>> > @@ -8983,9 +8984,9 @@ int btrfs_page_mkwrite(struct vm_fault *vmf)
>> >     }
>> >     if (ret) {
>> >             if (ret == -ENOMEM)
>> > -                   ret = VM_FAULT_OOM;
>> > +                   retval = VM_FAULT_OOM;
>> >             else /* -ENOSPC, -EIO, etc */
>> > -                   ret = VM_FAULT_SIGBUS;
>> > +                   retval = VM_FAULT_SIGBUS;
>> >             if (reserved)
>> >                     goto out;
>> >             goto out_noreserve;
>>
>> I'm seeing this pattern _a lot_ in filesystems.  It gets written in a
>> few different ways, but
>>
>>       ret = (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
>>
>> is really common.  I think we should do a helper function as part of
>> these cleanups ... maybe:
>>
>> static inline vm_fault_t vmf_error(int errno)
>> {
>>       if (err == -ENOMEM)
>>               return VM_FAULT_OOM;
>>       return VM_FAULT_SIGBUS;
>> }
>>
>> -             if (ret == -ENOMEM)
>> -                     ret = VM_FAULT_OOM;
>> -             else /* -ENOSPC, -EIO, etc */
>> -                     ret = VM_FAULT_SIGBUS;
>> +             ret = vmf_error(err);
>>
>> I know we've mostly been deleting these errno-to-vm_fault converters,
>> but those try to do too much -- they handle an errno of 0 (when there
>> are at least three ways to return success -- 0, NOPAGE and LOCKED),
>> and often they've encoded some other VM_FAULT code in a different
>> errno, eg the way block_page_mkwrite() uses -EFAULT.
>>
>> There are a few other error codes to handle under special conditions,
>> but the caller can handle them first.  eg I see block_page_mkwrite()
>> eventually looking like this:
>>
>>       err = __block_write_begin(page, 0, end, get_block);
>>       if (!err)
>>               err = block_commit_write(page, 0, end);
>>
>>       if (unlikely(err < 0))
>>               goto error;
>>       set_page_dirty(page);
>>       wait_for_stable_page(page);
>>       return 0;
>> error:
>>       if (err == -EAGAIN)
>>               ret = VM_FAULT_NOPAGE;
>>       else
>>               ret = vmf_error(err);
>> out_unlock:
>>       unlock_page(page);
>>       return ret;
>

Re: [PATCH] include: mm: Adding new inline function vmf_error

[Andrew Morton]

On Wed, 2 May 2018 11:47:37 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> >> few different ways, but
> >>
> >>       ret = (err == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS;
> >>
> >> is really common.  I think we should do a helper function as part of
> >> these cleanups ... maybe:
> >>

(top-posting repaired.  Please don't top-post)

> Hi Andrew,
> 
> Any further comment on this patch ?
> Around 10 drivers/file systems changes (vm_fault_t type changes)
> depend on this patch.

Well I think we're expecting a new version which is coded less
verbosely?  Also, Matthew's comments up-thread would be a welcome
addition to the changelog.