remove generic_writepages

remove generic_writepages

[Christoph Hellwig]

Hi all,

this series removes generic_writepages by open coding the current
functionality in the three remaining callers.  Besides removing some
code the main benefit is that one of the few remaining ->writepage
callers from outside the core page cache code go away.

Note that testing has been a bit limited - ntfs3 does not seem to
be supported by xfstests at all, and xfstests on ocfs2 is a complete
shit show even for the base line.

Diffstat:
 fs/jbd2/commit.c          |   25 ---------------------
 fs/jbd2/journal.c         |    1 
 fs/mpage.c                |    8 ------
 fs/ntfs3/inode.c          |   33 +++++++++++++---------------
 fs/ocfs2/journal.c        |   16 +++++++++++++
 include/linux/jbd2.h      |    2 -
 include/linux/writeback.h |    2 -
 mm/page-writeback.c       |   53 +++++++++++++---------------------------------
 8 files changed, 45 insertions(+), 95 deletions(-)

[PATCH 1/6] fs: remove an outdated comment on mpage_writepages

[Christoph Hellwig]

mpage_writepages doesn't do any of the page locking itself, so remove
and outdated comment on the locking pattern there.

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

diff --git a/fs/mpage.c b/fs/mpage.c
index 0f8ae954a57903..910cfe8a60d2e4 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -641,14 +641,6 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
  *
  * This is a library function, which implements the writepages()
  * address_space_operation.
- *
- * If a page is already under I/O, generic_writepages() skips it, even
- * if it's dirty.  This is desirable behaviour for memory-cleaning writeback,
- * but it is INCORRECT for data-integrity system calls such as fsync().  fsync()
- * and msync() need to guarantee that all the data which was dirty at the time
- * the call was made get new I/O started against them.  If wbc->sync_mode is
- * WB_SYNC_ALL then we were called for data integrity and we must wait for
- * existing IO to complete.
  */
 int
 mpage_writepages(struct address_space *mapping,
-- 
2.35.1

Re: [PATCH 1/6] fs: remove an outdated comment on mpage_writepages

[Jan Kara]

On Thu 29-12-22 06:10:26, Christoph Hellwig wrote:
> mpage_writepages doesn't do any of the page locking itself, so remove
> and outdated comment on the locking pattern there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/mpage.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 0f8ae954a57903..910cfe8a60d2e4 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -641,14 +641,6 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
>   *
>   * This is a library function, which implements the writepages()
>   * address_space_operation.
> - *
> - * If a page is already under I/O, generic_writepages() skips it, even
> - * if it's dirty.  This is desirable behaviour for memory-cleaning writeback,
> - * but it is INCORRECT for data-integrity system calls such as fsync().  fsync()
> - * and msync() need to guarantee that all the data which was dirty at the time
> - * the call was made get new I/O started against them.  If wbc->sync_mode is
> - * WB_SYNC_ALL then we were called for data integrity and we must wait for
> - * existing IO to complete.
>   */
>  int
>  mpage_writepages(struct address_space *mapping,
> -- 
> 2.35.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/6] ntfs3: stop using generic_writepages

[Christoph Hellwig]

Open code the resident inode handling in ntfs_writepages by directly
using write_cache_pages to prepare removing the ->writepage handler
in ntfs3.

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

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 20b953871574b8..b6dad2da59501b 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -852,12 +852,29 @@ static int ntfs_writepage(struct page *page, struct writeback_control *wbc)
 	return block_write_full_page(page, ntfs_get_block, wbc);
 }
 
+static int ntfs_resident_writepage(struct page *page,
+		struct writeback_control *wbc, void *data)
+{
+	struct address_space *mapping = data;
+	struct ntfs_inode *ni = ntfs_i(mapping->host);
+	int ret;
+
+	ni_lock(ni);
+	ret = attr_data_write_resident(ni, page);
+	ni_unlock(ni);
+
+	if (ret != E_NTFS_NONRESIDENT)
+		unlock_page(page);
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 static int ntfs_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
-	/* Redirect call to 'ntfs_writepage' for resident files. */
 	if (is_resident(ntfs_i(mapping->host)))
-		return generic_writepages(mapping, wbc);
+		return write_cache_pages(mapping, wbc, ntfs_resident_writepage,
+					 mapping);
 	return mpage_writepages(mapping, wbc, ntfs_get_block);
 }
 
-- 
2.35.1

[PATCH 3/6] ntfs3: 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/ntfs3/inode.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index b6dad2da59501b..6b50b6e3237876 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -832,26 +832,6 @@ int ntfs_set_size(struct inode *inode, u64 new_size)
 	return err;
 }
 
