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

[Souptick Joarder <jrdr.linux@gmail.com>]

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;
>