* [PATCH] include: mm: Adding new inline function vmf_error @ 2018-04-05 16:22 Souptick Joarder 2018-04-05 16:52 ` Matthew Wilcox 2018-04-05 19:53 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Souptick Joarder @ 2018-04-05 16:22 UTC (permalink / raw) To: willy, akpm; +Cc: linux-mm 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] include: mm: Adding new inline function vmf_error 2018-04-05 16:22 [PATCH] include: mm: Adding new inline function vmf_error Souptick Joarder @ 2018-04-05 16:52 ` Matthew Wilcox 2018-04-05 19:53 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2018-04-05 16:52 UTC (permalink / raw) To: Souptick Joarder; +Cc: akpm, linux-mm 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] include: mm: Adding new inline function vmf_error 2018-04-05 16:22 [PATCH] include: mm: Adding new inline function vmf_error Souptick Joarder 2018-04-05 16:52 ` Matthew Wilcox @ 2018-04-05 19:53 ` Andrew Morton 2018-04-05 20:26 ` Matthew Wilcox 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2018-04-05 19:53 UTC (permalink / raw) To: Souptick Joarder; +Cc: willy, linux-mm 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] include: mm: Adding new inline function vmf_error 2018-04-05 19:53 ` Andrew Morton @ 2018-04-05 20:26 ` Matthew Wilcox 2018-05-02 6:17 ` Souptick Joarder 0 siblings, 1 reply; 6+ messages in thread From: Matthew Wilcox @ 2018-04-05 20:26 UTC (permalink / raw) To: Andrew Morton; +Cc: Souptick Joarder, linux-mm 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] include: mm: Adding new inline function vmf_error 2018-04-05 20:26 ` Matthew Wilcox @ 2018-05-02 6:17 ` Souptick Joarder 2018-05-02 21:12 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Souptick Joarder @ 2018-05-02 6:17 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Andrew Morton, Linux-MM 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] include: mm: Adding new inline function vmf_error 2018-05-02 6:17 ` Souptick Joarder @ 2018-05-02 21:12 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2018-05-02 21:12 UTC (permalink / raw) To: Souptick Joarder; +Cc: Matthew Wilcox, Linux-MM 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-02 21:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-05 16:22 [PATCH] include: mm: Adding new inline function vmf_error Souptick Joarder 2018-04-05 16:52 ` Matthew Wilcox 2018-04-05 19:53 ` Andrew Morton 2018-04-05 20:26 ` Matthew Wilcox 2018-05-02 6:17 ` Souptick Joarder 2018-05-02 21:12 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).