Re: [PATCH] ext4: Convert int to vm_fault_t type

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Souptick Joarder]

On Sat, Jul 28, 2018 at 2:20 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> Use new return type vm_fault_t for ext4_page_mkwrite
> handler and block_page_mkwrite_return.
>
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>

As part of commit 2fae376c0a16db42eb438 in linux-next tree,
this conversion was missed. Sorry about it.

> ---
>  fs/ext4/ext4.h              |  2 +-
>  fs/ext4/inode.c             | 20 ++++++++++----------
>  include/linux/buffer_head.h |  3 ++-
>  3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index aec0010..1183773 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2469,7 +2469,7 @@ int do_journal_get_write_access(handle_t *handle,
>  extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
>  extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
>                              loff_t lstart, loff_t lend);
> -extern int ext4_page_mkwrite(struct vm_fault *vmf);
> +extern vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf);
>  extern vm_fault_t ext4_filemap_fault(struct vm_fault *vmf);
>  extern qsize_t *ext4_get_reserved_space(struct inode *inode);
>  extern int ext4_get_projid(struct inode *inode, kprojid_t *projid);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 03ac322..b491fdb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6108,27 +6108,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
>         return !buffer_mapped(bh);
>  }
>
> -int ext4_page_mkwrite(struct vm_fault *vmf)
> +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  {
>         struct vm_area_struct *vma = vmf->vma;
>         struct page *page = vmf->page;
>         loff_t size;
>         unsigned long len;
> -       int ret;
> +       vm_fault_t ret;
>         struct file *file = vma->vm_file;
>         struct inode *inode = file_inode(file);
>         struct address_space *mapping = inode->i_mapping;
>         handle_t *handle;
>         get_block_t *get_block;
> -       int retries = 0;
> +       int retries = 0, err;
>
>         sb_start_pagefault(inode->i_sb);
>         file_update_time(vma->vm_file);
>
>         down_read(&EXT4_I(inode)->i_mmap_sem);
>
> -       ret = ext4_convert_inline_data(inode);
> -       if (ret)
> +       err = ext4_convert_inline_data(inode);
> +       if (err)
>                 goto out_ret;
>
>         /* Delalloc case is easy... */
> @@ -6138,9 +6138,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
>                 do {
>                         ret = block_page_mkwrite(vma, vmf,
>                                                    ext4_da_get_block_prep);
> -               } while (ret == -ENOSPC &&
> +               } while (ret == VM_FAULT_SIGBUS &&
>                        ext4_should_retry_alloc(inode->i_sb, &retries));
> -               goto out_ret;
> +               goto out;
>         }
>
>         lock_page(page);
> @@ -6188,17 +6188,17 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
>                 if (ext4_walk_page_buffers(handle, page_buffers(page), 0,
>                           PAGE_SIZE, NULL, do_journal_get_write_access)) {
>                         unlock_page(page);
> -                       ret = VM_FAULT_SIGBUS;
>                         ext4_journal_stop(handle);
>                         goto out;
>                 }
>                 ext4_set_inode_state(inode, EXT4_STATE_JDATA);
>         }
>         ext4_journal_stop(handle);
> -       if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
> +       if (ret == VM_FAULT_SIGBUS &&
> +               ext4_should_retry_alloc(inode->i_sb, &retries))
>                 goto retry_alloc;
>  out_ret:
> -       ret = block_page_mkwrite_return(ret);
> +       ret = block_page_mkwrite_return(err);
>  out:
>         up_read(&EXT4_I(inode)->i_mmap_sem);
>         sb_end_pagefault(inode->i_sb);
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 96225a7..ed7a81b 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -14,6 +14,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/wait.h>
>  #include <linux/atomic.h>
> +#include <linux/mm_types.h>
>
>  #ifdef CONFIG_BLOCK
>
> @@ -242,7 +243,7 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
>  int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
>                                 get_block_t get_block);
>  /* Convert errno to return value from ->page_mkwrite() call */
> -static inline int block_page_mkwrite_return(int err)
> +static inline vm_fault_t block_page_mkwrite_return(int err)
>  {
>         if (err == 0)
>                 return VM_FAULT_LOCKED;
> --
> 1.9.1
>

[PATCH] ext4: Convert int to vm_fault_t type

[Souptick Joarder]

Use new return type vm_fault_t for ext4_page_mkwrite
handler and block_page_mkwrite_return.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 fs/ext4/ext4.h              |  2 +-
 fs/ext4/inode.c             | 20 ++++++++++----------
 include/linux/buffer_head.h |  3 ++-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index aec0010..1183773 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2469,7 +2469,7 @@ int do_journal_get_write_access(handle_t *handle,
 extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
 extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
 			     loff_t lstart, loff_t lend);
-extern int ext4_page_mkwrite(struct vm_fault *vmf);
+extern vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf);
 extern vm_fault_t ext4_filemap_fault(struct vm_fault *vmf);
 extern qsize_t *ext4_get_reserved_space(struct inode *inode);
 extern int ext4_get_projid(struct inode *inode, kprojid_t *projid);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 03ac322..b491fdb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6108,27 +6108,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
 	return !buffer_mapped(bh);
 }
 
