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;
