[PATCH] fs: xfs: Change return type to vm_fault_t

[PATCH] fs: xfs: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handlers.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/xfs/xfs_file.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9ea0832..988ec56 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1027,7 +1027,7 @@
  *       page_lock (MM)
  *         i_lock (XFS - extent map serialisation)
  */
-static int
+static vm_fault_t
 __xfs_filemap_fault(
 	struct vm_fault		*vmf,
 	enum page_entry_size	pe_size,
@@ -1035,7 +1035,7 @@
 {
 	struct inode		*inode = file_inode(vmf->vma->vm_file);
 	struct xfs_inode	*ip = XFS_I(inode);
-	int			ret;
+	vm_fault_t		ret;
 
 	trace_xfs_filemap_fault(ip, pe_size, write_fault);
 
@@ -1064,7 +1064,7 @@
 	return ret;
 }
 
-static int
+static vm_fault_t
 xfs_filemap_fault(
 	struct vm_fault		*vmf)
 {
@@ -1074,7 +1074,7 @@
 			(vmf->flags & FAULT_FLAG_WRITE));
 }
 
-static int
+static vm_fault_t
 xfs_filemap_huge_fault(
 	struct vm_fault		*vmf,
 	enum page_entry_size	pe_size)
@@ -1087,7 +1087,7 @@
 			(vmf->flags & FAULT_FLAG_WRITE));
 }
 
-static int
+static vm_fault_t
 xfs_filemap_page_mkwrite(
 	struct vm_fault		*vmf)
 {
@@ -1099,7 +1099,7 @@
  * on write faults. In reality, it needs to serialise against truncate and
  * prepare memory for writing so handle is as standard write fault.
  */
-static int
+static vm_fault_t
 xfs_filemap_pfn_mkwrite(
 	struct vm_fault		*vmf)
 {
-- 
1.9.1

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Dave Chinner]

On Sun, Apr 15, 2018 at 01:35:30AM +0530, Souptick Joarder wrote:
> Use new return type vm_fault_t for fault handlers.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>

Ummm. Why are we adding typedefs to hide basic types?

typedef int vm_fault_t;

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Matthew Wilcox]

On Sun, Apr 15, 2018 at 07:59:37AM +1000, Dave Chinner wrote:
> On Sun, Apr 15, 2018 at 01:35:30AM +0530, Souptick Joarder wrote:
> > Use new return type vm_fault_t for fault handlers.
> > 
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Ummm. Why are we adding typedefs to hide basic types?
> 
> typedef int vm_fault_t;

That's the intermediate step.  It's going to become a sparse typedef
like gfp_t once everything's converted.  And the "why" is that people
are still returning -EFOO when they should be returning VM_FAULT_FOO.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Christoph Hellwig]

On Sat, Apr 14, 2018 at 03:24:07PM -0700, Matthew Wilcox wrote:
> On Sun, Apr 15, 2018 at 07:59:37AM +1000, Dave Chinner wrote:
> > On Sun, Apr 15, 2018 at 01:35:30AM +0530, Souptick Joarder wrote:
> > > Use new return type vm_fault_t for fault handlers.
> > > 
> > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > Ummm. Why are we adding typedefs to hide basic types?
> > 
> > typedef int vm_fault_t;
> 
> That's the intermediate step.  It's going to become a sparse typedef
> like gfp_t once everything's converted.  And the "why" is that people
> are still returning -EFOO when they should be returning VM_FAULT_FOO.

Please just send one damn patch adding the typedef and the sparse
annotation and converting everyone over.  Splitting this into five
gazillion patches that do nothing doesn't help anyone.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Matthew Wilcox]

On Sun, Apr 15, 2018 at 12:21:21AM -0700, Christoph Hellwig wrote:
> On Sat, Apr 14, 2018 at 03:24:07PM -0700, Matthew Wilcox wrote:
> > On Sun, Apr 15, 2018 at 07:59:37AM +1000, Dave Chinner wrote:
> > > On Sun, Apr 15, 2018 at 01:35:30AM +0530, Souptick Joarder wrote:
> > > > Use new return type vm_fault_t for fault handlers.
> > > > 
> > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > > Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > Ummm. Why are we adding typedefs to hide basic types?
> > > 
> > > typedef int vm_fault_t;
> > 
> > That's the intermediate step.  It's going to become a sparse typedef
> > like gfp_t once everything's converted.  And the "why" is that people
> > are still returning -EFOO when they should be returning VM_FAULT_FOO.
> 
> Please just send one damn patch adding the typedef and the sparse
> annotation and converting everyone over.  Splitting this into five
> gazillion patches that do nothing doesn't help anyone.

The XFS one is trivial.  Many of the others are not.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Christoph Hellwig]