-int ext4_page_mkwrite(struct vm_fault *vmf)
+vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct page *page = vmf->page;
 	loff_t size;
 	unsigned long len;
-	int ret;
+	vm_fault_t ret;
 	struct file *file = vma->vm_file;
 	struct inode *inode = file_inode(file);
 	struct address_space *mapping = inode->i_mapping;
 	handle_t *handle;
 	get_block_t *get_block;
-	int retries = 0;
+	int retries = 0, err;
 
 	sb_start_pagefault(inode->i_sb);
 	file_update_time(vma->vm_file);
 
 	down_read(&EXT4_I(inode)->i_mmap_sem);
 
-	ret = ext4_convert_inline_data(inode);
-	if (ret)
+	err = ext4_convert_inline_data(inode);
+	if (err)
 		goto out_ret;
 
 	/* Delalloc case is easy... */
@@ -6138,9 +6138,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 		do {
 			ret = block_page_mkwrite(vma, vmf,
 						   ext4_da_get_block_prep);
-		} while (ret == -ENOSPC &&
+		} while (ret == VM_FAULT_SIGBUS &&
 		       ext4_should_retry_alloc(inode->i_sb, &retries));
-		goto out_ret;
+		goto out;
 	}
 
 	lock_page(page);
@@ -6188,17 +6188,17 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 		if (ext4_walk_page_buffers(handle, page_buffers(page), 0,
 			  PAGE_SIZE, NULL, do_journal_get_write_access)) {
 			unlock_page(page);
-			ret = VM_FAULT_SIGBUS;
 			ext4_journal_stop(handle);
 			goto out;
 		}
 		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 	}
 	ext4_journal_stop(handle);
-	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+	if (ret == VM_FAULT_SIGBUS &&
+		ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry_alloc;
 out_ret:
-	ret = block_page_mkwrite_return(ret);
+	ret = block_page_mkwrite_return(err);
 out:
 	up_read(&EXT4_I(inode)->i_mmap_sem);
 	sb_end_pagefault(inode->i_sb);
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 96225a7..ed7a81b 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -14,6 +14,7 @@
 #include <linux/pagemap.h>
 #include <linux/wait.h>
 #include <linux/atomic.h>
+#include <linux/mm_types.h>
 
 #ifdef CONFIG_BLOCK
 
@@ -242,7 +243,7 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
 int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 				get_block_t get_block);
 /* Convert errno to return value from ->page_mkwrite() call */
