fs: xfs: Change return type to vm_fault_t

PATCH In review Souptick Joarder 5 participants 13 messages Apr 14, 2018 – May 30, 2018 XFS filesystem

lore

Share & tools

lore

Souptick Joarder

author

2018-04-14 20:03 UTC #0 lore raw

PATCH 1/1 fs: xfs: Change return type to vm_fault_t

Use new return type vm_fault_t for fault handlers.

Signed-off-by Souptick Joarder <[email protected]>

Reviewed-by Matthew Wilcox <[email protected]>

---
 fs/xfs/xfs_file.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

fs/xfs/xfs_file.c +6 -6

index 9ea0832..988ec56 100644
@@ -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)
 {

Signature · 1 line

1.9.1

Dave Chinner

2018-04-14 21:59 UTC #1 lore raw

Quoted 4 lines · Souptick Joarder

Use new return type vm_fault_t for fault handlers.
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Matthew Wilcox <[email protected]>

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

typedef int vm_fault_t;

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Matthew Wilcox

2018-04-14 22:24 UTC #2 lore raw

Quoted 9 lines · Dave Chinner

Quoted 4 lines · Souptick Joarder
Use new return type vm_fault_t for fault handlers.
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Matthew Wilcox <[email protected]>
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.

Christoph Hellwig

2018-04-15 07:21 UTC #3 lore raw

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · Dave Chinner
Quoted 4 lines · Souptick Joarder
Use new return type vm_fault_t for fault handlers.
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Matthew Wilcox <[email protected]>

Quoted 7 lines

Quoted 3 lines
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.

Matthew Wilcox

2018-04-15 11:11 UTC #4 lore raw

Quoted 12 lines · Christoph Hellwig

Quoted 11 lines · Matthew Wilcox
Quoted 9 lines · Dave Chinner
Quoted 4 lines · Souptick Joarder
Use new return type vm_fault_t for fault handlers.
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Matthew Wilcox <[email protected]>
Ummm. Why are we adding typedefs to hide basic types?
typedef int vm_fault_t;

Quoted 7 lines

Quoted 3 lines
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.

Christoph Hellwig

2018-04-15 12:04 UTC #5 lore raw

Quoted 5 lines · Matthew Wilcox

Quoted 3 lines
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.

Matthew Wilcox

2018-04-15 12:34 UTC #6 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Matthew Wilcox
Quoted 3 lines
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.

Quoted 8 lines

Quoted 1 line
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.

Christoph Hellwig

2018-04-16 08:57 UTC #7 lore raw

Quoted 12 lines · Matthew Wilcox

Quoted 11 lines · Christoph Hellwig
Quoted 5 lines · Matthew Wilcox
Quoted 3 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Matthew Wilcox

2018-04-16 11:14 UTC #8 lore raw

Quoted 16 lines · Christoph Hellwig

Quoted 15 lines · Matthew Wilcox
Quoted 13 lines · Christoph Hellwig
Quoted 5 lines · Matthew Wilcox
Quoted 3 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Christoph Hellwig

2018-04-16 11:23 UTC #9 lore raw

Quoted 4 lines · Matthew Wilcox

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.

Matthew Wilcox

2018-04-16 11:31 UTC #10 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 4 lines · Matthew Wilcox
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.

Quoted 2 lines

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.

Darrick J. Wong

2018-04-16 17:03 UTC #11 lore raw

Quoted 11 lines · Matthew Wilcox

Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Matthew Wilcox
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.

Quoted 6 lines

Quoted 2 lines
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

Quoted 4 lines

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

Souptick Joarder

author

2018-05-30 12:35 UTC #12 lore raw

Quoted 29 lines · Darrick J. Wong

Quoted 11 lines · Matthew Wilcox
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Matthew Wilcox
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.

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
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.