On Sun, Apr 15, 2018 at 04:11:04AM -0700, Matthew Wilcox wrote:
> > Please just send one damn patch adding the typedef and the sparse
> > annotation and converting everyone over.  Splitting this into five
> > gazillion patches that do nothing doesn't help anyone.
> 
> The XFS one is trivial.  Many of the others are not.

if they arne't trivial you are doing something wrong.  That is you
fix bugs that exist independent of the annotation together with the
annotation.

Fix all the bugs first, and send them in (including for -stable) and
then do the annotations in one big patch.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Matthew Wilcox]

On Sun, Apr 15, 2018 at 05:04:26AM -0700, Christoph Hellwig wrote:
> On Sun, Apr 15, 2018 at 04:11:04AM -0700, Matthew Wilcox wrote:
> > > Please just send one damn patch adding the typedef and the sparse
> > > annotation and converting everyone over.  Splitting this into five
> > > gazillion patches that do nothing doesn't help anyone.
> > 
> > The XFS one is trivial.  Many of the others are not.
> 
> if they arne't trivial you are doing something wrong.  That is you
> fix bugs that exist independent of the annotation together with the
> annotation.
> 
> Fix all the bugs first, and send them in (including for -stable) and
> then do the annotations in one big patch.

They aren't all bugs.  Some of them are just splitting errnos apart
from return values for typesafety.  The ones that are bugfixes are
unlikely-to-happen bugs like running out of memory when allocating a
page table, and returning "we inserted the PTE" instead of "we ran out
of memory".  More of a QOI issue than an exploitable bug.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Christoph Hellwig]

On Sun, Apr 15, 2018 at 05:34:27AM -0700, Matthew Wilcox wrote:
> On Sun, Apr 15, 2018 at 05:04:26AM -0700, Christoph Hellwig wrote:
> > On Sun, Apr 15, 2018 at 04:11:04AM -0700, Matthew Wilcox wrote:
> > > > Please just send one damn patch adding the typedef and the sparse
> > > > annotation and converting everyone over.  Splitting this into five
> > > > gazillion patches that do nothing doesn't help anyone.
> > > 
> > > The XFS one is trivial.  Many of the others are not.
> > 
> > if they arne't trivial you are doing something wrong.  That is you
> > fix bugs that exist independent of the annotation together with the
> > annotation.
> > 
> > Fix all the bugs first, and send them in (including for -stable) and
> > then do the annotations in one big patch.
> 
> They aren't all bugs.  Some of them are just splitting errnos apart
> from return values for typesafety.  The ones that are bugfixes are
> unlikely-to-happen bugs like running out of memory when allocating a
> page table, and returning "we inserted the PTE" instead of "we ran out
> of memory".  More of a QOI issue than an exploitable bug.

Then do all the cleanups first and do the batch rename of the type after
that, same concept.

One patch per functional change, not one change per subsystem, please.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Matthew Wilcox]

On Mon, Apr 16, 2018 at 01:57:07AM -0700, Christoph Hellwig wrote:
> On Sun, Apr 15, 2018 at 05:34:27AM -0700, Matthew Wilcox wrote:
> > On Sun, Apr 15, 2018 at 05:04:26AM -0700, Christoph Hellwig wrote:
> > > On Sun, Apr 15, 2018 at 04:11:04AM -0700, Matthew Wilcox wrote:
> > > > > Please just send one damn patch adding the typedef and the sparse
> > > > > annotation and converting everyone over.  Splitting this into five
> > > > > gazillion patches that do nothing doesn't help anyone.
> > > > 
> > > > The XFS one is trivial.  Many of the others are not.
> > > 
> > > if they arne't trivial you are doing something wrong.  That is you
> > > fix bugs that exist independent of the annotation together with the
> > > annotation.
> > > 
> > > Fix all the bugs first, and send them in (including for -stable) and
> > > then do the annotations in one big patch.
> > 
> > They aren't all bugs.  Some of them are just splitting errnos apart
> > from return values for typesafety.  The ones that are bugfixes are
> > unlikely-to-happen bugs like running out of memory when allocating a
> > page table, and returning "we inserted the PTE" instead of "we ran out
> > of memory".  More of a QOI issue than an exploitable bug.
> 
> Then do all the cleanups first and do the batch rename of the type after
> that, same concept.

That's an unreasonable request.  It would (usually) require writing more
code to convert errnos into VM_FAULT returns.  And that code would get
precisely zero testing upstream because the next commit would come along
and delete it, replacing it with calls to common code.

If anybody wants to backport any of this, they should just pick up the
common code (1c8f422059a "mm: change return type to vm_fault_t") and then
the commit that they want.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Christoph Hellwig]