-static inline int block_page_mkwrite_return(int err)
+static inline vm_fault_t block_page_mkwrite_return(int err)
 {
 	if (err == 0)
 		return VM_FAULT_LOCKED;
-- 
1.9.1

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Theodore Y. Ts'o]

On Sat, Jul 28, 2018 at 02:20:00PM +0530, Souptick Joarder wrote:
> Use new return type vm_fault_t for ext4_page_mkwrite
> handler and block_page_mkwrite_return.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>

FYI, this patch was very sloppy, and didn't do the right thing.  That's
because of how you messed with the changing how the return codes are
now handled.

> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6108,27 +6108,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
>  	return !buffer_mapped(bh);
>  }
>  
> -int ext4_page_mkwrite(struct vm_fault *vmf)
> +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct page *page = vmf->page;
>  	loff_t size;
>  	unsigned long len;
> -	int ret;
> +	vm_fault_t ret;
>  	struct file *file = vma->vm_file;
>  	struct inode *inode = file_inode(file);
>  	struct address_space *mapping = inode->i_mapping;
>  	handle_t *handle;
>  	get_block_t *get_block;
> -	int retries = 0;
> +	int retries = 0, err;

OK, ret now is a vm_fault_t, and err is an error return....

> @@ -6138,9 +6138,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
>  		do {
>  			ret = block_page_mkwrite(vma, vmf,
>  						   ext4_da_get_block_prep);

But block_page_mkwrite() still returns an int, not a vm_fault_t....

>  -		} while (ret == -ENOSPC &&
> +		} while (ret == VM_FAULT_SIGBUS &&
>  		       ext4_should_retry_alloc(inode->i_sb, &retries));

So this is Just wrong,  This needed to be:

  		do {
  			err = block_page_mkwrite(vma, vmf,
  						   ext4_da_get_block_prep);
		} while (err == -ENOSPC &&
  		         ext4_should_retry_alloc(inode->i_sb, &retries));
		goto out_ret;

That's because out_ret is what will translate the int error code to
the vm_fault_t via:

	ret = block_page_mkwrite_return(err);

The fact that ext4_page_mkwrite() returns a vm_fault_t, while
block_page_mkwrite() returns an int which then has to get translated
into a vm_fault_t via block_page_mkwrite_return() is I suspect going
to confuse an awful lot of callers.

I'll fix up the patch, but I just wanted to call your attention to
this pitfall in the patch which confused even you as the patch author....

