linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] fs: Convert return type int to vm_fault_t
@ 2018-08-30 17:25 Souptick Joarder
  2018-08-30 23:33 ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Souptick Joarder @ 2018-08-30 17:25 UTC (permalink / raw)
  To: akpm, konishi.ryusuke
  Cc: viro, tytso, adilger.kernel, axboe, darrick.wong, ebiggers,
	pombredanne, agruenba, gregkh, kemi.wang, willy, linux-fsdevel,
	linux-kernel, linux-ext4, linux-nilfs

Return type for fault handlers  in ext4 and nilfs are
changed to use vm_fault_t.

Return type of block_page_mkwrite() is changed from
int to vm_fault_t. The function signature of
block_page_mkwrite() is changed to add one new parameter
int *err. This will provide a way for caller functions
to get error value along with return value and use it
further.

Return type of block_page_mkwrite_return() is also changed
to use new vm_fault_t type.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
v2: return type of nilfs_page_mkwrite() changed to vm_fault_t
    Address Ryusuke's comment. remove err =0.

 fs/buffer.c                 | 20 +++++++++++---------
 fs/ext4/ext4.h              |  4 ++--
 fs/ext4/inode.c             | 31 +++++++++++++++----------------
 fs/nilfs2/file.c            | 17 ++++++++++-------
 include/linux/buffer_head.h |  7 ++++---
 5 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index cabc045..94b8086 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2444,21 +2444,21 @@ int block_commit_write(struct page *page, unsigned from, unsigned to)
  * Direct callers of this function should protect against filesystem freezing
  * using sb_start_pagefault() - sb_end_pagefault() functions.
  */
-int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
-			 get_block_t get_block)
+vm_fault_t block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+			 get_block_t get_block, int *err)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vma->vm_file);
 	unsigned long end;
 	loff_t size;
-	int ret;
+	int err1;
 
 	lock_page(page);
 	size = i_size_read(inode);
 	if ((page->mapping != inode->i_mapping) ||
 	    (page_offset(page) > size)) {
 		/* We overload EFAULT to mean page got truncated */
-		ret = -EFAULT;
+		err1 = -EFAULT;
 		goto out_unlock;
 	}
 
@@ -2468,18 +2468,20 @@ int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 	else
 		end = PAGE_SIZE;
 
-	ret = __block_write_begin(page, 0, end, get_block);
-	if (!ret)
-		ret = block_commit_write(page, 0, end);
+	err1 = __block_write_begin(page, 0, end, get_block);
+	if (!err1)
+		err1 = block_commit_write(page, 0, end);
 
-	if (unlikely(ret < 0))
+	if (unlikely(err1 < 0))
 		goto out_unlock;
+	*err = err1;
 	set_page_dirty(page);
 	wait_for_stable_page(page);
 	return 0;
 out_unlock:
 	unlock_page(page);
-	return ret;
+	*err = err1;
+	return block_page_mkwrite_return(err1);
 }
 EXPORT_SYMBOL(block_page_mkwrite);
 
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 7c7123f..bff7b42 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2465,8 +2465,8 @@ 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 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 7d6c100..c597483 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6109,27 +6109,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,10 +6138,10 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 	    !ext4_nonda_switch(inode->i_sb)) {
 		do {
 			ret = block_page_mkwrite(vma, vmf,
-						   ext4_da_get_block_prep);
-		} while (ret == -ENOSPC &&
+						ext4_da_get_block_prep, &err);
+		} while (err == -ENOSPC &&
 		       ext4_should_retry_alloc(inode->i_sb, &retries));
-		goto out_ret;
+		goto out;
 	}
 
 	lock_page(page);
@@ -6184,36 +6184,35 @@ int ext4_page_mkwrite(struct vm_fault *vmf)
 		ret = VM_FAULT_SIGBUS;
 		goto out;
 	}
-	ret = block_page_mkwrite(vma, vmf, get_block);
+	ret = block_page_mkwrite(vma, vmf, get_block, &err);
 	if (!ret && 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);
-			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 (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/fs/nilfs2/file.c b/fs/nilfs2/file.c
index c5fa3de..3533efe 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	return err;
 }
 
-static int nilfs_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vma->vm_file);
 	struct nilfs_transaction_info ti;
-	int ret = 0;
+	vm_fault_t ret = VM_FAULT_LOCKED;
+	int err = 0;
 
 	if (unlikely(nilfs_near_disk_full(inode->i_sb->s_fs_info)))
 		return VM_FAULT_SIGBUS; /* -ENOSPC */
@@ -67,7 +68,7 @@ static int nilfs_page_mkwrite(struct vm_fault *vmf)
 	if (page->mapping != inode->i_mapping ||
 	    page_offset(page) >= i_size_read(inode) || !PageUptodate(page)) {
 		unlock_page(page);
-		ret = -EFAULT;	/* make the VM retry the fault */
+		ret = VM_FAULT_NOPAGE;	/* make the VM retry the fault */
 		goto out;
 	}
 
@@ -99,13 +100,15 @@ static int nilfs_page_mkwrite(struct vm_fault *vmf)
 	/*
 	 * fill hole blocks
 	 */
-	ret = nilfs_transaction_begin(inode->i_sb, &ti, 1);
+	err = nilfs_transaction_begin(inode->i_sb, &ti, 1);
 	/* never returns -ENOMEM, but may return -ENOSPC */
-	if (unlikely(ret))
+	if (unlikely(err)) {
+		ret = block_page_mkwrite_return(err);
 		goto out;
+	}
 
 	file_update_time(vma->vm_file);
-	ret = block_page_mkwrite(vma, vmf, nilfs_get_block);
+	ret = block_page_mkwrite(vma, vmf, nilfs_get_block, &err);
 	if (ret) {
 		nilfs_transaction_abort(inode->i_sb);
 		goto out;
@@ -117,7 +120,7 @@ static int nilfs_page_mkwrite(struct vm_fault *vmf)
 	wait_for_stable_page(page);
  out:
 	sb_end_pagefault(inode->i_sb);
-	return block_page_mkwrite_return(ret);
+	return ret;
 }
 
 static const struct vm_operations_struct nilfs_file_vm_ops = {
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 96225a7..e212b13 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
 
@@ -239,10 +240,10 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
 			get_block_t *, loff_t *);
 int generic_cont_expand_simple(struct inode *inode, loff_t size);
 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);
+vm_fault_t block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+				get_block_t get_block, int *err);
 /* 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 17:25 [PATCH v2] fs: Convert return type int to vm_fault_t Souptick Joarder
@ 2018-08-30 23:33 ` Andrew Morton
  2018-08-31  6:03   ` Souptick Joarder
  2018-09-03  2:13   ` Ryusuke Konishi
  2018-08-30 23:35 ` Andrew Morton
  2018-09-03  4:49 ` Matthew Wilcox
  2 siblings, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2018-08-30 23:33 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: konishi.ryusuke, viro, tytso, adilger.kernel, axboe,
	darrick.wong, ebiggers, pombredanne, agruenba, gregkh, kemi.wang,
	willy, linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Thu, 30 Aug 2018 22:55:47 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> Return type for fault handlers  in ext4 and nilfs are
> changed to use vm_fault_t.
> 
> Return type of block_page_mkwrite() is changed from
> int to vm_fault_t. The function signature of
> block_page_mkwrite() is changed to add one new parameter
> int *err. This will provide a way for caller functions
> to get error value along with return value and use it
> further.
> 
> Return type of block_page_mkwrite_return() is also changed
> to use new vm_fault_t type.
> --- a/fs/nilfs2/file.c
> +++ b/fs/nilfs2/file.c
> @@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  	return err;
>  }
>  
> -static int nilfs_page_mkwrite(struct vm_fault *vmf)
> +static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)

nilfs_page_mkwrite() already has return type vm_fault_t in Linus's
kernel, due to the now-merged
fs-nilfs2-adding-new-return-type-vm_fault_t.patch.  Looks like a simple
fix.

I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
benefit we're going to get out of all this churn?

> 
> ...
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 17:25 [PATCH v2] fs: Convert return type int to vm_fault_t Souptick Joarder
  2018-08-30 23:33 ` Andrew Morton
@ 2018-08-30 23:35 ` Andrew Morton
  2018-08-31 16:58   ` Theodore Y. Ts'o
  2018-09-03  4:49 ` Matthew Wilcox
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2018-08-30 23:35 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: konishi.ryusuke, viro, tytso, adilger.kernel, axboe,
	darrick.wong, ebiggers, pombredanne, agruenba, gregkh, kemi.wang,
	willy, linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Thu, 30 Aug 2018 22:55:47 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> Return type for fault handlers  in ext4 and nilfs are
> changed to use vm_fault_t.
> 
> Return type of block_page_mkwrite() is changed from
> int to vm_fault_t. The function signature of
> block_page_mkwrite() is changed to add one new parameter
> int *err. This will provide a way for caller functions
> to get error value along with return value and use it
> further.
> 
> Return type of block_page_mkwrite_return() is also changed
> to use new vm_fault_t type.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> ---
> v2: return type of nilfs_page_mkwrite() changed to vm_fault_t
>     Address Ryusuke's comment. remove err =0.

The v1->v2 delta (below) reveals unchangelogged ext4 changes?

--- a/fs/ext4/inode.c~fs-convert-return-type-int-to-vm_fault_t-v2
+++ a/fs/ext4/inode.c
@@ -6174,14 +6174,13 @@ vm_fault_t ext4_page_mkwrite(struct vm_f
 	if (err)
 		goto out_ret;
 
-	err = 0;
 	/* Delalloc case is easy... */
 	if (test_opt(inode->i_sb, DELALLOC) &&
 	    !ext4_should_journal_data(inode) &&
 	    !ext4_nonda_switch(inode->i_sb)) {
 		do {
 			ret = block_page_mkwrite(vma, vmf,
-					ext4_da_get_block_prep, &err);
+						ext4_da_get_block_prep, &err);
 		} while (err == -ENOSPC &&
 		       ext4_should_retry_alloc(inode->i_sb, &retries));
 		goto out;
@@ -6227,7 +6226,6 @@ retry_alloc:
 		ret = VM_FAULT_SIGBUS;
 		goto out;
 	}
-	err = 0;
 	ret = block_page_mkwrite(vma, vmf, get_block, &err);
 	if (!ret && ext4_should_journal_data(inode)) {
 		if (ext4_walk_page_buffers(handle, page_buffers(page), 0,
@@ -6239,8 +6237,7 @@ retry_alloc:
 		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 	}
 	ext4_journal_stop(handle);
-	if (err == -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(err);
--- a/fs/nilfs2/file.c~fs-convert-return-type-int-to-vm_fault_t-v2
+++ a/fs/nilfs2/file.c
@@ -107,7 +107,6 @@ static vm_fault_t nilfs_page_mkwrite(str
 		goto out;
 	}
 
-	err = 0;
 	file_update_time(vma->vm_file);
 	ret = block_page_mkwrite(vma, vmf, nilfs_get_block, &err);
 	if (ret) {
_

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 23:33 ` Andrew Morton
@ 2018-08-31  6:03   ` Souptick Joarder
  2018-09-04  0:28     ` Andrew Morton
  2018-09-03  2:13   ` Ryusuke Konishi
  1 sibling, 1 reply; 11+ messages in thread
From: Souptick Joarder @ 2018-08-31  6:03 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Ryusuke Konishi, Al Viro, Theodore Ts'o, adilger.kernel,
	Jens Axboe, Darrick J. Wong, Eric Biggers, Philippe Ombredanne,
	Andreas Gruenbacher, Greg KH, kemi.wang, Matthew Wilcox,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Fri, Aug 31, 2018 at 5:03 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 30 Aug 2018 22:55:47 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> > Return type for fault handlers  in ext4 and nilfs are
> > changed to use vm_fault_t.
> >
> > Return type of block_page_mkwrite() is changed from
> > int to vm_fault_t. The function signature of
> > block_page_mkwrite() is changed to add one new parameter
> > int *err. This will provide a way for caller functions
> > to get error value along with return value and use it
> > further.
> >
> > Return type of block_page_mkwrite_return() is also changed
> > to use new vm_fault_t type.
> > --- a/fs/nilfs2/file.c
> > +++ b/fs/nilfs2/file.c
> > @@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> >       return err;
> >  }
> >
> > -static int nilfs_page_mkwrite(struct vm_fault *vmf)
> > +static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)
>
> nilfs_page_mkwrite() already has return type vm_fault_t in Linus's
> kernel, due to the now-merged
> fs-nilfs2-adding-new-return-type-vm_fault_t.patch.  Looks like a simple
> fix.
>
> I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
> benefit we're going to get out of all this churn?

The problem and benefit of these changes was discussed under this mail
thread when the first vm_fault_t patch was posted.

https://marc.info/?l=linux-mm&m=152054772413234&w=4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 23:35 ` Andrew Morton
@ 2018-08-31 16:58   ` Theodore Y. Ts'o
  2018-09-01 16:21     ` Souptick Joarder
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Y. Ts'o @ 2018-08-31 16:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Souptick Joarder, konishi.ryusuke, viro, adilger.kernel, axboe,
	darrick.wong, ebiggers, pombredanne, agruenba, gregkh, kemi.wang,
	willy, linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Thu, Aug 30, 2018 at 04:35:21PM -0700, Andrew Morton wrote:
> 
> The v1->v2 delta (below) reveals unchangelogged ext4 changes?

Souptick, please don't make unrelated changes in a vm_fault_t patch.

Especially please don't make whitespace changes that will cause
checkpatch.pl to whine about line lengths longer than 80 characters.
There's a *reason* for the original indentation.

> @@ -6239,8 +6237,7 @@ retry_alloc:
>  		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
>  	}
>  	ext4_journal_stop(handle);
> -	if (err == -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(err);

The fact that this is not a non-trivials set of changes means anything
to make reviews harder is really not appreciated.


Also, the fact that the patch series involves multiple file system is
a massive pain.  It means I'm going to have to do a separate
regression test --- or preferably, I would ask *you* to run a file
system regression test[1] --- to make sure what is *not* a trivial
patch doesn't break things.  Also, it means that this patch series is
going to get more complicated to get into kernel, and we may have to
deal with patch conflicts if this goes in via some third party tree
(such as Andrew's tree).

[1] https:/thunk.org/gce-xfstests

One way to make life easier is to add the new function with the new
interface first, and then wait a release cycle, and then move file
systems over in independant patches.  

					- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-31 16:58   ` Theodore Y. Ts'o
@ 2018-09-01 16:21     ` Souptick Joarder
  0 siblings, 0 replies; 11+ messages in thread
From: Souptick Joarder @ 2018-09-01 16:21 UTC (permalink / raw)
  To: Theodore Ts'o, Andrew Morton, Ryusuke Konishi, Al Viro,
	adilger.kernel, Jens Axboe, Darrick J. Wong, Eric Biggers,
	Philippe Ombredanne, Andreas Gruenbacher, Greg KH, kemi.wang,
	Matthew Wilcox, linux-fsdevel, linux-kernel, linux-ext4,
	linux-nilfs

> Also, the fact that the patch series involves multiple file system is
> a massive pain.  It means I'm going to have to do a separate
> regression test --- or preferably, I would ask *you* to run a file
> system regression test[1] --- to make sure what is *not* a trivial
> patch doesn't break things.  Also, it means that this patch series is
> going to get more complicated to get into kernel, and we may have to
> deal with patch conflicts if this goes in via some third party tree
> (such as Andrew's tree).
>
> [1] https:/thunk.org/gce-xfstests

Sure, I will run the regression.

>
> One way to make life easier is to add the new function with the new
> interface first, and then wait a release cycle, and then move file
> systems over in independant patches.

In last review, you left it to me either to add new function or modify the
input parameters of block_page_mkwrite() to return err to caller.
As block_page_mkwrite() is getting called from 2 places in ext4 & nilfs,
I choose to add new input argument in block_page_mkwrite() rather than
introducing new function and put everything in a single commit.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 23:33 ` Andrew Morton
  2018-08-31  6:03   ` Souptick Joarder
@ 2018-09-03  2:13   ` Ryusuke Konishi
  2018-09-03 11:58     ` Souptick Joarder
  1 sibling, 1 reply; 11+ messages in thread
From: Ryusuke Konishi @ 2018-09-03  2:13 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: Andrew Morton, viro, tytso, adilger.kernel, axboe, darrick.wong,
	ebiggers, pombredanne, agruenba, gregkh, kemi.wang, willy,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

Hi Souptick,

On Thu, 30 Aug 2018 16:33:52 -0700, Andrew Morton wrote:
> On Thu, 30 Aug 2018 22:55:47 +0530 Souptick Joarder wrote:
> 
>> Return type for fault handlers  in ext4 and nilfs are
>> changed to use vm_fault_t.
>> 
>> Return type of block_page_mkwrite() is changed from
>> int to vm_fault_t. The function signature of
>> block_page_mkwrite() is changed to add one new parameter
>> int *err. This will provide a way for caller functions
>> to get error value along with return value and use it
>> further.
>> 
>> Return type of block_page_mkwrite_return() is also changed
>> to use new vm_fault_t type.
>> --- a/fs/nilfs2/file.c
>> +++ b/fs/nilfs2/file.c
>> @@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>  	return err;
>>  }
>>  
>> -static int nilfs_page_mkwrite(struct vm_fault *vmf)
>> +static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)
> 
> nilfs_page_mkwrite() already has return type vm_fault_t in Linus's
> kernel, due to the now-merged
> fs-nilfs2-adding-new-return-type-vm_fault_t.patch.  Looks like a simple
> fix.

In the first patch in this thread, this return type change was
excluded correctly for nilfs_page_mkwrite() though the changelog was
inaccurate in that sense.

Please confirm your base point of the revised patch.

Regards,
Ryusuke Konishi


> 
> I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
> benefit we're going to get out of all this churn?
> 
>> 
>> ...
>>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-30 17:25 [PATCH v2] fs: Convert return type int to vm_fault_t Souptick Joarder
  2018-08-30 23:33 ` Andrew Morton
  2018-08-30 23:35 ` Andrew Morton
@ 2018-09-03  4:49 ` Matthew Wilcox
  2 siblings, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2018-09-03  4:49 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: akpm, konishi.ryusuke, viro, tytso, adilger.kernel, axboe,
	darrick.wong, ebiggers, pombredanne, agruenba, gregkh, kemi.wang,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Thu, Aug 30, 2018 at 10:55:47PM +0530, Souptick Joarder wrote:
> +vm_fault_t block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> +			 get_block_t get_block, int *err)

I don't like returning both the errno and the vm_fault_t.  To me that's a
sign we need to rethink this interface.

I have two suggestions.  First, we could allocate a new VM_FAULT_NOSPC
bit.  Second, we could repurpose one of the existing bits, such as
VM_FAULT_RETRY for this purpose.

> -int ext4_page_mkwrite(struct vm_fault *vmf)
> +vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)

I also think perhaps we could start by _not_ converting block_page_mkwrite().
Just convert ext4_page_mkwrite(), and save converting block_page_mkwrite()
for later.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-09-03  2:13   ` Ryusuke Konishi
@ 2018-09-03 11:58     ` Souptick Joarder
  0 siblings, 0 replies; 11+ messages in thread
From: Souptick Joarder @ 2018-09-03 11:58 UTC (permalink / raw)
  To: Ryusuke Konishi
  Cc: Andrew Morton, Al Viro, Theodore Ts'o, adilger.kernel,
	Jens Axboe, Darrick J. Wong, Eric Biggers, Philippe Ombredanne,
	Andreas Gruenbacher, Greg KH, kemi.wang, Matthew Wilcox,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Mon, Sep 3, 2018 at 7:43 AM Ryusuke Konishi
<konishi.ryusuke@lab.ntt.co.jp> wrote:
>
> Hi Souptick,
>
> On Thu, 30 Aug 2018 16:33:52 -0700, Andrew Morton wrote:
> > On Thu, 30 Aug 2018 22:55:47 +0530 Souptick Joarder wrote:
> >
> >> Return type for fault handlers  in ext4 and nilfs are
> >> changed to use vm_fault_t.
> >>
> >> Return type of block_page_mkwrite() is changed from
> >> int to vm_fault_t. The function signature of
> >> block_page_mkwrite() is changed to add one new parameter
> >> int *err. This will provide a way for caller functions
> >> to get error value along with return value and use it
> >> further.
> >>
> >> Return type of block_page_mkwrite_return() is also changed
> >> to use new vm_fault_t type.
> >> --- a/fs/nilfs2/file.c
> >> +++ b/fs/nilfs2/file.c
> >> @@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> >>      return err;
> >>  }
> >>
> >> -static int nilfs_page_mkwrite(struct vm_fault *vmf)
> >> +static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)
> >
> > nilfs_page_mkwrite() already has return type vm_fault_t in Linus's
> > kernel, due to the now-merged
> > fs-nilfs2-adding-new-return-type-vm_fault_t.patch.  Looks like a simple
> > fix.
>
> In the first patch in this thread, this return type change was
> excluded correctly for nilfs_page_mkwrite() though the changelog was
> inaccurate in that sense.
>
> Please confirm your base point of the revised patch.
>

Ryusuku,
Base point of revised patch is to address your comment by
removing err = 0. Nothing else was suppose to there in v2.

But unfortunately I mixed up my local branches and created v2 against
old branch which leads to confusion of v1 -> v2 delta changes.
Sorry for creating more confusions.

>
> >
> > I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
> > benefit we're going to get out of all this churn?
> >
> >>
> >> ...
> >>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-08-31  6:03   ` Souptick Joarder
@ 2018-09-04  0:28     ` Andrew Morton
  2018-09-04  2:13       ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2018-09-04  0:28 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: Ryusuke Konishi, Al Viro, Theodore Ts'o, adilger.kernel,
	Jens Axboe, Darrick J. Wong, Eric Biggers, Philippe Ombredanne,
	Andreas Gruenbacher, Greg KH, kemi.wang, Matthew Wilcox,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Fri, 31 Aug 2018 11:33:48 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote:

> > > Return type of block_page_mkwrite_return() is also changed
> > > to use new vm_fault_t type.
> > > --- a/fs/nilfs2/file.c
> > > +++ b/fs/nilfs2/file.c
> > > @@ -51,13 +51,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > >       return err;
> > >  }
> > >
> > > -static int nilfs_page_mkwrite(struct vm_fault *vmf)
> > > +static vm_fault_t nilfs_page_mkwrite(struct vm_fault *vmf)
> >
> > nilfs_page_mkwrite() already has return type vm_fault_t in Linus's
> > kernel, due to the now-merged
> > fs-nilfs2-adding-new-return-type-vm_fault_t.patch.  Looks like a simple
> > fix.
> >
> > I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
> > benefit we're going to get out of all this churn?
> 
> The problem and benefit of these changes was discussed under this mail
> thread when the first vm_fault_t patch was posted.
> 
> https://marc.info/?l=linux-mm&m=152054772413234&w=4

That tells us about the merging plans.  But not much about what actual
benefit anyone gets from this.

This?

: There's some interesting patterns and commonalities between drivers
: (not to mention a few outright bugs) that we've noticed, and this'll be
: a good time to clean them up.

That is terribly vague.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: Convert return type int to vm_fault_t
  2018-09-04  0:28     ` Andrew Morton
@ 2018-09-04  2:13       ` Matthew Wilcox
  0 siblings, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2018-09-04  2:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Souptick Joarder, Ryusuke Konishi, Al Viro, Theodore Ts'o,
	adilger.kernel, Jens Axboe, Darrick J. Wong, Eric Biggers,
	Philippe Ombredanne, Andreas Gruenbacher, Greg KH, kemi.wang,
	linux-fsdevel, linux-kernel, linux-ext4, linux-nilfs

On Mon, Sep 03, 2018 at 05:28:43PM -0700, Andrew Morton wrote:
> > > I'm beginning to feel vm_fault_t exhaustion.  Please remind me what
> > > benefit we're going to get out of all this churn?

Hi Andrew,

The primary benefit is to help driver writers.  At the moment, there
is nothing to stop them returning -ENOMEM instead of VM_FAULT_NOMEM.
There were one or two examples of this in the tree, but I think they're
all gone now.

Secondarily, there are a number of places which translate between error
codes and vm_fault codes.  Those places are reduced as a result of these
patches, if not entirely eliminated yet.  There was some pretty extreme
cargo-culting of errno to vm_fault switch statements, particularly in
the DRM drivers.

There were also several places which were just ignoring the return value
of vm_insert_foo(), and as a result of this audit, those have been fixed.
Those errors are going to be rare, but can cause inappropriate decisions
to be made by the OOM killer.

Now that I think about it, vmf_insert_foo() should probably get marked
with __must_check to prevent those kinds of errors from being introduced
again.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-09-04  6:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-30 17:25 [PATCH v2] fs: Convert return type int to vm_fault_t Souptick Joarder
2018-08-30 23:33 ` Andrew Morton
2018-08-31  6:03   ` Souptick Joarder
2018-09-04  0:28     ` Andrew Morton
2018-09-04  2:13       ` Matthew Wilcox
2018-09-03  2:13   ` Ryusuke Konishi
2018-09-03 11:58     ` Souptick Joarder
2018-08-30 23:35 ` Andrew Morton
2018-08-31 16:58   ` Theodore Y. Ts'o
2018-09-01 16:21     ` Souptick Joarder
2018-09-03  4:49 ` Matthew Wilcox

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