[PATCH v2 0/5] clean up block_commit_write

[PATCH v2 0/5] clean up block_commit_write

[Bean Huo]

Changelog:

    v1--v2:
        1. Re-order patches to avoid breaking compilation.

Bean Huo (5):
  fs/buffer: clean up block_commit_write
  ext4: No need to check return value of block_commit_write()
  fs/ocfs2: No need to check return value of block_commit_write()
  udf: No need to check return value of block_commit_write()
  fs/buffer.c: convert block_commit_write to return void

 fs/buffer.c                 | 24 +++++++-----------------
 fs/ext4/move_extent.c       |  7 ++-----
 fs/ocfs2/file.c             |  7 +------
 fs/udf/file.c               |  6 +++---
 include/linux/buffer_head.h |  2 +-
 5 files changed, 14 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] fs/buffer: clean up block_commit_write

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Originally inode is used to get blksize, after commit 45bce8f3e343
("fs/buffer.c: make block-size be per-page and protected by the page lock"),
__block_commit_write no longer uses this parameter inode, this patch is to
remove inode and clean up block_commit_write.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index a7fc561758b1..b88bb7ec38be 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2116,8 +2116,7 @@ int __block_write_begin(struct page *page, loff_t pos, unsigned len,
 }
 EXPORT_SYMBOL(__block_write_begin);
 
-static int __block_commit_write(struct inode *inode, struct page *page,
-		unsigned from, unsigned to)
+int block_commit_write(struct page *page, unsigned int from, unsigned int to)
 {
 	unsigned block_start, block_end;
 	int partial = 0;
@@ -2154,6 +2153,7 @@ static int __block_commit_write(struct inode *inode, struct page *page,
 		SetPageUptodate(page);
 	return 0;
 }
+EXPORT_SYMBOL(block_commit_write);
 
 /*
  * block_write_begin takes care of the basic task of block allocation and
@@ -2188,7 +2188,6 @@ int block_write_end(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len, unsigned copied,
 			struct page *page, void *fsdata)
 {
-	struct inode *inode = mapping->host;
 	unsigned start;
 
 	start = pos & (PAGE_SIZE - 1);
@@ -2214,7 +2213,7 @@ int block_write_end(struct file *file, struct address_space *mapping,
 	flush_dcache_page(page);
 
 	/* This could be a short (even 0-length) commit */
-	__block_commit_write(inode, page, start, start+copied);
+	block_commit_write(page, start, start+copied);
 
 	return copied;
 }
@@ -2535,14 +2534,6 @@ int cont_write_begin(struct file *file, struct address_space *mapping,
 }
 EXPORT_SYMBOL(cont_write_begin);
 
-int block_commit_write(struct page *page, unsigned from, unsigned to)
-{
-	struct inode *inode = page->mapping->host;
-	__block_commit_write(inode,page,from,to);
-	return 0;
-}
-EXPORT_SYMBOL(block_commit_write);
-
 /*
  * block_page_mkwrite() is not allowed to change the file size as it gets
  * called from a page fault handler when a page is first dirtied. Hence we must
-- 
2.34.1

Re: [PATCH v2 1/5] fs/buffer: clean up block_commit_write

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/5] fs/buffer: clean up block_commit_write

[Matthew Wilcox]

On Mon, Jun 19, 2023 at 11:18:23PM +0200, Bean Huo wrote:
> +++ b/fs/buffer.c
> @@ -2116,8 +2116,7 @@ int __block_write_begin(struct page *page, loff_t pos, unsigned len,
>  }
>  EXPORT_SYMBOL(__block_write_begin);
>  
> -static int __block_commit_write(struct inode *inode, struct page *page,
> -		unsigned from, unsigned to)
> +int block_commit_write(struct page *page, unsigned int from, unsigned int to)
>  {
>  	unsigned block_start, block_end;
>  	int partial = 0;

You're going to need to redo these patches, I'm afraid.  A series of
patches I wrote just went in that convert __block_commit_write (but
not block_commit_write) to take a folio instead of a page.

Re: [PATCH v2 1/5] fs/buffer: clean up block_commit_write

[Bean Huo]

On Tue, 2023-06-20 at 06:33 +0100, Matthew Wilcox wrote:
> You're going to need to redo these patches, I'm afraid.  A series of
> patches I wrote just went in that convert __block_commit_write (but
> not block_commit_write) to take a folio instead of a page.

Do you know which git repo merged with your patches? 

Re: [PATCH v2 1/5] fs/buffer: clean up block_commit_write

[Matthew Wilcox]

On Tue, Jun 20, 2023 at 12:57:11PM +0200, Bean Huo wrote:
> On Tue, 2023-06-20 at 06:33 +0100, Matthew Wilcox wrote:
> > You're going to need to redo these patches, I'm afraid.  A series of
> > patches I wrote just went in that convert __block_commit_write (but
> > not block_commit_write) to take a folio instead of a page.
> 
> Do you know which git repo merged with your patches? 

They're in git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
It's usually easiest to just work against
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next

[PATCH v2 2/5] ext4: No need to check return value of block_commit_write()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Remove unnecessary check on the return value of block_commit_write(),
because it always returns 0.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/move_extent.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index b5af2fc03b2f..f4b4861a74ee 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -392,14 +392,11 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
 	for (i = 0; i < block_len_in_page; i++) {
 		*err = ext4_get_block(orig_inode, orig_blk_offset + i, bh, 0);
 		if (*err < 0)
-			break;
+			goto repair_branches;
 		bh = bh->b_this_page;
 	}
-	if (!*err)
-		*err = block_commit_write(&folio[0]->page, from, from + replaced_size);
 
-	if (unlikely(*err < 0))
-		goto repair_branches;
+	block_commit_write(&folio[0]->page, from, from + replaced_size);
 
 	/* Even in case of data=writeback it is reasonable to pin
 	 * inode to transaction, to prevent unexpected data loss */
-- 
2.34.1

Re: [PATCH v2 2/5] ext4: No need to check return value of block_commit_write()

[Theodore Ts'o]

On Mon, Jun 19, 2023 at 11:18:24PM +0200, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Remove unnecessary check on the return value of block_commit_write(),
> because it always returns 0.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Acked-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH v2 2/5] ext4: No need to check return value of block_commit_write()

[Christoph Hellwig]

On Mon, Jun 19, 2023 at 11:18:24PM +0200, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Remove unnecessary check on the return value of block_commit_write(),
> because it always returns 0.

Dropping the error check before the function signature is changes is
really odd.  I'd suggest to merge this and the following patches into
a single one.

Re: [PATCH v2 2/5] ext4: No need to check return value of block_commit_write()

[Bean Huo]

On Mon, 2023-06-19 at 21:48 -0700, Christoph Hellwig wrote:
> On Mon, Jun 19, 2023 at 11:18:24PM +0200, Bean Huo wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Remove unnecessary check on the return value of
> > block_commit_write(),
> > because it always returns 0.
> 
> Dropping the error check before the function signature is changes is
> really odd.  I'd suggest to merge this and the following patches into
> a single one.

No problem, I will merge them together, thanks.

Kind regards,
Bean

[PATCH v2 3/5] fs/ocfs2: No need to check return value of block_commit_write()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Remove unnecessary check on the return value of block_commit_write(),
because it always returns 0.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/file.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index efb09de4343d..39d8dbb26bb3 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -808,12 +808,7 @@ static int ocfs2_write_zero_page(struct inode *inode, u64 abs_from,
 
 
 		/* must not update i_size! */
-		ret = block_commit_write(page, block_start + 1,
-					 block_start + 1);
-		if (ret < 0)
-			mlog_errno(ret);
-		else
-			ret = 0;
+		block_commit_write(page, block_start + 1, block_start + 1);
 	}
 
 	/*
-- 
2.34.1

[PATCH v2 4/5] udf: No need to check return value of block_commit_write()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Remove unnecessary check on the return value of block_commit_write(),
because it always returns 0.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 8238f742377b..b1a062922a24 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -67,13 +67,13 @@ static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
 	else
 		end = PAGE_SIZE;
 	err = __block_write_begin(page, 0, end, udf_get_block);
-	if (!err)
-		err = block_commit_write(page, 0, end);
-	if (err < 0) {
+	if (err) {
 		unlock_page(page);
 		ret = block_page_mkwrite_return(err);
 		goto out_unlock;
 	}
+
+	block_commit_write(page, 0, end);
 out_dirty:
 	set_page_dirty(page);
 	wait_for_stable_page(page);
-- 
2.34.1

[PATCH v2 5/5] fs/buffer.c: convert block_commit_write to return void

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

block_commit_write() always returns 0, this patch changes it to
return void.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c                 | 11 +++++------
 include/linux/buffer_head.h |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index b88bb7ec38be..fa09cf94f771 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2116,7 +2116,7 @@ int __block_write_begin(struct page *page, loff_t pos, unsigned len,
 }
 EXPORT_SYMBOL(__block_write_begin);
 
-int block_commit_write(struct page *page, unsigned int from, unsigned int to)
+void block_commit_write(struct page *page, unsigned int from, unsigned int to)
 {
 	unsigned block_start, block_end;
 	int partial = 0;
@@ -2151,7 +2151,6 @@ int block_commit_write(struct page *page, unsigned int from, unsigned int to)
 	 */
 	if (!partial)
 		SetPageUptodate(page);
-	return 0;
 }
 EXPORT_SYMBOL(block_commit_write);
 
@@ -2577,11 +2576,11 @@ int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 		end = PAGE_SIZE;
 
 	ret = __block_write_begin(page, 0, end, get_block);
-	if (!ret)
-		ret = block_commit_write(page, 0, end);
-
-	if (unlikely(ret < 0))
+	if (unlikely(ret))
 		goto out_unlock;
+
+	block_commit_write(page, 0, end);
+
 	set_page_dirty(page);
 	wait_for_stable_page(page);
 	return 0;
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 1520793c72da..873653d2f1aa 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -284,7 +284,7 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
 			unsigned, struct page **, void **,
 			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);
+void block_commit_write(struct page *page, unsigned int from, unsigned int 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 */
-- 
2.34.1