(BTW, the buggy patch triggered a new failure, ext4/307, which is how
I noticed that the patch was all wrong.  If you had run any kind of
static code checker you would have noticed that block_page_mkwrite()
was returning an int and that was getting assigned into a variable of
type vm_fault_t.  The fact that you *didn't* notice makes me worry
that all of this code churn may, in the end, not actually help us as
much as we thought.  :-(

     	     	    	  		      - Ted

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Souptick Joarder]

On Wed, Aug 1, 2018 at 6:25 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Sat, Jul 28, 2018 at 02:20:00PM +0530, Souptick Joarder wrote:
>> Use new return type vm_fault_t for ext4_page_mkwrite
>> handler and block_page_mkwrite_return.
>>
>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>
> FYI, this patch was very sloppy, and didn't do the right thing.  That's
> because of how you messed with the changing how the return codes are
> now handled.
>
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -6108,27 +6108,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
>>       return !buffer_mapped(bh);
>>  }
>>
>> -int ext4_page_mkwrite(struct vm_fault *vmf)
>> +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>>  {
>>       struct vm_area_struct *vma = vmf->vma;
>>       struct page *page = vmf->page;
>>       loff_t size;
>>       unsigned long len;
>> -     int ret;
>> +     vm_fault_t ret;
>>       struct file *file = vma->vm_file;
>>       struct inode *inode = file_inode(file);
>>       struct address_space *mapping = inode->i_mapping;
>>       handle_t *handle;
>>       get_block_t *get_block;
>> -     int retries = 0;
>> +     int retries = 0, err;
>
> OK, ret now is a vm_fault_t, and err is an error return....
>
>> @@ -6138,9 +6138,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
>>               do {
>>                       ret = block_page_mkwrite(vma, vmf,
>>                                                  ext4_da_get_block_prep);
>
> But block_page_mkwrite() still returns an int, not a vm_fault_t....
>
>>  -            } while (ret == -ENOSPC &&
>> +             } while (ret == VM_FAULT_SIGBUS &&
>>                      ext4_should_retry_alloc(inode->i_sb, &retries));
>
> So this is Just wrong,  This needed to be:
>
>                 do {
>                         err = block_page_mkwrite(vma, vmf,
>                                                    ext4_da_get_block_prep);
>                 } while (err == -ENOSPC &&
>                          ext4_should_retry_alloc(inode->i_sb, &retries));
>                 goto out_ret;
>
> That's because out_ret is what will translate the int error code to
> the vm_fault_t via:
>
>         ret = block_page_mkwrite_return(err);
>
> The fact that ext4_page_mkwrite() returns a vm_fault_t, while
> block_page_mkwrite() returns an int which then has to get translated
> into a vm_fault_t via block_page_mkwrite_return() is I suspect going
> to confuse an awful lot of callers.

We have also changed block_page_mkwrite() to return vm_fault_t, but in
a different patch. Hopefully that patch will be in linux-next tree soon.

>
> I'll fix up the patch, but I just wanted to call your attention to
> this pitfall in the patch which confused even you as the patch author....
>
> (BTW, the buggy patch triggered a new failure, ext4/307, which is how
> I noticed that the patch was all wrong.  If you had run any kind of
> static code checker you would have noticed that block_page_mkwrite()
> was returning an int and that was getting assigned into a variable of
> type vm_fault_t.  The fact that you *didn't* notice makes me worry
> that all of this code churn may, in the end, not actually help us as
> much as we thought.  :-(
>
>                                               - Ted

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Souptick Joarder]

On Wed, Aug 1, 2018 at 6:34 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> On Wed, Aug 1, 2018 at 6:25 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>> On Sat, Jul 28, 2018 at 02:20:00PM +0530, Souptick Joarder wrote:
>>> Use new return type vm_fault_t for ext4_page_mkwrite
>>> handler and block_page_mkwrite_return.
>>>
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>
>> FYI, this patch was very sloppy, and didn't do the right thing.  That's
>> because of how you messed with the changing how the return codes are
>> now handled.
>>
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -6108,27 +6108,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
>>>       return !buffer_mapped(bh);
>>>  }
>>>
>>> -int ext4_page_mkwrite(struct vm_fault *vmf)
>>> +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>>>  {
>>>       struct vm_area_struct *vma = vmf->vma;
>>>       struct page *page = vmf->page;
>>>       loff_t size;
>>>       unsigned long len;
>>> -     int ret;
>>> +     vm_fault_t ret;
>>>       struct file *file = vma->vm_file;
>>>       struct inode *inode = file_inode(file);
>>>       struct address_space *mapping = inode->i_mapping;
>>>       handle_t *handle;
>>>       get_block_t *get_block;
>>> -     int retries = 0;
>>> +     int retries = 0, err;
>>
>> OK, ret now is a vm_fault_t, and err is an error return....
>>
>>> @@ -6138,9 +6138,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
>>>               do {
>>>                       ret = block_page_mkwrite(vma, vmf,
>>>                                                  ext4_da_get_block_prep);
>>
>> But block_page_mkwrite() still returns an int, not a vm_fault_t....
>>
>>>  -            } while (ret == -ENOSPC &&
>>> +             } while (ret == VM_FAULT_SIGBUS &&
>>>                      ext4_should_retry_alloc(inode->i_sb, &retries));
>>
>> So this is Just wrong,  This needed to be:
>>
>>                 do {
>>                         err = block_page_mkwrite(vma, vmf,
>>                                                    ext4_da_get_block_prep);
>>                 } while (err == -ENOSPC &&
>>                          ext4_should_retry_alloc(inode->i_sb, &retries));
>>                 goto out_ret;
>>
>> That's because out_ret is what will translate the int error code to
>> the vm_fault_t via:
>>
>>         ret = block_page_mkwrite_return(err);
>>
>> The fact that ext4_page_mkwrite() returns a vm_fault_t, while
>> block_page_mkwrite() returns an int which then has to get translated
>> into a vm_fault_t via block_page_mkwrite_return() is I suspect going
>> to confuse an awful lot of callers.
>
> We have also changed block_page_mkwrite() to return vm_fault_t, but in
> a different patch. Hopefully that patch will be in linux-next tree soon.

Link to other patch where block_page_mkwrite() is changed to return
vm_fault_t type.

https://www.spinics.net/lists/linux-fsdevel/msg130670.html

>
>>
>> I'll fix up the patch, but I just wanted to call your attention to
>> this pitfall in the patch which confused even you as the patch author....
>>
>> (BTW, the buggy patch triggered a new failure, ext4/307, which is how
>> I noticed that the patch was all wrong.  If you had run any kind of
>> static code checker you would have noticed that block_page_mkwrite()
>> was returning an int and that was getting assigned into a variable of
>> type vm_fault_t.  The fact that you *didn't* notice makes me worry
>> that all of this code churn may, in the end, not actually help us as
>> much as we thought.  :-(
>>
>>                                               - Ted

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Matthew Wilcox]

On Wed, Aug 01, 2018 at 06:34:45PM +0530, Souptick Joarder wrote:
> > The fact that ext4_page_mkwrite() returns a vm_fault_t, while
> > block_page_mkwrite() returns an int which then has to get translated
> > into a vm_fault_t via block_page_mkwrite_return() is I suspect going
> > to confuse an awful lot of callers.
> 
> We have also changed block_page_mkwrite() to return vm_fault_t, but in
> a different patch. Hopefully that patch will be in linux-next tree soon.

I didn't sign off on that, so that's not "we", but "I".  And this is
completely against everything I've been telling you for this whole effort.
Patches should each make sense individually.  You can't make this patch
dependent on another patch without putting that in writing.

Leave block_page_mkwrite() alone for now.  Eventually it should return
a vm_fault_t, probably.  But that patch needs to be delayed at least
one kernel cycle.

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Souptick Joarder]

On Wed, Aug 1, 2018 at 6:43 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Aug 01, 2018 at 06:34:45PM +0530, Souptick Joarder wrote:
>> > The fact that ext4_page_mkwrite() returns a vm_fault_t, while
>> > block_page_mkwrite() returns an int which then has to get translated
>> > into a vm_fault_t via block_page_mkwrite_return() is I suspect going
>> > to confuse an awful lot of callers.
>>
>> We have also changed block_page_mkwrite() to return vm_fault_t, but in
>> a different patch. Hopefully that patch will be in linux-next tree soon.
>
> I didn't sign off on that, so that's not "we", but "I".  And this is
> completely against everything I've been telling you for this whole effort.
> Patches should each make sense individually.  You can't make this patch
> dependent on another patch without putting that in writing.

It was mistake form my side. Sorry about it.

>
> Leave block_page_mkwrite() alone for now.  Eventually it should return
> a vm_fault_t, probably.  But that patch needs to be delayed at least
> one kernel cycle.

As caller of block_page_mkwrite() are -
fs/ext4/inode.c
fs/nilfs2/file.c

I will merge both changes in a single patch and send it.

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Theodore Y. Ts'o]

On Wed, Aug 01, 2018 at 06:56:39PM +0530, Souptick Joarder wrote:
> As caller of block_page_mkwrite() are -
> fs/ext4/inode.c
> fs/nilfs2/file.c
> 
> I will merge both changes in a single patch and send it.

Note that it's *important* for ext4 that we know what kind of error
was returned by the helper function passed into block_page_write() and
called by it.  In particular, whether the error was ENOSPC or not.

That's why this change should have raised all sorts of alarums:

 	ext4_journal_stop(handle);
-	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+	if (ret == VM_FAULT_SIGBUS &&
+		ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry_alloc;

Note the ret == -ENOSPC --- you blindly changed it to return
VM_FAULT_SIGBUS!!!

You need to understand *why* the code does what it does, and not just
make blind mechanical replacements.

In this particular case, it probably means that if you insist on
making block_page_mkwrite() return vmfault_t, it will probably need to
take an optional "int *err" parameter, so the error can be returned to
the caller if the caller needs it.

I'm going to drop the whole ext4 changes for vm_fault_t for this
cycle, and I'll let you try to fix it up properly for the next cycle.

Regards,

						- Ted

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Theodore Y. Ts'o]

On Wed, Aug 01, 2018 at 10:38:30AM -0400, Theodore Y. Ts'o wrote:
> I'm going to drop the whole ext4 changes for vm_fault_t for this
> cycle, and I'll let you try to fix it up properly for the next cycle.

Here's the fixed up commit that I'm going to drop since you plan to be
making changes in block_page_mkpage(), and I don't want us to get out
of sync.

						- Ted

commit 37ec0b791ff90c6fe480fdf74c7df934c1756819
Author: Souptick Joarder <jrdr.linux@gmail.com>
Date:   Wed Aug 1 11:54:31 2018 -0400

    ext4: use new return type vm_fault_t
    
    Use new return type vm_fault_t for fault handler
    ext4_filemap_fault.
    
    Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 6d7dec48372b..21fb1964a672 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2468,8 +2468,8 @@ extern int ext4_writepage_trans_blocks(struct inode *);
 extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
 extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
 			     loff_t lstart, loff_t lend);
-extern int ext4_page_mkwrite(struct vm_fault *vmf);
-extern int ext4_filemap_fault(struct vm_fault *vmf);
+extern vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf);
+extern vm_fault_t ext4_filemap_fault(struct vm_fault *vmf);
 extern qsize_t *ext4_get_reserved_space(struct inode *inode);
 extern int ext4_get_projid(struct inode *inode, kprojid_t *projid);
 extern void ext4_da_update_reserve_space(struct inode *inode,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ba0de19fb1ad..a6da9eda2194 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6107,27 +6107,27 @@ static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
 	return !buffer_mapped(bh);
 }
 
-int ext4_page_mkwrite(struct vm_fault *vmf)
+vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct page *page = vmf->page;
 	loff_t size;
 	unsigned long len;
-	int ret;
+	vm_fault_t ret;
 	struct file *file = vma->vm_file;
 	struct inode *inode = file_inode(file);
 	struct address_space *mapping = inode->i_mapping;
 	handle_t *handle;
 	get_block_t *get_block;
-	int retries = 0;
+	int retries = 0, err;
 
 	sb_start_pagefault(inode->i_sb);
 	file_update_time(vma->vm_file);
 
 	down_read(&EXT4_I(inode)->i_mmap_sem);
 
-	ret = ext4_convert_inline_data(inode);
-	if (ret)
+	err = ext4_convert_inline_data(inode);
+	if (err)
 		goto out_ret;
 
 	/* Delalloc case is easy... */
@@ -6135,9 +6135,9 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 	    !ext4_should_journal_data(inode) &&
 	    !ext4_nonda_switch(inode->i_sb)) {
 		do {
-			ret = block_page_mkwrite(vma, vmf,
-						   ext4_da_get_block_prep);
-		} while (ret == -ENOSPC &&
+			err = block_page_mkwrite(vma, vmf,
+						 ext4_da_get_block_prep);
+		} while (err == -ENOSPC &&
 		       ext4_should_retry_alloc(inode->i_sb, &retries));
 		goto out_ret;
 	}
@@ -6182,8 +6182,8 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 		ret = VM_FAULT_SIGBUS;
 		goto out;
 	}
-	ret = block_page_mkwrite(vma, vmf, get_block);
-	if (!ret && ext4_should_journal_data(inode)) {
+	err = block_page_mkwrite(vma, vmf, get_block);
+	if (!err && ext4_should_journal_data(inode)) {
 		if (ext4_walk_page_buffers(handle, page_buffers(page), 0,
 			  PAGE_SIZE, NULL, do_journal_get_write_access)) {
 			unlock_page(page);
@@ -6194,24 +6194,24 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 	}
 	ext4_journal_stop(handle);
-	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry_alloc;
 out_ret:
-	ret = block_page_mkwrite_return(ret);
+	ret = block_page_mkwrite_return(err);
 out:
 	up_read(&EXT4_I(inode)->i_mmap_sem);
 	sb_end_pagefault(inode->i_sb);
 	return ret;
 }
 
-int ext4_filemap_fault(struct vm_fault *vmf)
+vm_fault_t ext4_filemap_fault(struct vm_fault *vmf)
 {
 	struct inode *inode = file_inode(vmf->vma->vm_file);
-	int err;
+	vm_fault_t ret;
 
 	down_read(&EXT4_I(inode)->i_mmap_sem);
-	err = filemap_fault(vmf);
+	ret = filemap_fault(vmf);
 	up_read(&EXT4_I(inode)->i_mmap_sem);
 
-	return err;
+	return ret;
 }
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 96225a77c112..ed7a81b8f7cc 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -14,6 +14,7 @@
 #include <linux/pagemap.h>
 #include <linux/wait.h>
 #include <linux/atomic.h>
+#include <linux/mm_types.h>
 
 #ifdef CONFIG_BLOCK
 
@@ -242,7 +243,7 @@ int block_commit_write(struct page *page, unsigned from, unsigned to);
 int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 				get_block_t get_block);
 /* Convert errno to return value from ->page_mkwrite() call */