-static int ntfs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	struct address_space *mapping = page->mapping;
-	struct inode *inode = mapping->host;
-	struct ntfs_inode *ni = ntfs_i(inode);
-	int err;
-
-	if (is_resident(ni)) {
-		ni_lock(ni);
-		err = attr_data_write_resident(ni, page);
-		ni_unlock(ni);
-		if (err != E_NTFS_NONRESIDENT) {
-			unlock_page(page);
-			return err;
-		}
-	}
-
-	return block_write_full_page(page, ntfs_get_block, wbc);
-}
-
 static int ntfs_resident_writepage(struct page *page,
 		struct writeback_control *wbc, void *data)
 {
@@ -2083,13 +2063,13 @@ const struct inode_operations ntfs_link_inode_operations = {
 const struct address_space_operations ntfs_aops = {
 	.read_folio	= ntfs_read_folio,
 	.readahead	= ntfs_readahead,
-	.writepage	= ntfs_writepage,
 	.writepages	= ntfs_writepages,
 	.write_begin	= ntfs_write_begin,
 	.write_end	= ntfs_write_end,
 	.direct_IO	= ntfs_direct_IO,
 	.bmap		= ntfs_bmap,
 	.dirty_folio	= block_dirty_folio,
+	.migrate_folio	= buffer_migrate_folio,
 	.invalidate_folio = block_invalidate_folio,
 };
 
-- 
2.35.1

[PATCH 4/6] jbd2,ocfs2: move jbd2_journal_submit_inode_data_buffers to ocfs2

[Christoph Hellwig]

jbd2_journal_submit_inode_data_buffers is only used by ocfs2, so move
it there to prepare for removing generic_writepages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/jbd2/commit.c     | 25 -------------------------
 fs/jbd2/journal.c    |  1 -
 fs/ocfs2/journal.c   | 16 +++++++++++++++-
 include/linux/jbd2.h |  2 --
 4 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 4810438b7856aa..aeee6b8a6da218 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -181,31 +181,6 @@ static int journal_wait_on_commit_record(journal_t *journal,
 	return ret;
 }
 
-/*
- * write the filemap data using writepage() address_space_operations.
- * We don't do block allocation here even for delalloc. We don't
- * use writepages() because with delayed allocation we may be doing
- * block allocation in writepages().
- */
-int jbd2_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
-{
-	struct address_space *mapping = jinode->i_vfs_inode->i_mapping;
-	struct writeback_control wbc = {
-		.sync_mode =  WB_SYNC_ALL,
-		.nr_to_write = mapping->nrpages * 2,
-		.range_start = jinode->i_dirty_start,
-		.range_end = jinode->i_dirty_end,
-	};
-
-	/*
-	 * submit the inode data buffers. We use writepage
-	 * instead of writepages. Because writepages can do
-	 * block allocation with delalloc. We need to write
-	 * only allocated blocks here.
-	 */
-	return generic_writepages(mapping, &wbc);
-}
-
 /* Send all the data buffers related to an inode */
 int jbd2_submit_inode_data(journal_t *journal, struct jbd2_inode *jinode)
 {
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 2696f43e7239f8..d331f6ece20a28 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -89,7 +89,6 @@ EXPORT_SYMBOL(jbd2_journal_try_to_free_buffers);
 EXPORT_SYMBOL(jbd2_journal_force_commit);
 EXPORT_SYMBOL(jbd2_journal_inode_ranged_write);
 EXPORT_SYMBOL(jbd2_journal_inode_ranged_wait);
-EXPORT_SYMBOL(jbd2_journal_submit_inode_data_buffers);
 EXPORT_SYMBOL(jbd2_journal_finish_inode_data_buffers);
 EXPORT_SYMBOL(jbd2_journal_init_jbd_inode);
 EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 3fb98b4569a28b..59f612684c5178 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -15,6 +15,7 @@
 #include <linux/time.h>
 #include <linux/random.h>
 #include <linux/delay.h>
+#include <linux/writeback.h>
 
 #include <cluster/masklog.h>
 
@@ -841,6 +842,19 @@ int ocfs2_journal_alloc(struct ocfs2_super *osb)
 	return status;
 }
 
+static int ocfs2_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
+{
+	struct address_space *mapping = jinode->i_vfs_inode->i_mapping;
+	struct writeback_control wbc = {
+		.sync_mode =  WB_SYNC_ALL,
+		.nr_to_write = mapping->nrpages * 2,
+		.range_start = jinode->i_dirty_start,
+		.range_end = jinode->i_dirty_end,
+	};
+
+	return generic_writepages(mapping, &wbc);
+}
+
 int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
 {
 	int status = -1;
@@ -910,7 +924,7 @@ int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
 
 	journal->j_journal = j_journal;
 	journal->j_journal->j_submit_inode_data_buffers =
-		jbd2_journal_submit_inode_data_buffers;
+		ocfs2_journal_submit_inode_data_buffers;
 	journal->j_journal->j_finish_inode_data_buffers =
 		jbd2_journal_finish_inode_data_buffers;
 	journal->j_inode = inode;
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 2170e0cc279d5c..5962072a4b19e6 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1570,8 +1570,6 @@ extern int	   jbd2_journal_inode_ranged_write(handle_t *handle,
 extern int	   jbd2_journal_inode_ranged_wait(handle_t *handle,
 			struct jbd2_inode *inode, loff_t start_byte,
 			loff_t length);
-extern int	   jbd2_journal_submit_inode_data_buffers(
-			struct jbd2_inode *jinode);
 extern int	   jbd2_journal_finish_inode_data_buffers(
 			struct jbd2_inode *jinode);
 extern int	   jbd2_journal_begin_ordered_truncate(journal_t *journal,
-- 
2.35.1

[PATCH 5/6] ocfs2: use filemap_fdatawrite_wbc instead of generic_writepages

[Christoph Hellwig]

filemap_fdatawrite_wbc is a fairly thing wrapper around do_writepages,
and the big difference there is support for cgroup writeback, which
is not supported by ocfs2, and the potential to use ->writepages instead
of ->writepage, which ocfs2 does not currently implement but eventually
should.

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

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 59f612684c5178..25d8072ccfce46 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -852,7 +852,7 @@ static int ocfs2_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
 		.range_end = jinode->i_dirty_end,
 	};
 
-	return generic_writepages(mapping, &wbc);
+	return filemap_fdatawrite_wbc(mapping, &wbc);
 }
 
 int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
-- 
2.35.1

Re: [PATCH 5/6] ocfs2: use filemap_fdatawrite_wbc instead of generic_writepages

[Jan Kara]

On Thu 29-12-22 06:10:30, Christoph Hellwig wrote:
> filemap_fdatawrite_wbc is a fairly thing wrapper around do_writepages,
> and the big difference there is support for cgroup writeback, which
> is not supported by ocfs2, and the potential to use ->writepages instead
> of ->writepage, which ocfs2 does not currently implement but eventually
> should.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ocfs2/journal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index 59f612684c5178..25d8072ccfce46 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -852,7 +852,7 @@ static int ocfs2_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
>  		.range_end = jinode->i_dirty_end,
>  	};
>  
> -	return generic_writepages(mapping, &wbc);
> +	return filemap_fdatawrite_wbc(mapping, &wbc);
>  }
>  
>  int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
> -- 
> 2.35.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 6/6] mm: remove generic_writepages