On Mon, Apr 16, 2018 at 04:14:23AM -0700, Matthew Wilcox wrote:
> That's an unreasonable request.  It would (usually) require writing more
> code to convert errnos into VM_FAULT returns.  And that code would get
> precisely zero testing upstream because the next commit would come along
> and delete it, replacing it with calls to common code.

Well, introduce the damn common code first.  It does not require
using the typedef in any way.

In this current form of single patches:  NAK to anything I maintain
or review.

Send out an actual series, and think about how to order it best.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Matthew Wilcox]

On Mon, Apr 16, 2018 at 04:23:53AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 04:14:23AM -0700, Matthew Wilcox wrote:
> > That's an unreasonable request.  It would (usually) require writing more
> > code to convert errnos into VM_FAULT returns.  And that code would get
> > precisely zero testing upstream because the next commit would come along
> > and delete it, replacing it with calls to common code.
> 
> Well, introduce the damn common code first.  It does not require
> using the typedef in any way.

Already done.

> In this current form of single patches:  NAK to anything I maintain
> or review.

Yeah, we already have a plan for dealing with arsehole or MIA maintainers.
Fortunately, you're not actually the maintainer for XFS, so it'll be up
to Darrick whether to take it or not.

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Darrick J. Wong]

On Mon, Apr 16, 2018 at 04:31:39AM -0700, Matthew Wilcox wrote:
> On Mon, Apr 16, 2018 at 04:23:53AM -0700, Christoph Hellwig wrote:
> > On Mon, Apr 16, 2018 at 04:14:23AM -0700, Matthew Wilcox wrote:
> > > That's an unreasonable request.  It would (usually) require writing more
> > > code to convert errnos into VM_FAULT returns.  And that code would get
> > > precisely zero testing upstream because the next commit would come along
> > > and delete it, replacing it with calls to common code.
> > 
> > Well, introduce the damn common code first.  It does not require
> > using the typedef in any way.
> 
> Already done.

I wondered "Yeah, why isn't this one big series?  Where are the rest of
the patches to convert everything else?" and then realized the patch
author landed the function pointer prototype changes in mm.h prior to
4.17-rc1 and is now playing scattershot catch-up across the tree...

...hm, the original mm patch wasn't cc'd to fsdevel either, so that's
probably why I never heard of any of this until now.

So, uh, why wasn't this whole series (all the mm changes and all the
required fs changes) sent out for review prior to the merge window?

I don't like "Hey we bitrotted your filesystem without telling you, now
here's a patch to fix it..."  The code change looks ok, but now I'm
going to schedule some time to go digging to see if there's anything
else lurking beneath the surface here.

> > In this current form of single patches:  NAK to anything I maintain
> > or review.
> 
> Yeah, we already have a plan for dealing with arsehole or MIA maintainers.
> Fortunately, you're not actually the maintainer for XFS, so it'll be up
> to Darrick whether to take it or not.

I also dislike being pitted against the other XFS developers.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: xfs: Change return type to vm_fault_t

[Souptick Joarder]

On Mon, Apr 16, 2018 at 10:33 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Apr 16, 2018 at 04:31:39AM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 16, 2018 at 04:23:53AM -0700, Christoph Hellwig wrote:
>> > On Mon, Apr 16, 2018 at 04:14:23AM -0700, Matthew Wilcox wrote:
>> > > That's an unreasonable request.  It would (usually) require writing more
>> > > code to convert errnos into VM_FAULT returns.  And that code would get
>> > > precisely zero testing upstream because the next commit would come along
>> > > and delete it, replacing it with calls to common code.
>> >
>> > Well, introduce the damn common code first.  It does not require
>> > using the typedef in any way.
>>
>> Already done.
>
> I wondered "Yeah, why isn't this one big series?  Where are the rest of
> the patches to convert everything else?" and then realized the patch
> author landed the function pointer prototype changes in mm.h prior to
> 4.17-rc1 and is now playing scattershot catch-up across the tree...
>
> ...hm, the original mm patch wasn't cc'd to fsdevel either, so that's
> probably why I never heard of any of this until now.
>
> So, uh, why wasn't this whole series (all the mm changes and all the
> required fs changes) sent out for review prior to the merge window?
>
> I don't like "Hey we bitrotted your filesystem without telling you, now
> here's a patch to fix it..."  The code change looks ok, but now I'm
> going to schedule some time to go digging to see if there's anything
> else lurking beneath the surface here.
>
>> > In this current form of single patches:  NAK to anything I maintain
>> > or review.
>>
>> Yeah, we already have a plan for dealing with arsehole or MIA maintainers.
>> Fortunately, you're not actually the maintainer for XFS, so it'll be up
>> to Darrick whether to take it or not.
>
> I also dislike being pitted against the other XFS developers.
>

Thanks Darrick. This patch is now available in linux-next-20180530.