-static inline int block_page_mkwrite_return(int err)
+static inline vm_fault_t block_page_mkwrite_return(int err)
 {
 	if (err == 0)
 		return VM_FAULT_LOCKED;

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Matthew Wilcox]

On Wed, Aug 01, 2018 at 12:06:18PM -0400, Theodore Y. Ts'o wrote:
> On Wed, Aug 01, 2018 at 10:38:30AM -0400, Theodore Y. Ts'o wrote:
> > I'm going to drop the whole ext4 changes for vm_fault_t for this
> > cycle, and I'll let you try to fix it up properly for the next cycle.
> 
> Here's the fixed up commit that I'm going to drop since you plan to be
> making changes in block_page_mkpage(), and I don't want us to get out
> of sync.

This looks sane to me.

Re: [PATCH] ext4: Convert int to vm_fault_t type

[Theodore Y. Ts'o]

On Wed, Aug 01, 2018 at 09:13:19AM -0700, Matthew Wilcox wrote:
> On Wed, Aug 01, 2018 at 12:06:18PM -0400, Theodore Y. Ts'o wrote:
> > On Wed, Aug 01, 2018 at 10:38:30AM -0400, Theodore Y. Ts'o wrote:
> > > I'm going to drop the whole ext4 changes for vm_fault_t for this
> > > cycle, and I'll let you try to fix it up properly for the next cycle.
> > 
> > Here's the fixed up commit that I'm going to drop since you plan to be
> > making changes in block_page_mkwrite(), and I don't want us to get out
> > of sync.
> 
> This looks sane to me.

Yeah, it's fine except for the cognitive load to the file system
programmer where block_page_mkwrite() returns an int, and not a
vm_fault_t, and you have use block_page_mkwrite_return() in order to
convert from the negative error code convention to a vm_fault_t.

Souptick has a separate patch out which changes block_page_mkpage() to
return a vm_fault_t.  It's broken in that it clobbers the error return
and doesn't provide a way for the caller to get the error return.  So
Souptick, please consider that other patch to have received a NACK
from me, as it *will* break ext4.

Souptick, perhaps you could change block_page_mkwrite() so that its
function signature looks like this instead:

vm_fault_t block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
	   		      get_block_t get_block, int *err)

...that's sane.  Or you can maybe simply change the *name* of the
function so it's clear it's differnt from all other xxx_page_mkwrite()
functions in that it returns an int.

I'll let you decide what you want to do --- since part of your
development to be a sophisticated programmer is to get experience
making these sorts of decisions that involve having good programming
"taste".

Regards,

						- Ted