[Cluster-devel] [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Cluster-devel] [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for page_mkwrite
handler.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/gfs2/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 4f88e20..2c471d6 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -387,7 +387,7 @@ static int gfs2_allocate_page_backing(struct page *page)
  * blocks allocated on disk to back that page.
  */
 
-static int gfs2_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vmf->vma->vm_file);
-- 
1.9.1

Re: [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Bob Peterson]

----- Original Message -----
> Use new return type vm_fault_t for page_mkwrite
> handler.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  fs/gfs2/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 4f88e20..2c471d6 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -387,7 +387,7 @@ static int gfs2_allocate_page_backing(struct page *page)
>   * blocks allocated on disk to back that page.
>   */
>  
> -static int gfs2_page_mkwrite(struct vm_fault *vmf)
> +static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
>  {
>  	struct page *page = vmf->page;
>  	struct inode *inode = file_inode(vmf->vma->vm_file);
> --
> 1.9.1

Hi,

This patch is straightforward enough, but there are a lot of other
file systems that need similar patches. Shouldn't you do one big
patch set that fixes several file systems at once and run it through
Viro's kernel or Linus's kernel or something?
Adding Viro and linux-fsdevel for more opinions.

Regards,

Bob Peterson

[Cluster-devel] [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Bob Peterson]

----- Original Message -----
> Use new return type vm_fault_t for page_mkwrite
> handler.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  fs/gfs2/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 4f88e20..2c471d6 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -387,7 +387,7 @@ static int gfs2_allocate_page_backing(struct page *page)
>   * blocks allocated on disk to back that page.
>   */
>  
> -static int gfs2_page_mkwrite(struct vm_fault *vmf)
> +static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
>  {
>  	struct page *page = vmf->page;
>  	struct inode *inode = file_inode(vmf->vma->vm_file);
> --
> 1.9.1

Hi,

This patch is straightforward enough, but there are a lot of other
file systems that need similar patches. Shouldn't you do one big
patch set that fixes several file systems at once and run it through
Viro's kernel or Linus's kernel or something?
Adding Viro and linux-fsdevel for more opinions.

Regards,

Bob Peterson

Re: [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Souptick Joarder]

> Hi,
>
> This patch is straightforward enough, but there are a lot of other
> file systems that need similar patches. Shouldn't you do one big
> patch set that fixes several file systems at once and run it through
> Viro's kernel or Linus's kernel or something?
> Adding Viro and linux-fsdevel for more opinions.

The plan for these patches is to introduce the typedef, initially just
as documentation ("These functions should return a VM_FAULT_ status").
We'll trickle the patches to individual drivers/filesystems in through
the maintainers, as far as possible.  Then we'll change the typedef to
an unsigned int and break the compilation of any unconverted
drivers/filesystems.

We have already started sending out drivers/filesystems changes
to different maintainers.

Reference commit id - 1c8f422059ae5da07db7406ab916203f9417e396

[Cluster-devel] [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Souptick Joarder]

> Hi,
>
> This patch is straightforward enough, but there are a lot of other
> file systems that need similar patches. Shouldn't you do one big
> patch set that fixes several file systems at once and run it through
> Viro's kernel or Linus's kernel or something?
> Adding Viro and linux-fsdevel for more opinions.

The plan for these patches is to introduce the typedef, initially just
as documentation ("These functions should return a VM_FAULT_ status").
We'll trickle the patches to individual drivers/filesystems in through
the maintainers, as far as possible.  Then we'll change the typedef to
an unsigned int and break the compilation of any unconverted
drivers/filesystems.

We have already started sending out drivers/filesystems changes
to different maintainers.

Reference commit id - 1c8f422059ae5da07db7406ab916203f9417e396

Re: [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Dave Chinner]

On Mon, Apr 16, 2018 at 11:20:59PM +0530, Souptick Joarder wrote:
> > Hi,
> >
> > This patch is straightforward enough, but there are a lot of other
> > file systems that need similar patches. Shouldn't you do one big
> > patch set that fixes several file systems at once and run it through
> > Viro's kernel or Linus's kernel or something?
> > Adding Viro and linux-fsdevel for more opinions.
> 
> The plan for these patches is to introduce the typedef, initially just
> as documentation ("These functions should return a VM_FAULT_ status").
> We'll trickle the patches to individual drivers/filesystems in through
> the maintainers, as far as possible.  Then we'll change the typedef to
> an unsigned int and break the compilation of any unconverted
> drivers/filesystems.
>
> We have already started sending out drivers/filesystems changes
> to different maintainers.

Yes, we can see that. The response you are getting is "this is not
how we do cross-subsystem API changes.  Why are you doing it this
way?"

i.e. the problem being pointed out is that your process has not
followed the correct/normal process for proposing, reviewing and
mering cross-subsystem API changes. Bob has raised the same
questions as both Christoph and Darrick have asked in response to
the XFS patch. I only implied these questions by asking about
introducing useless typedefs with no context for reviewers...

I'd really like to have Darrick's questions answered(*) in a
constructive, non-abusive manner - I'll quote it here to get it all
in one thread on -fsdevel:

| ...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?

We're not asking for a description of what you are doing - we are
asking why the normal processes for proposing and merging such a
change is not being followed, and how you plan to rectify that.

Cheers,

Dave.

https://marc.info/?l=linux-xfs&m=152389824107375&w=2
-- 
Dave Chinner
david@fromorbit.com

[Cluster-devel] [PATCH] fs: gfs2: Adding new return type vm_fault_t

[Dave Chinner]

On Mon, Apr 16, 2018 at 11:20:59PM +0530, Souptick Joarder wrote:
> > Hi,
> >
> > This patch is straightforward enough, but there are a lot of other
> > file systems that need similar patches. Shouldn't you do one big
> > patch set that fixes several file systems at once and run it through
> > Viro's kernel or Linus's kernel or something?
> > Adding Viro and linux-fsdevel for more opinions.
> 
> The plan for these patches is to introduce the typedef, initially just
> as documentation ("These functions should return a VM_FAULT_ status").
> We'll trickle the patches to individual drivers/filesystems in through
> the maintainers, as far as possible.  Then we'll change the typedef to
> an unsigned int and break the compilation of any unconverted
> drivers/filesystems.
>
> We have already started sending out drivers/filesystems changes
> to different maintainers.

Yes, we can see that. The response you are getting is "this is not
how we do cross-subsystem API changes.  Why are you doing it this
way?"

i.e. the problem being pointed out is that your process has not
followed the correct/normal process for proposing, reviewing and
mering cross-subsystem API changes. Bob has raised the same
questions as both Christoph and Darrick have asked in response to
the XFS patch. I only implied these questions by asking about
introducing useless typedefs with no context for reviewers...

I'd really like to have Darrick's questions answered(*) in a
constructive, non-abusive manner - I'll quote it here to get it all
in one thread on -fsdevel:

| ...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?

We're not asking for a description of what you are doing - we are
asking why the normal processes for proposing and merging such a
change is not being followed, and how you plan to rectify that.

Cheers,

Dave.

https://marc.info/?l=linux-xfs&m=152389824107375&w=2
-- 
Dave Chinner
david at fromorbit.com

[PATCH] fs: gfs2: Adding new return type vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for gfs2_page_mkwrite
handler.

see commit 1c8f422059ae ("mm: change return type to
vm_fault_t") for reference.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/gfs2/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 4f88e20..2c471d6 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -387,7 +387,7 @@ static int gfs2_allocate_page_backing(struct page *page)
  * blocks allocated on disk to back that page.
  */
 
-static int gfs2_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vmf->vma->vm_file);
-- 
1.9.1