[Christoph Hellwig]

Now that all external callers are gone, just fold it into do_writepages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/writeback.h |  2 --
 mm/page-writeback.c       | 53 +++++++++++----------------------------
 2 files changed, 15 insertions(+), 40 deletions(-)

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 06f9291b6fd512..2554b71765e9d0 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -369,8 +369,6 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb);
 typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc,
 				void *data);
 
-int generic_writepages(struct address_space *mapping,
-		       struct writeback_control *wbc);
 void tag_pages_for_writeback(struct address_space *mapping,
 			     pgoff_t start, pgoff_t end);
 int write_cache_pages(struct address_space *mapping,
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index ad608ef2a24365..dfeeceebba0ae0 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2526,12 +2526,8 @@ int write_cache_pages(struct address_space *mapping,
 }
 EXPORT_SYMBOL(write_cache_pages);
 
-/*
- * Function used by generic_writepages to call the real writepage
- * function and set the mapping flags on error
- */
-static int __writepage(struct page *page, struct writeback_control *wbc,
-		       void *data)
+static int writepage_cb(struct page *page, struct writeback_control *wbc,
+		void *data)
 {
 	struct address_space *mapping = data;
 	int ret = mapping->a_ops->writepage(page, wbc);
@@ -2539,34 +2535,6 @@ static int __writepage(struct page *page, struct writeback_control *wbc,
 	return ret;
 }
 
-/**
- * generic_writepages - walk the list of dirty pages of the given address space and writepage() all of them.
- * @mapping: address space structure to write
- * @wbc: subtract the number of written pages from *@wbc->nr_to_write
- *
- * This is a library function, which implements the writepages()
- * address_space_operation.
- *
- * Return: %0 on success, negative error code otherwise
- */
-int generic_writepages(struct address_space *mapping,
-		       struct writeback_control *wbc)
-{
-	struct blk_plug plug;
-	int ret;
-
-	/* deal with chardevs and other special file */
-	if (!mapping->a_ops->writepage)
-		return 0;
-
-	blk_start_plug(&plug);
-	ret = write_cache_pages(mapping, wbc, __writepage, mapping);
-	blk_finish_plug(&plug);
-	return ret;
-}
-
-EXPORT_SYMBOL(generic_writepages);
-
 int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 {
 	int ret;
@@ -2577,11 +2545,20 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	wb = inode_to_wb_wbc(mapping->host, wbc);
 	wb_bandwidth_estimate_start(wb);
 	while (1) {
-		if (mapping->a_ops->writepages)
+		if (mapping->a_ops->writepages) {
 			ret = mapping->a_ops->writepages(mapping, wbc);
-		else
-			ret = generic_writepages(mapping, wbc);
-		if ((ret != -ENOMEM) || (wbc->sync_mode != WB_SYNC_ALL))
+		} else if (mapping->a_ops->writepage) {
+			struct blk_plug plug;
+
+			blk_start_plug(&plug);
+			ret = write_cache_pages(mapping, wbc, writepage_cb,
+						mapping);
+			blk_finish_plug(&plug);
+		} else {
+			/* deal with chardevs and other special files */
+			ret = 0;
+		}
+		if (ret != -ENOMEM || wbc->sync_mode != WB_SYNC_ALL)
 			break;
 
 		/*
-- 
2.35.1

Re: [PATCH 6/6] mm: remove generic_writepages

[Jan Kara]

On Thu 29-12-22 06:10:31, Christoph Hellwig wrote:
> Now that all external callers are gone, just fold it into do_writepages.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

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

								Honza


> ---
>  include/linux/writeback.h |  2 --
>  mm/page-writeback.c       | 53 +++++++++++----------------------------
>  2 files changed, 15 insertions(+), 40 deletions(-)
> 
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index 06f9291b6fd512..2554b71765e9d0 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -369,8 +369,6 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb);
>  typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc,
>  				void *data);
>  
> -int generic_writepages(struct address_space *mapping,
> -		       struct writeback_control *wbc);
>  void tag_pages_for_writeback(struct address_space *mapping,
>  			     pgoff_t start, pgoff_t end);
>  int write_cache_pages(struct address_space *mapping,
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index ad608ef2a24365..dfeeceebba0ae0 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2526,12 +2526,8 @@ int write_cache_pages(struct address_space *mapping,
>  }
>  EXPORT_SYMBOL(write_cache_pages);
>  
> -/*
> - * Function used by generic_writepages to call the real writepage
> - * function and set the mapping flags on error
> - */
> -static int __writepage(struct page *page, struct writeback_control *wbc,
> -		       void *data)
> +static int writepage_cb(struct page *page, struct writeback_control *wbc,
> +		void *data)
>  {
>  	struct address_space *mapping = data;
>  	int ret = mapping->a_ops->writepage(page, wbc);
> @@ -2539,34 +2535,6 @@ static int __writepage(struct page *page, struct writeback_control *wbc,
>  	return ret;
>  }
>  
> -/**
> - * generic_writepages - walk the list of dirty pages of the given address space and writepage() all of them.
> - * @mapping: address space structure to write
> - * @wbc: subtract the number of written pages from *@wbc->nr_to_write
> - *
> - * This is a library function, which implements the writepages()
> - * address_space_operation.
> - *
> - * Return: %0 on success, negative error code otherwise
> - */
> -int generic_writepages(struct address_space *mapping,
> -		       struct writeback_control *wbc)
> -{
> -	struct blk_plug plug;
> -	int ret;
> -
> -	/* deal with chardevs and other special file */
> -	if (!mapping->a_ops->writepage)
> -		return 0;
> -
> -	blk_start_plug(&plug);
> -	ret = write_cache_pages(mapping, wbc, __writepage, mapping);
> -	blk_finish_plug(&plug);
> -	return ret;
> -}
> -
> -EXPORT_SYMBOL(generic_writepages);
> -
>  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  {
>  	int ret;
> @@ -2577,11 +2545,20 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  	wb = inode_to_wb_wbc(mapping->host, wbc);
>  	wb_bandwidth_estimate_start(wb);
>  	while (1) {
> -		if (mapping->a_ops->writepages)
> +		if (mapping->a_ops->writepages) {
>  			ret = mapping->a_ops->writepages(mapping, wbc);
> -		else
> -			ret = generic_writepages(mapping, wbc);
> -		if ((ret != -ENOMEM) || (wbc->sync_mode != WB_SYNC_ALL))
> +		} else if (mapping->a_ops->writepage) {
> +			struct blk_plug plug;
> +
> +			blk_start_plug(&plug);
> +			ret = write_cache_pages(mapping, wbc, writepage_cb,
> +						mapping);
> +			blk_finish_plug(&plug);
> +		} else {
> +			/* deal with chardevs and other special files */
> +			ret = 0;
> +		}
> +		if (ret != -ENOMEM || wbc->sync_mode != WB_SYNC_ALL)
>  			break;
>  
>  		/*
> -- 
> 2.35.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR