start removing writepage instances

start removing writepage instances

[Christoph Hellwig]

Hi all,

The VM doesn't need or want ->writepage for writeback and is fine with
just having ->writepages as long as ->migrate_folio is implemented.

This series removes all ->writepage instances that use
block_write_full_page directly and also have a plain mpage_writepages
based ->writepages.

Diffstat:
 fs/exfat/inode.c   |    9 ++-------
 fs/ext2/inode.c    |    6 ------
 fs/fat/inode.c     |    9 ++-------
 fs/hfs/inode.c     |    2 +-
 fs/hfsplus/inode.c |    2 +-
 fs/hpfs/file.c     |    9 ++-------
 fs/jfs/inode.c     |    7 +------
 fs/omfs/file.c     |    7 +------
 fs/udf/inode.c     |    7 +------
 9 files changed, 11 insertions(+), 47 deletions(-)

[PATCH 1/9] extfat: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/exfat/inode.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 5590a1e83126c..eac95bcd9a8aa 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -345,11 +345,6 @@ static void exfat_readahead(struct readahead_control *rac)
 	mpage_readahead(rac, exfat_get_block);
 }
 
-static int exfat_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, exfat_get_block, wbc);
-}
-
 static int exfat_writepages(struct address_space *mapping,
 		struct writeback_control *wbc)
 {
@@ -473,12 +468,12 @@ static const struct address_space_operations exfat_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= exfat_read_folio,
 	.readahead	= exfat_readahead,
-	.writepage	= exfat_writepage,
 	.writepages	= exfat_writepages,
 	.write_begin	= exfat_write_begin,
 	.write_end	= exfat_write_end,
 	.direct_IO	= exfat_direct_IO,
-	.bmap		= exfat_aop_bmap
+	.bmap		= exfat_aop_bmap,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 static inline unsigned long exfat_hash(loff_t i_pos)
-- 
2.30.2

[PATCH 2/9] ext2: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ext2/inode.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 918ab2f9e4c05..3b2e3e1e0fa25 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -869,11 +869,6 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	return ret;
 }
 
-static int ext2_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, ext2_get_block, wbc);
-}
-
 static int ext2_read_folio(struct file *file, struct folio *folio)
 {
 	return mpage_read_folio(folio, ext2_get_block);
@@ -948,7 +943,6 @@ const struct address_space_operations ext2_aops = {
 	.invalidate_folio	= block_invalidate_folio,
 	.read_folio		= ext2_read_folio,
 	.readahead		= ext2_readahead,
-	.writepage		= ext2_writepage,
 	.write_begin		= ext2_write_begin,
 	.write_end		= ext2_write_end,
 	.bmap			= ext2_bmap,
-- 
2.30.2

[PATCH 3/9] fat: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/fat/inode.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 1cbcc4608dc78..d99b8549ec8f9 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -194,11 +194,6 @@ static int fat_get_block(struct inode *inode, sector_t iblock,
 	return 0;
 }
 
-static int fat_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, fat_get_block, wbc);
-}
-
 static int fat_writepages(struct address_space *mapping,
 			  struct writeback_control *wbc)
 {
@@ -346,12 +341,12 @@ static const struct address_space_operations fat_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= fat_read_folio,
 	.readahead	= fat_readahead,
-	.writepage	= fat_writepage,
 	.writepages	= fat_writepages,
 	.write_begin	= fat_write_begin,
 	.write_end	= fat_write_end,
 	.direct_IO	= fat_direct_IO,
-	.bmap		= _fat_bmap
+	.bmap		= _fat_bmap,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 /*
-- 
2.30.2

[PATCH 4/9] hfs: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and stop
wiring up ->writepage for hfs_aops.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/hfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index c4526f16355d5..16466a5e88b44 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -173,12 +173,12 @@ const struct address_space_operations hfs_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= hfs_read_folio,
-	.writepage	= hfs_writepage,
 	.write_begin	= hfs_write_begin,
 	.write_end	= generic_write_end,
 	.bmap		= hfs_bmap,
 	.direct_IO	= hfs_direct_IO,
 	.writepages	= hfs_writepages,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 /*
-- 
2.30.2

[PATCH 5/9] hfsplus: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and stop
wiring up ->writepage for hfsplus_aops.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/hfsplus/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index aeab83ed1c9c6..d6572ad2407a7 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -170,12 +170,12 @@ const struct address_space_operations hfsplus_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= hfsplus_read_folio,
-	.writepage	= hfsplus_writepage,
 	.write_begin	= hfsplus_write_begin,
 	.write_end	= generic_write_end,
 	.bmap		= hfsplus_bmap,
 	.direct_IO	= hfsplus_direct_IO,
 	.writepages	= hfsplus_writepages,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 const struct dentry_operations hfsplus_dentry_operations = {
-- 
2.30.2

[PATCH 6/9] hpfs: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/hpfs/file.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
index f7547a62c81f6..88952d4a631e6 100644
--- a/fs/hpfs/file.c
+++ b/fs/hpfs/file.c
@@ -163,11 +163,6 @@ static int hpfs_read_folio(struct file *file, struct folio *folio)
 	return mpage_read_folio(folio, hpfs_get_block);
 }
 
-static int hpfs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, hpfs_get_block, wbc);
-}
-
 static void hpfs_readahead(struct readahead_control *rac)
 {
 	mpage_readahead(rac, hpfs_get_block);
@@ -248,12 +243,12 @@ const struct address_space_operations hpfs_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio = hpfs_read_folio,
-	.writepage = hpfs_writepage,
 	.readahead = hpfs_readahead,
 	.writepages = hpfs_writepages,
 	.write_begin = hpfs_write_begin,
 	.write_end = hpfs_write_end,
-	.bmap = _hpfs_bmap
+	.bmap = _hpfs_bmap,
+	.migrate_folio = buffer_migrate_folio,
 };
 
 const struct file_operations hpfs_file_ops =
-- 
2.30.2

[PATCH 7/9] jfs: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/jfs/inode.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index d1ec920aa030a..8ac10e3960508 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -264,11 +264,6 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
 	return rc;
 }
 
-static int jfs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, jfs_get_block, wbc);
-}
-
 static int jfs_writepages(struct address_space *mapping,
 			struct writeback_control *wbc)
 {
@@ -355,12 +350,12 @@ const struct address_space_operations jfs_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= jfs_read_folio,
 	.readahead	= jfs_readahead,
-	.writepage	= jfs_writepage,
 	.writepages	= jfs_writepages,
 	.write_begin	= jfs_write_begin,
 	.write_end	= jfs_write_end,
 	.bmap		= jfs_bmap,
 	.direct_IO	= jfs_direct_IO,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 /*
-- 
2.30.2

[PATCH 8/9] omfs: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/omfs/file.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/omfs/file.c b/fs/omfs/file.c
index fa7fe2393ff68..3a5b4b88a5838 100644
--- a/fs/omfs/file.c
+++ b/fs/omfs/file.c
@@ -294,11 +294,6 @@ static void omfs_readahead(struct readahead_control *rac)
 	mpage_readahead(rac, omfs_get_block);
 }
 
-static int omfs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, omfs_get_block, wbc);
-}
-
 static int
 omfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 {
@@ -375,10 +370,10 @@ const struct address_space_operations omfs_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio = omfs_read_folio,
 	.readahead = omfs_readahead,
-	.writepage = omfs_writepage,
 	.writepages = omfs_writepages,
 	.write_begin = omfs_write_begin,
 	.write_end = generic_write_end,
 	.bmap = omfs_bmap,
+	.migrate_folio = buffer_migrate_folio,
 };
 
-- 
2.30.2

[PATCH 9/9] udf: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.

Set ->migrate_folio to the generic buffer_head based helper, and remove
the ->writepage implementation in extfat.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/udf/inode.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index dce6ae9ae306c..0246b1b86fb91 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -182,11 +182,6 @@ static void udf_write_failed(struct address_space *mapping, loff_t to)
 	}
 }
 
-static int udf_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page, udf_get_block, wbc);
-}
-
 static int udf_writepages(struct address_space *mapping,
 			struct writeback_control *wbc)
 {
@@ -239,12 +234,12 @@ const struct address_space_operations udf_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= udf_read_folio,
 	.readahead	= udf_readahead,
-	.writepage	= udf_writepage,
 	.writepages	= udf_writepages,
 	.write_begin	= udf_write_begin,
 	.write_end	= generic_write_end,
 	.direct_IO	= udf_direct_IO,
 	.bmap		= udf_bmap,
+	.migrate_folio	= buffer_migrate_folio,
 };
 
 /*
-- 
2.30.2

Re: [PATCH 2/9] ext2: remove ->writepage

[Jan Kara]

On Sun 13-11-22 17:28:55, Christoph Hellwig wrote:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good! Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/ext2/inode.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 918ab2f9e4c05..3b2e3e1e0fa25 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -869,11 +869,6 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  	return ret;
>  }
>  
> -static int ext2_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -	return block_write_full_page(page, ext2_get_block, wbc);
> -}
> -
>  static int ext2_read_folio(struct file *file, struct folio *folio)
>  {
>  	return mpage_read_folio(folio, ext2_get_block);
> @@ -948,7 +943,6 @@ const struct address_space_operations ext2_aops = {
>  	.invalidate_folio	= block_invalidate_folio,
>  	.read_folio		= ext2_read_folio,
>  	.readahead		= ext2_readahead,
> -	.writepage		= ext2_writepage,
>  	.write_begin		= ext2_write_begin,
>  	.write_end		= ext2_write_end,
>  	.bmap			= ext2_bmap,
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 9/9] udf: remove ->writepage

[Jan Kara]

On Sun 13-11-22 17:29:02, Christoph Hellwig wrote:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
> 
> Set ->migrate_folio to the generic buffer_head based helper, and remove
> the ->writepage implementation in extfat.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/udf/inode.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index dce6ae9ae306c..0246b1b86fb91 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -182,11 +182,6 @@ static void udf_write_failed(struct address_space *mapping, loff_t to)
>  	}
>  }
>  
> -static int udf_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -	return block_write_full_page(page, udf_get_block, wbc);
> -}
> -
>  static int udf_writepages(struct address_space *mapping,
>  			struct writeback_control *wbc)
>  {
> @@ -239,12 +234,12 @@ const struct address_space_operations udf_aops = {
>  	.invalidate_folio = block_invalidate_folio,
>  	.read_folio	= udf_read_folio,
>  	.readahead	= udf_readahead,
> -	.writepage	= udf_writepage,
>  	.writepages	= udf_writepages,
>  	.write_begin	= udf_write_begin,
>  	.write_end	= generic_write_end,
>  	.direct_IO	= udf_direct_IO,
>  	.bmap		= udf_bmap,
> +	.migrate_folio	= buffer_migrate_folio,
>  };
>  
>  /*
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/9] extfat: remove ->writepage

[Namjae Jeon]

2022-11-14 1:28 GMT+09:00, Christoph Hellwig <hch@lst.de>:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
>
> Set ->migrate_folio to the generic buffer_head based helper, and remove
> the ->writepage implementation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!

Re: [PATCH 7/9] jfs: remove ->writepage

[Dave Kleikamp]

On 11/13/22 10:29AM, Christoph Hellwig wrote:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
> 
> Set ->migrate_folio to the generic buffer_head based helper, and remove
> the ->writepage implementation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Dave Kleikamp <dave.kleikamp@oracle.com>

> ---
>   fs/jfs/inode.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
> index d1ec920aa030a..8ac10e3960508 100644
> --- a/fs/jfs/inode.c
> +++ b/fs/jfs/inode.c
> @@ -264,11 +264,6 @@ int jfs_get_block(struct inode *ip, sector_t lblock,
>   	return rc;
>   }
>   
> -static int jfs_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -	return block_write_full_page(page, jfs_get_block, wbc);
> -}
> -
>   static int jfs_writepages(struct address_space *mapping,
>   			struct writeback_control *wbc)
>   {
> @@ -355,12 +350,12 @@ const struct address_space_operations jfs_aops = {
>   	.invalidate_folio = block_invalidate_folio,
>   	.read_folio	= jfs_read_folio,
>   	.readahead	= jfs_readahead,
> -	.writepage	= jfs_writepage,
>   	.writepages	= jfs_writepages,
>   	.write_begin	= jfs_write_begin,
>   	.write_end	= jfs_write_end,
>   	.bmap		= jfs_bmap,
>   	.direct_IO	= jfs_direct_IO,
> +	.migrate_folio	= buffer_migrate_folio,
>   };
>   
>   /*

Re: start removing writepage instances

[Johannes Weiner]

On Sun, Nov 13, 2022 at 05:28:53PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> The VM doesn't need or want ->writepage for writeback and is fine with
> just having ->writepages as long as ->migrate_folio is implemented.
> 
> This series removes all ->writepage instances that use
> block_write_full_page directly and also have a plain mpage_writepages
> based ->writepages.

The series looks good from the MM side.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH 8/9] omfs: remove ->writepage

[Bob Copeland]

On Sun, Nov 13, 2022 at 05:29:01PM +0100, Christoph Hellwig wrote:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
> 
> Set ->migrate_folio to the generic buffer_head based helper, and remove
> the ->writepage implementation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,

Acked-by: Bob Copeland <me@bobcopeland.com>

-- 
Bob Copeland %% https://bobcopeland.com/

Re: [PATCH 2/9] ext2: remove ->writepage

[Christoph Hellwig]

On Mon, Nov 14, 2022 at 11:49:27AM +0100, Jan Kara wrote:
> On Sun 13-11-22 17:28:55, Christoph Hellwig wrote:
> > ->writepage is a very inefficient method to write back data, and only
> > used through write_cache_pages or a a fallback when no ->migrate_folio
> > method is present.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Looks good! Feel free to add:

The testbot found a problem with this:

ext2_commit_chunk calls write_one_page for the IS_DIRSYNC case,
and write_one_page calls ->writepage.

So I think I need to drop this one for now (none of the other
file systems calls write_one_page).  And then think what best
to do about write_one_page/write_one_folio.  I suspect just
passing a writepage pointer to them might make most sense,
as they are only used by a few file systems, and the calling
convention with the locked page doesn't lend itself to using
->writepages.

Re: [PATCH 2/9] ext2: remove ->writepageo

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Wed 16-11-22 08:14:15, Christoph Hellwig wrote:
> On Mon, Nov 14, 2022 at 11:49:27AM +0100, Jan Kara wrote:
> > On Sun 13-11-22 17:28:55, Christoph Hellwig wrote:
> > > ->writepage is a very inefficient method to write back data, and only
> > > used through write_cache_pages or a a fallback when no ->migrate_folio
> > > method is present.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Looks good! Feel free to add:
> 
> The testbot found a problem with this:
> 
> ext2_commit_chunk calls write_one_page for the IS_DIRSYNC case,
> and write_one_page calls ->writepage.

Right.

> So I think I need to drop this one for now (none of the other
> file systems calls write_one_page).  And then think what best
> to do about write_one_page/write_one_folio.  I suspect just
> passing a writepage pointer to them might make most sense,
> as they are only used by a few file systems, and the calling
> convention with the locked page doesn't lend itself to using
> ->writepages.

Looking at the code, IMO the write_one_page() looks somewhat premature
anyway in that place. AFAICS we could handle the writeout using
filemap_write_and_wait() if we moved it to somewhat later moment. So
something like attached patch (only basic testing only so far)?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-ext2-Don-t-flush-page-immediately-for-DIRSYNC-direct.patch --]
[-- Type: text/x-patch, Size: 3880 bytes --]

From dc36b90da4c23134ac2fd02d7a21a736fe68d598 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 16 Nov 2022 19:08:23 +0100
Subject: [PATCH] ext2: Don't flush page immediately for DIRSYNC directories

We do not need to writeout modified directory blocks immediately when
modifying them while the page is locked. It is enough to do the flush
somewhat later which has the added benefit that inode times can be
flushed as well. It also allows us to stop depending on
write_one_page() function.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/dir.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 8f597753ac12..17efc784af53 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -81,11 +81,10 @@ ext2_last_byte(struct inode *inode, unsigned long page_nr)
 	return last_byte;
 }
 
-static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
+static void ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
 {
 	struct address_space *mapping = page->mapping;
 	struct inode *dir = mapping->host;
-	int err = 0;
 
 	inode_inc_iversion(dir);
 	block_write_end(NULL, mapping, pos, len, len, page, NULL);
@@ -94,16 +93,7 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
 		i_size_write(dir, pos+len);
 		mark_inode_dirty(dir);
 	}
-
-	if (IS_DIRSYNC(dir)) {
-		err = write_one_page(page);
-		if (!err)
-			err = sync_inode_metadata(dir, 1);
-	} else {
-		unlock_page(page);
-	}
-
-	return err;
+	unlock_page(page);
 }
 
 static bool ext2_check_page(struct page *page, int quiet, char *kaddr)
@@ -460,6 +450,17 @@ static int ext2_prepare_chunk(struct page *page, loff_t pos, unsigned len)
 	return __block_write_begin(page, pos, len, ext2_get_block);
 }
 
+
+static int ext2_handle_dirsync(struct inode *dir)
+{
+	int err;
+
+	err = filemap_write_and_wait(dir->i_mapping);
+	if (!err)
+		err = sync_inode_metadata(dir, 1);
+	return err;
+}
+
 void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 		   struct page *page, void *page_addr, struct inode *inode,
 		   int update_times)
@@ -474,11 +475,12 @@ void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 	BUG_ON(err);
 	de->inode = cpu_to_le32(inode->i_ino);
 	ext2_set_de_type(de, inode);
-	err = ext2_commit_chunk(page, pos, len);
+	ext2_commit_chunk(page, pos, len);
 	if (update_times)
 		dir->i_mtime = dir->i_ctime = current_time(dir);
 	EXT2_I(dir)->i_flags &= ~EXT2_BTREE_FL;
 	mark_inode_dirty(dir);
+	ext2_handle_dirsync(dir);
 }
 
 /*
@@ -566,10 +568,11 @@ int ext2_add_link (struct dentry *dentry, struct inode *inode)
 	memcpy(de->name, name, namelen);
 	de->inode = cpu_to_le32(inode->i_ino);
 	ext2_set_de_type (de, inode);
-	err = ext2_commit_chunk(page, pos, rec_len);
+	ext2_commit_chunk(page, pos, rec_len);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 	EXT2_I(dir)->i_flags &= ~EXT2_BTREE_FL;
 	mark_inode_dirty(dir);
+	err = ext2_handle_dirsync(dir);
 	/* OFFSET_CACHE */
 out_put:
 	ext2_put_page(page, page_addr);
@@ -615,10 +618,11 @@ int ext2_delete_entry (struct ext2_dir_entry_2 *dir, struct page *page,
 	if (pde)
 		pde->rec_len = ext2_rec_len_to_disk(to - from);
 	dir->inode = 0;
-	err = ext2_commit_chunk(page, pos, to - from);
+	ext2_commit_chunk(page, pos, to - from);
 	inode->i_ctime = inode->i_mtime = current_time(inode);
 	EXT2_I(inode)->i_flags &= ~EXT2_BTREE_FL;
 	mark_inode_dirty(inode);
+	err = ext2_handle_dirsync(dir);
 out:
 	return err;
 }
@@ -658,7 +662,8 @@ int ext2_make_empty(struct inode *inode, struct inode *parent)
 	memcpy (de->name, "..\0", 4);
 	ext2_set_de_type (de, inode);
 	kunmap_atomic(kaddr);
-	err = ext2_commit_chunk(page, 0, chunk_size);
+	ext2_commit_chunk(page, 0, chunk_size);
+	err = ext2_handle_dirsync(inode);
 fail:
 	put_page(page);
 	return err;
-- 
2.35.3

Re: start removing writepage instances

[Ritesh Harjani (IBM)]

On 22/11/13 05:28PM, Christoph Hellwig wrote:
> Hi all,
> 
> The VM doesn't need or want ->writepage for writeback and is fine with
> just having ->writepages as long as ->migrate_folio is implemented.

Ok, so here is, (what I think) is the motivation for doing this. 
Please correct me if this is incorrect... 
1. writepage is mainly called from pageout, which happens as part of the memory
   reclaim. Now IIUC from previous discussions [1][2][3], reclaims happens from
   the tail end of the LRU list which could do an I/O of a single page while 
   an ongoing writeback was in progress of multiple pages. This disrupts the I/O 
   pattern to become more random in nature, compared to, if we would have let 
   writeback/flusher do it's job of writing back dirty pages.

   Also many filesystems behave very differently within their ->writepage calls,
   e.g. ext4 doesn't actually write in ->writepage for DELAYED blocks.

2. Now the other place from where ->writepage can be called from is, writeout()
   function, which is a fallback function for migration (fallback_migrate_folio()).
   fallback_migrate_folio() is called from move_to_new_folio() if ->migrate_folio 
   is not defined for the FS.

So what you are doing here is removing the ->writepage from address_space
operations of the filesystems which implements ->writepage using
block_write_full_page() (i.e. those who uses buffer_heads). This is done for 
those FS who already have ->migrate_folio() implemented (hence no need of
->writepage).
...Now this is also a step towards reducing the callers from kernel which uses
buffer_heads.

[1]: https://lore.kernel.org/all/1310567487-15367-1-git-send-email-mgorman@suse.de/
[2]: https://lore.kernel.org/all/20181107063127.3902-2-david@fromorbit.com/
[3]: https://lore.kernel.org/all/1271117878-19274-1-git-send-email-david@fromorbit.com/

Is above a correct understanding?

> 
> This series removes all ->writepage instances that use
> block_write_full_page directly and also have a plain mpage_writepages
> based ->writepages.

Ok.


> 
> Diffstat:
>  fs/exfat/inode.c   |    9 ++-------
>  fs/ext2/inode.c    |    6 ------
>  fs/fat/inode.c     |    9 ++-------
>  fs/hfs/inode.c     |    2 +-
>  fs/hfsplus/inode.c |    2 +-
>  fs/hpfs/file.c     |    9 ++-------
>  fs/jfs/inode.c     |    7 +------
>  fs/omfs/file.c     |    7 +------
>  fs/udf/inode.c     |    7 +------
>  9 files changed, 11 insertions(+), 47 deletions(-)

Re: [PATCH 2/9] ext2: remove ->writepageo

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 07:20:40PM +0100, Jan Kara wrote:
> Looking at the code, IMO the write_one_page() looks somewhat premature
> anyway in that place. AFAICS we could handle the writeout using
> filemap_write_and_wait() if we moved it to somewhat later moment. So
> something like attached patch (only basic testing only so far)?

Yes, this looks sensible.  Do you want to queue this one and the
ext2 and udf patches from this series if the testing works fine?

The same transformation should also be done for minix, sysfs and
ufs.  And a bunch of the others are probaby similar as well.

Re: start removing writepage instances

[David Howells]

Christoph Hellwig <hch@lst.de> wrote:

> The VM doesn't need or want ->writepage for writeback and is fine with
> just having ->writepages as long as ->migrate_folio is implemented.

Yay! :-)

David

Re: start removing writepage instances

[David Howells]

Also ->writepage() is called with the page already locked, which is a problem
if you need to write out a number of surrounding pages with it.

David

Re: [PATCH 2/9] ext2: remove ->writepageo

[Jan Kara]

On Wed 16-11-22 22:31:47, Christoph Hellwig wrote:
> On Wed, Nov 16, 2022 at 07:20:40PM +0100, Jan Kara wrote:
> > Looking at the code, IMO the write_one_page() looks somewhat premature
> > anyway in that place. AFAICS we could handle the writeout using
> > filemap_write_and_wait() if we moved it to somewhat later moment. So
> > something like attached patch (only basic testing only so far)?
> 
> Yes, this looks sensible.  Do you want to queue this one and the
> ext2 and udf patches from this series if the testing works fine?

OK, I'll take udf and ext2 patches through my tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: start removing writepage instances

[Christoph Hellwig]

On Thu, Nov 17, 2022 at 12:09:00AM +0530, Ritesh Harjani (IBM) wrote:
>    reclaim. Now IIUC from previous discussions [1][2][3], reclaims happens from
>    the tail end of the LRU list which could do an I/O of a single page while 
>    an ongoing writeback was in progress of multiple pages. This disrupts the I/O 
>    pattern to become more random in nature, compared to, if we would have let 
>    writeback/flusher do it's job of writing back dirty pages.

Yes.

>    Also many filesystems behave very differently within their ->writepage calls,
>    e.g. ext4 doesn't actually write in ->writepage for DELAYED blocks.

I don't think it's many file systems.  As far as I can tell only ext4
actually is significantly different.

> 2. Now the other place from where ->writepage can be called from is, writeout()
>    function, which is a fallback function for migration (fallback_migrate_folio()).
>    fallback_migrate_folio() is called from move_to_new_folio() if ->migrate_folio 
>    is not defined for the FS.

Also there is generic_writepages and folio_write_one/write_one_page.

> Is above a correct understanding?

Yes.

Re: [PATCH 4/9] hfs: remove ->writepage

[Matthew Wilcox]

On Sun, Nov 13, 2022 at 05:28:57PM +0100, Christoph Hellwig wrote:
> ->writepage is a very inefficient method to write back data, and only
> used through write_cache_pages or a a fallback when no ->migrate_folio
> method is present.
> 
> Set ->migrate_folio to the generic buffer_head based helper, and stop
> wiring up ->writepage for hfs_aops.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Was there a reason you only did this for hfs_aops and not for
hfs_btree_aops?  It feels like anything that just calls
block_write_full_page() in the writepage handler should be converted
to just calling mpage_writepages() in the writepages handler.
I have a few of those conversions done, but obviously they're in
filesystems that are basically untestable.

Re: [PATCH 4/9] hfs: remove ->writepage

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 07:01:25PM +0000, Matthew Wilcox wrote:
> Was there a reason you only did this for hfs_aops and not for
> hfs_btree_aops?  It feels like anything that just calls
> block_write_full_page() in the writepage handler should be converted
> to just calling mpage_writepages() in the writepages handler.
> I have a few of those conversions done, but obviously they're in
> filesystems that are basically untestable.

Probably.  I remember I had a good reason to skip, and the lack of
testability might have been it.  Note that for hfsplus in particular
we should actually be able to test now that the port of the hfs
userspace has returned to distros.  I haven't actually gotten to
see what the test baseline looks like, though.