remove write_one_page / folio_write_one

remove write_one_page / folio_write_one

[Christoph Hellwig]

Hi all,

this series removes the write_one_page API, and it's folioized
implementation as folio_write_one.  These helpers internally call
->writepage which we are gradually removing from the kernel.

For most callers there are better APIs to use, and this cleans them up.
The big questionmark is jfs, where the metapage abstraction uses the
pagecache in a bit of an odd way, and which would probably benefit from
not using the page cache at all like the XFS buffer cache, but given
that jfs has been in minimum maintaince mode for a long time that might
not be worth it.  So for now it just moves the implementation of
write_one_page into jfs instead.

Diffstat:
 fs/btrfs/volumes.c      |   50 ++++++++++++++++++++++++------------------------
 fs/jfs/jfs_metapage.c   |   39 ++++++++++++++++++++++++++++++++-----
 fs/minix/dir.c          |   30 +++++++++++++++++++---------
 fs/ocfs2/refcounttree.c |    9 ++++----
 fs/sysv/dir.c           |   29 ++++++++++++++++++---------
 fs/ufs/dir.c            |   29 ++++++++++++++++++---------
 include/linux/pagemap.h |    6 -----
 mm/page-writeback.c     |   40 --------------------------------------
 8 files changed, 122 insertions(+), 110 deletions(-)

[PATCH 1/7] btrfs: don't read the disk superblock for zoned devices in btrfs_scratch_superblocks

[Christoph Hellwig]

For zoned devices, btrfs_scratch_superblocks just resets the sb zones,
which means there is no need to even read the previous superblock.
Split the code to read, zero and write the superblock for conventional
devices into a separate helper so that it isn't called for zoned
devices.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/volumes.c | 51 +++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aa25fa335d3ed1..1378f5ad5ed4c4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2005,42 +2005,43 @@ static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
 	return num_devices;
 }
 
+static void btrfs_scratch_superblock(struct btrfs_fs_info *fs_info,
+				     struct block_device *bdev, int copy_num)
+{
+	struct btrfs_super_block *disk_super;
+	struct page *page;
+	int ret;
+
+	disk_super = btrfs_read_dev_one_super(bdev, copy_num, false);
+	if (IS_ERR(disk_super))
+		return;
+	memset(&disk_super->magic, 0, sizeof(disk_super->magic));
+	page = virt_to_page(disk_super);
+	set_page_dirty(page);
+	lock_page(page);
+	/* write_on_page() unlocks the page */
+	ret = write_one_page(page);
+	if (ret)
+		btrfs_warn(fs_info,
+			"error clearing superblock number %d (%d)",
+			copy_num, ret);
+	btrfs_release_disk_super(disk_super);
+}
+
 void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
 			       struct block_device *bdev,
 			       const char *device_path)
 {
-	struct btrfs_super_block *disk_super;
 	int copy_num;
 
 	if (!bdev)
 		return;
 
 	for (copy_num = 0; copy_num < BTRFS_SUPER_MIRROR_MAX; copy_num++) {
-		struct page *page;
-		int ret;
-
-		disk_super = btrfs_read_dev_one_super(bdev, copy_num, false);
-		if (IS_ERR(disk_super))
-			continue;
-
-		if (bdev_is_zoned(bdev)) {
+		if (bdev_is_zoned(bdev))
 			btrfs_reset_sb_log_zones(bdev, copy_num);
-			continue;
-		}
-
-		memset(&disk_super->magic, 0, sizeof(disk_super->magic));
-
-		page = virt_to_page(disk_super);
-		set_page_dirty(page);
-		lock_page(page);
-		/* write_on_page() unlocks the page */
-		ret = write_one_page(page);
-		if (ret)
-			btrfs_warn(fs_info,
-				"error clearing superblock number %d (%d)",
-				copy_num, ret);
-		btrfs_release_disk_super(disk_super);
-
+		else
+			btrfs_scratch_superblock(fs_info, bdev, copy_num);
 	}
 
 	/* Notify udev that device has changed */
-- 
2.35.1

[PATCH 2/7] btrfs: stop using write_one_page in btrfs_scratch_superblock

[Christoph Hellwig]

write_one_page is an awkward interface that expects the page locked
and ->writepage to be implemented.  Just mark the sb dirty, put
the page and then call the proper bdev helper to sync the range.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1378f5ad5ed4c4..10e98b004a2fa3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2009,23 +2009,22 @@ static void btrfs_scratch_superblock(struct btrfs_fs_info *fs_info,
 				     struct block_device *bdev, int copy_num)
 {
 	struct btrfs_super_block *disk_super;
-	struct page *page;
+	const size_t len = sizeof(disk_super->magic);
+	u64 bytenr = btrfs_sb_offset(copy_num);
 	int ret;
 
-	disk_super = btrfs_read_dev_one_super(bdev, copy_num, false);
+	disk_super = btrfs_read_disk_super(bdev, bytenr, bytenr);
 	if (IS_ERR(disk_super))
 		return;
-	memset(&disk_super->magic, 0, sizeof(disk_super->magic));
-	page = virt_to_page(disk_super);
-	set_page_dirty(page);
-	lock_page(page);
-	/* write_on_page() unlocks the page */
-	ret = write_one_page(page);
+	memset(&disk_super->magic, 0, len);
+	set_page_dirty(virt_to_page(disk_super));
+	btrfs_release_disk_super(disk_super);
+
+	ret = sync_blockdev_range(bdev, bytenr, bytenr + len - 1);
 	if (ret)
 		btrfs_warn(fs_info,
 			"error clearing superblock number %d (%d)",
 			copy_num, ret);
-	btrfs_release_disk_super(disk_super);
 }
 
 void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
-- 
2.35.1

[PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

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.

Ported from an ext2 patch by Jan Kara.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/minix/dir.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/fs/minix/dir.c b/fs/minix/dir.c
index dcfe5b25378b54..d48b09271dc48f 100644
--- a/fs/minix/dir.c
+++ b/fs/minix/dir.c
@@ -46,21 +46,27 @@ minix_last_byte(struct inode *inode, unsigned long page_nr)
 	return last_byte;
 }
 
-static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
+static void dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
 {
 	struct address_space *mapping = page->mapping;
 	struct inode *dir = mapping->host;
-	int err = 0;
+
 	block_write_end(NULL, mapping, pos, len, len, page, NULL);
 
 	if (pos+len > dir->i_size) {
 		i_size_write(dir, pos+len);
 		mark_inode_dirty(dir);
 	}
-	if (IS_DIRSYNC(dir))
-		err = write_one_page(page);
-	else
-		unlock_page(page);
+	unlock_page(page);
+}
+
+static int minix_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;
 }
 
@@ -274,9 +280,10 @@ int minix_add_link(struct dentry *dentry, struct inode *inode)
 		memset (namx + namelen, 0, sbi->s_dirsize - namelen - 2);
 		de->inode = inode->i_ino;
 	}
-	err = dir_commit_chunk(page, pos, sbi->s_dirsize);
+	dir_commit_chunk(page, pos, sbi->s_dirsize);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 	mark_inode_dirty(dir);
+	minix_handle_dirsync(dir);
 out_put:
 	dir_put_page(page);
 out:
@@ -302,13 +309,15 @@ int minix_delete_entry(struct minix_dir_entry *de, struct page *page)
 			((minix3_dirent *) de)->inode = 0;
 		else
 			de->inode = 0;
-		err = dir_commit_chunk(page, pos, len);
+		dir_commit_chunk(page, pos, len);
 	} else {
 		unlock_page(page);
 	}
 	dir_put_page(page);
 	inode->i_ctime = inode->i_mtime = current_time(inode);
 	mark_inode_dirty(inode);
+	if (!err)
+		err = minix_handle_dirsync(inode);
 	return err;
 }
 
@@ -349,7 +358,8 @@ int minix_make_empty(struct inode *inode, struct inode *dir)
 	}
 	kunmap_atomic(kaddr);
 
-	err = dir_commit_chunk(page, 0, 2 * sbi->s_dirsize);
+	dir_commit_chunk(page, 0, 2 * sbi->s_dirsize);
+	err = minix_handle_dirsync(inode);
 fail:
 	put_page(page);
 	return err;
@@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
 			((minix3_dirent *) de)->inode = inode->i_ino;
 		else
 			de->inode = inode->i_ino;
-		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
+		dir_commit_chunk(page, pos, sbi->s_dirsize);
 	} else {
 		unlock_page(page);
 	}
-- 
2.35.1

[PATCH 4/7] sysv: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

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.

Ported from an ext2 patch by Jan Kara.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/sysv/dir.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c
index 88e38cd8f5c9ae..1d852ca6388297 100644
--- a/fs/sysv/dir.c
+++ b/fs/sysv/dir.c
@@ -34,21 +34,26 @@ static inline void dir_put_page(struct page *page)
 	put_page(page);
 }
 
-static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
+static void dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
 {
 	struct address_space *mapping = page->mapping;
 	struct inode *dir = mapping->host;
-	int err = 0;
 
 	block_write_end(NULL, mapping, pos, len, len, page, NULL);
 	if (pos+len > dir->i_size) {
 		i_size_write(dir, pos+len);
 		mark_inode_dirty(dir);
 	}
-	if (IS_DIRSYNC(dir))
-		err = write_one_page(page);
-	else
-		unlock_page(page);
+	unlock_page(page);
+}
+
+static int sysv_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;
 }
 
@@ -215,9 +220,10 @@ int sysv_add_link(struct dentry *dentry, struct inode *inode)
 	memcpy (de->name, name, namelen);
 	memset (de->name + namelen, 0, SYSV_DIRSIZE - namelen - 2);
 	de->inode = cpu_to_fs16(SYSV_SB(inode->i_sb), inode->i_ino);
-	err = dir_commit_chunk(page, pos, SYSV_DIRSIZE);
+	dir_commit_chunk(page, pos, SYSV_DIRSIZE);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 	mark_inode_dirty(dir);
+	sysv_handle_dirsync(dir);
 out_page:
 	dir_put_page(page);
 out:
@@ -238,10 +244,11 @@ int sysv_delete_entry(struct sysv_dir_entry *de, struct page *page)
 	err = sysv_prepare_chunk(page, pos, SYSV_DIRSIZE);
 	BUG_ON(err);
 	de->inode = 0;
-	err = dir_commit_chunk(page, pos, SYSV_DIRSIZE);
+	dir_commit_chunk(page, pos, SYSV_DIRSIZE);
 	dir_put_page(page);
 	inode->i_ctime = inode->i_mtime = current_time(inode);
 	mark_inode_dirty(inode);
+	sysv_handle_dirsync(inode);
 	return err;
 }
 
@@ -272,7 +279,8 @@ int sysv_make_empty(struct inode *inode, struct inode *dir)
 	strcpy(de->name,"..");
 
 	kunmap(page);
-	err = dir_commit_chunk(page, 0, 2 * SYSV_DIRSIZE);
+	dir_commit_chunk(page, 0, 2 * SYSV_DIRSIZE);
+	err = sysv_handle_dirsync(inode);
 fail:
 	put_page(page);
 	return err;
@@ -336,10 +344,11 @@ void sysv_set_link(struct sysv_dir_entry *de, struct page *page,
 	err = sysv_prepare_chunk(page, pos, SYSV_DIRSIZE);
 	BUG_ON(err);
 	de->inode = cpu_to_fs16(SYSV_SB(inode->i_sb), inode->i_ino);
-	err = dir_commit_chunk(page, pos, SYSV_DIRSIZE);
+	dir_commit_chunk(page, pos, SYSV_DIRSIZE);
 	dir_put_page(page);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 	mark_inode_dirty(dir);
+	sysv_handle_dirsync(inode);
 }
 
 struct sysv_dir_entry * sysv_dotdot (struct inode *dir, struct page **p)
-- 
2.35.1

[PATCH 5/7] ufs: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

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.

Ported from an ext2 patch by Jan Kara.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ufs/dir.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
index 391efaf1d52897..379d75796a5ce3 100644
--- a/fs/ufs/dir.c
+++ b/fs/ufs/dir.c
@@ -42,11 +42,10 @@ static inline int ufs_match(struct super_block *sb, int len,
 	return !memcmp(name, de->d_name, len);
 }
 
-static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len)
+static void ufs_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);
@@ -54,10 +53,16 @@ static int ufs_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);
-	else
-		unlock_page(page);
+	unlock_page(page);
+}
+
+static int ufs_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;
 }
 
@@ -99,11 +104,12 @@ void ufs_set_link(struct inode *dir, struct ufs_dir_entry *de,
 	de->d_ino = cpu_to_fs32(dir->i_sb, inode->i_ino);
 	ufs_set_de_type(dir->i_sb, de, inode->i_mode);
 
-	err = ufs_commit_chunk(page, pos, len);
+	ufs_commit_chunk(page, pos, len);
 	ufs_put_page(page);
 	if (update_times)
 		dir->i_mtime = dir->i_ctime = current_time(dir);
 	mark_inode_dirty(dir);
+	ufs_handle_dirsync(dir);
 }
 
 
@@ -390,10 +396,11 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
 	de->d_ino = cpu_to_fs32(sb, inode->i_ino);
 	ufs_set_de_type(sb, de, inode->i_mode);
 
-	err = ufs_commit_chunk(page, pos, rec_len);
+	ufs_commit_chunk(page, pos, rec_len);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 
 	mark_inode_dirty(dir);
+	err = ufs_handle_dirsync(dir);
 	/* OFFSET_CACHE */
 out_put:
 	ufs_put_page(page);
@@ -531,9 +538,10 @@ int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir,
 	if (pde)
 		pde->d_reclen = cpu_to_fs16(sb, to - from);
 	dir->d_ino = 0;
-	err = ufs_commit_chunk(page, pos, to - from);
+	ufs_commit_chunk(page, pos, to - from);
 	inode->i_ctime = inode->i_mtime = current_time(inode);
 	mark_inode_dirty(inode);
+	err = ufs_handle_dirsync(inode);
 out:
 	ufs_put_page(page);
 	UFSD("EXIT\n");
@@ -579,7 +587,8 @@ int ufs_make_empty(struct inode * inode, struct inode *dir)
 	strcpy (de->d_name, "..");
 	kunmap(page);
 
-	err = ufs_commit_chunk(page, 0, chunk_size);
+	ufs_commit_chunk(page, 0, chunk_size);
+	err = ufs_handle_dirsync(inode);
 fail:
 	put_page(page);
 	return err;
-- 
2.35.1

[PATCH 6/7] ocfs2: don't use write_one_page in ocfs2_duplicate_clusters_by_page

[Christoph Hellwig]

Use filemap_write_and_wait_range to write back the range of the dirty
page instead of write_one_page in preparation of removing write_one_page
and eventually ->writepage.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 623db358b1efa8..4a73405962ec4f 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -2952,10 +2952,11 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 		 */
 		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
 			if (PageDirty(page)) {
-				/*
-				 * write_on_page will unlock the page on return
-				 */
-				ret = write_one_page(page);
+				unlock_page(page);
+				put_page(page);
+
+				ret = filemap_write_and_wait_range(mapping,
+						offset, map_end - 1);
 				goto retry;
 			}
 		}
-- 
2.35.1

[PATCH 7/7] mm,jfs: move write_one_page/folio_write_one to jfs

[Christoph Hellwig]

The last remaining user of folio_write_one through the write_one_page
wrapper is jfs, so move the functionality there and hard code the
call to metapage_writepage.

Note that the use of the pagecache by the jfs 'metapage' buffer cache
is a bit odd, and we could probably do without VM-level dirty tracking
at all, but that's a change for another time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/jfs/jfs_metapage.c   | 39 ++++++++++++++++++++++++++++++++++-----
 include/linux/pagemap.h |  6 ------
 mm/page-writeback.c     | 40 ----------------------------------------
 3 files changed, 34 insertions(+), 51 deletions(-)

diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index 2e8461ce74de69..961569c1115901 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -691,6 +691,35 @@ void grab_metapage(struct metapage * mp)
 	unlock_page(mp->page);
 }
 
+static int metapage_write_one(struct page *page)
+{
+	struct folio *folio = page_folio(page);
+	struct address_space *mapping = folio->mapping;
+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+		.nr_to_write = folio_nr_pages(folio),
+	};
+	int ret = 0;
+
+	BUG_ON(!folio_test_locked(folio));
+
+	folio_wait_writeback(folio);
+
+	if (folio_clear_dirty_for_io(folio)) {
+		folio_get(folio);
+		ret = metapage_writepage(page, &wbc);
+		if (ret == 0)
+			folio_wait_writeback(folio);
+		folio_put(folio);
+	} else {
+		folio_unlock(folio);
+	}
+
+	if (!ret)
+		ret = filemap_check_errors(mapping);
+	return ret;
+}
+
 void force_metapage(struct metapage *mp)
 {
 	struct page *page = mp->page;
@@ -700,8 +729,8 @@ void force_metapage(struct metapage *mp)
 	get_page(page);
 	lock_page(page);
 	set_page_dirty(page);
-	if (write_one_page(page))
-		jfs_error(mp->sb, "write_one_page() failed\n");
+	if (metapage_write_one(page))
+		jfs_error(mp->sb, "metapage_write_one() failed\n");
 	clear_bit(META_forcewrite, &mp->flag);
 	put_page(page);
 }
@@ -746,9 +775,9 @@ void release_metapage(struct metapage * mp)
 		set_page_dirty(page);
 		if (test_bit(META_sync, &mp->flag)) {
 			clear_bit(META_sync, &mp->flag);
-			if (write_one_page(page))
-				jfs_error(mp->sb, "write_one_page() failed\n");
-			lock_page(page); /* write_one_page unlocks the page */
+			if (metapage_write_one(page))
+				jfs_error(mp->sb, "metapage_write_one() failed\n");
+			lock_page(page);
 		}
 	} else if (mp->lsn)	/* discard_metapage doesn't remove it */
 		remove_from_logsync(mp);
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 29e1f9e76eb6dd..4b3a7124c76712 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -1062,12 +1062,6 @@ static inline void folio_cancel_dirty(struct folio *folio)
 bool folio_clear_dirty_for_io(struct folio *folio);
 bool clear_page_dirty_for_io(struct page *page);
 void folio_invalidate(struct folio *folio, size_t offset, size_t length);
-int __must_check folio_write_one(struct folio *folio);
-static inline int __must_check write_one_page(struct page *page)
-{
-	return folio_write_one(page_folio(page));
-}
-
 int __set_page_dirty_nobuffers(struct page *page);
 bool noop_dirty_folio(struct address_space *mapping, struct folio *folio);
 
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index dfeeceebba0ae0..2430fd09607742 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2581,46 +2581,6 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	return ret;
 }
 
-/**
- * folio_write_one - write out a single folio and wait on I/O.
- * @folio: The folio to write.
- *
- * The folio must be locked by the caller and will be unlocked upon return.
- *
- * Note that the mapping's AS_EIO/AS_ENOSPC flags will be cleared when this
- * function returns.
- *
- * Return: %0 on success, negative error code otherwise
- */
-int folio_write_one(struct folio *folio)
-{
-	struct address_space *mapping = folio->mapping;
-	int ret = 0;
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_ALL,
-		.nr_to_write = folio_nr_pages(folio),
-	};
-
-	BUG_ON(!folio_test_locked(folio));
-
-	folio_wait_writeback(folio);
-
-	if (folio_clear_dirty_for_io(folio)) {
-		folio_get(folio);
-		ret = mapping->a_ops->writepage(&folio->page, &wbc);
-		if (ret == 0)
-			folio_wait_writeback(folio);
-		folio_put(folio);
-	} else {
-		folio_unlock(folio);
-	}
-
-	if (!ret)
-		ret = filemap_check_errors(mapping);
-	return ret;
-}
-EXPORT_SYMBOL(folio_write_one);
-
 /*
  * For address_spaces which do not use buffers nor write back.
  */
-- 
2.35.1

Re: [PATCH 2/7] btrfs: stop using write_one_page in btrfs_scratch_superblock

[Matthew Wilcox]

On Sun, Jan 08, 2023 at 05:56:40PM +0100, Christoph Hellwig wrote:
> +	memset(&disk_super->magic, 0, len);
> +	set_page_dirty(virt_to_page(disk_super));

Could we make this:

	folio_mark_dirty(virt_to_folio(disk_super));

Other than that, looks good.

Re: [PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Matthew Wilcox]

On Sun, Jan 08, 2023 at 05:56:41PM +0100, Christoph Hellwig wrote:
> @@ -274,9 +280,10 @@ int minix_add_link(struct dentry *dentry, struct inode *inode)
>  		memset (namx + namelen, 0, sbi->s_dirsize - namelen - 2);
>  		de->inode = inode->i_ino;
>  	}
> -	err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> +	dir_commit_chunk(page, pos, sbi->s_dirsize);
>  	dir->i_mtime = dir->i_ctime = current_time(dir);
>  	mark_inode_dirty(dir);
> +	minix_handle_dirsync(dir);

Doesn't this need to be:

	err = minix_handle_dirsync(dir);

> @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
>  			((minix3_dirent *) de)->inode = inode->i_ino;
>  		else
>  			de->inode = inode->i_ino;
> -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> +		dir_commit_chunk(page, pos, sbi->s_dirsize);
>  	} else {
>  		unlock_page(page);
>  	}
> -- 

Aren't you missing a call to minix_handle_dirsync() in this function?

Re: [PATCH 4/7] sysv: don't flush page immediately for DIRSYNC directories

[Matthew Wilcox]

On Sun, Jan 08, 2023 at 05:56:42PM +0100, Christoph Hellwig wrote:
> 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.

Similar concerns to the minix patch here ... missing assignments to
'err'.

Re: remove write_one_page / folio_write_one

[Matthew Wilcox]

On Sun, Jan 08, 2023 at 05:56:38PM +0100, Christoph Hellwig wrote:
> this series removes the write_one_page API, and it's folioized
> implementation as folio_write_one.  These helpers internally call
> ->writepage which we are gradually removing from the kernel.
> 
> For most callers there are better APIs to use, and this cleans them up.
> The big questionmark is jfs, where the metapage abstraction uses the
> pagecache in a bit of an odd way, and which would probably benefit from
> not using the page cache at all like the XFS buffer cache, but given
> that jfs has been in minimum maintaince mode for a long time that might
> not be worth it.  So for now it just moves the implementation of
> write_one_page into jfs instead.

Thanks.  This totally wrecks a patchset I was working on, but it's
definitely the right thing to do, and I'll rebase on top of it once
it's in.  Looking forward to v2 with my niggles fixed ;-)

Re: [PATCH 6/7] ocfs2: don't use write_one_page in ocfs2_duplicate_clusters_by_page

[Jan Kara]

On Sun 08-01-23 17:56:44, Christoph Hellwig wrote:
> Use filemap_write_and_wait_range to write back the range of the dirty
> page instead of write_one_page in preparation of removing write_one_page
> and eventually ->writepage.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ocfs2/refcounttree.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index 623db358b1efa8..4a73405962ec4f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -2952,10 +2952,11 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
>  		 */
>  		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
>  			if (PageDirty(page)) {
> -				/*
> -				 * write_on_page will unlock the page on return
> -				 */
> -				ret = write_one_page(page);
> +				unlock_page(page);
> +				put_page(page);
> +
> +				ret = filemap_write_and_wait_range(mapping,
> +						offset, map_end - 1);
>  				goto retry;
>  			}
>  		}
> -- 
> 2.35.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: remove write_one_page / folio_write_one

[David Sterba]

On Sun, Jan 08, 2023 at 05:56:38PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series removes the write_one_page API, and it's folioized
> implementation as folio_write_one.  These helpers internally call
> ->writepage which we are gradually removing from the kernel.
> 
> For most callers there are better APIs to use, and this cleans them up.
> The big questionmark is jfs, where the metapage abstraction uses the
> pagecache in a bit of an odd way, and which would probably benefit from
> not using the page cache at all like the XFS buffer cache, but given
> that jfs has been in minimum maintaince mode for a long time that might
> not be worth it.  So for now it just moves the implementation of
> write_one_page into jfs instead.
> 
> Diffstat:
>  fs/btrfs/volumes.c      |   50 ++++++++++++++++++++++++------------------------

The btrfs patches were sent separately some time ago, now merged to
misc-next with updated changelogs and with the suggested switch to folio
API in the 2nd patch.

Re: [Ocfs2-devel] [PATCH 6/7] ocfs2: don't use write_one_page in ocfs2_duplicate_clusters_by_page

[Joseph Qi]

On 1/9/23 12:56 AM, Christoph Hellwig via Ocfs2-devel wrote:
> Use filemap_write_and_wait_range to write back the range of the dirty
> page instead of write_one_page in preparation of removing write_one_page
> and eventually ->writepage.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

> ---
>  fs/ocfs2/refcounttree.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index 623db358b1efa8..4a73405962ec4f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -2952,10 +2952,11 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
>  		 */
>  		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
>  			if (PageDirty(page)) {
> -				/*
> -				 * write_on_page will unlock the page on return
> -				 */
> -				ret = write_one_page(page);
> +				unlock_page(page);
> +				put_page(page);
> +
> +				ret = filemap_write_and_wait_range(mapping,
> +						offset, map_end - 1);
>  				goto retry;
>  			}
>  		}

Re: remove write_one_page / folio_write_one

[Christoph Hellwig]

On Mon, Jan 09, 2023 at 08:53:09PM +0100, David Sterba wrote:
> The btrfs patches were sent separately some time ago, now merged to
> misc-next with updated changelogs and with the suggested switch to folio
> API in the 2nd patch.

Yes, 7 weeks ago to be exact.  I wish we could just feed everything
together now that we've missed the previous merge window, as that
makes patch juggling for Andrew and Matthew a lot simpler.

Re: [PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

On Sun, Jan 08, 2023 at 09:17:26PM +0000, Matthew Wilcox wrote:
> > +	dir_commit_chunk(page, pos, sbi->s_dirsize);
> >  	dir->i_mtime = dir->i_ctime = current_time(dir);
> >  	mark_inode_dirty(dir);
> > +	minix_handle_dirsync(dir);
> 
> Doesn't this need to be:
> 
> 	err = minix_handle_dirsync(dir);

Yes, fixed.

> 
> > @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
> >  			((minix3_dirent *) de)->inode = inode->i_ino;
> >  		else
> >  			de->inode = inode->i_ino;
> > -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> > +		dir_commit_chunk(page, pos, sbi->s_dirsize);
> >  	} else {
> >  		unlock_page(page);
> >  	}
> > -- 
> 
> Aren't you missing a call to minix_handle_dirsync() in this function?

Yes, fixed.

Re: [PATCH 4/7] sysv: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

On Sun, Jan 08, 2023 at 09:19:22PM +0000, Matthew Wilcox wrote:
> On Sun, Jan 08, 2023 at 05:56:42PM +0100, Christoph Hellwig wrote:
> > 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.
> 
> Similar concerns to the minix patch here ... missing assignments to
> 'err'.

Fixed.

Re: remove write_one_page / folio_write_one

[David Sterba]

On Tue, Jan 10, 2023 at 09:16:53AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 09, 2023 at 08:53:09PM +0100, David Sterba wrote:
> > The btrfs patches were sent separately some time ago, now merged to
> > misc-next with updated changelogs and with the suggested switch to folio
> > API in the 2nd patch.
> 
> Yes, 7 weeks ago to be exact.  I wish we could just feed everything
> together now that we've missed the previous merge window, as that
> makes patch juggling for Andrew and Matthew a lot simpler.

The patches are not fixes so they should wait for the next merge window.

Re: remove write_one_page / folio_write_one

[Christoph Hellwig]

On Tue, Jan 10, 2023 at 02:00:42PM +0100, David Sterba wrote:
> On Tue, Jan 10, 2023 at 09:16:53AM +0100, Christoph Hellwig wrote:
> > On Mon, Jan 09, 2023 at 08:53:09PM +0100, David Sterba wrote:
> > > The btrfs patches were sent separately some time ago, now merged to
> > > misc-next with updated changelogs and with the suggested switch to folio
> > > API in the 2nd patch.
> > 
> > Yes, 7 weeks ago to be exact.  I wish we could just feed everything
> > together now that we've missed the previous merge window, as that
> > makes patch juggling for Andrew and Matthew a lot simpler.
> 
> The patches are not fixes so they should wait for the next merge window.

Agreed.  But it would be a lot simpler if we could queue them up in
-mm with the other patches now that we've missed the previous merge
window.

Re: [PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Al Viro]

On Tue, Jan 10, 2023 at 09:22:25AM +0100, Christoph Hellwig wrote:
> On Sun, Jan 08, 2023 at 09:17:26PM +0000, Matthew Wilcox wrote:
> > > +	dir_commit_chunk(page, pos, sbi->s_dirsize);
> > >  	dir->i_mtime = dir->i_ctime = current_time(dir);
> > >  	mark_inode_dirty(dir);
> > > +	minix_handle_dirsync(dir);
> > 
> > Doesn't this need to be:
> > 
> > 	err = minix_handle_dirsync(dir);
> 
> Yes, fixed.
> 
> > 
> > > @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
> > >  			((minix3_dirent *) de)->inode = inode->i_ino;
> > >  		else
> > >  			de->inode = inode->i_ino;
> > > -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> > > +		dir_commit_chunk(page, pos, sbi->s_dirsize);
> > >  	} else {
> > >  		unlock_page(page);
> > >  	}
> > > -- 
> > 
> > Aren't you missing a call to minix_handle_dirsync() in this function?
> 
> Yes, fixed.

More seriously, all those ..._set_link() need to return an error and their
callers (..._rename()) need to deal with failures.  That goes for ext2
as well, and that part is worth splitting off into a prereq - it's a -stable
fodder.

Re: [PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Christoph Hellwig]

On Wed, Jan 11, 2023 at 02:20:41AM +0000, Al Viro wrote:
> More seriously, all those ..._set_link() need to return an error and their
> callers (..._rename()) need to deal with failures.

That's actually what I did yesterday:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/remove-write_one_page

Re: [PATCH 3/7] minix: don't flush page immediately for DIRSYNC directories

[Al Viro]

On Wed, Jan 11, 2023 at 05:26:41AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 11, 2023 at 02:20:41AM +0000, Al Viro wrote:
> > More seriously, all those ..._set_link() need to return an error and their
> > callers (..._rename()) need to deal with failures.
> 
> That's actually what I did yesterday:
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/remove-write_one_page

ext2 also has that bug.  As well as "need to check for delete_entry errors"
one (also in ext2_rename()).

Completely untested patch follows:

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index e5cbc27ba459..b38fab33cd0d 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -461,7 +461,7 @@ static int ext2_handle_dirsync(struct inode *dir)
 	return err;
 }
 
-void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
+int ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 		   struct page *page, void *page_addr, struct inode *inode,
 		   int update_times)
 {
@@ -480,7 +480,7 @@ void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 		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);
+	return ext2_handle_dirsync(dir);
 }
 
 /*
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 28de11a22e5f..95c083bb1b7c 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -734,7 +734,7 @@ extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
 			     char *kaddr);
 extern int ext2_empty_dir (struct inode *);
 extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
-extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
+extern int ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
 			  struct inode *, int);
 static inline void ext2_put_page(struct page *page, void *page_addr)
 {
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index c056957221a2..5e3397680faa 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -370,8 +370,10 @@ static int ext2_rename (struct user_namespace * mnt_userns,
 			err = PTR_ERR(new_de);
 			goto out_dir;
 		}
-		ext2_set_link(new_dir, new_de, new_page, page_addr, old_inode, 1);
+		err = ext2_set_link(new_dir, new_de, new_page, page_addr, old_inode, 1);
 		ext2_put_page(new_page, page_addr);
+		if (err)
+			goto out_dir;
 		new_inode->i_ctime = current_time(new_inode);
 		if (dir_de)
 			drop_nlink(new_inode);
@@ -391,7 +393,9 @@ static int ext2_rename (struct user_namespace * mnt_userns,
 	old_inode->i_ctime = current_time(old_inode);
 	mark_inode_dirty(old_inode);
 
-	ext2_delete_entry(old_de, old_page, old_page_addr);
+	err = ext2_delete_entry(old_de, old_page, old_page_addr);
+	if (err)
+		goto out_dir;
 
 	if (dir_de) {
 		if (old_dir != new_dir)

Re: remove write_one_page / folio_write_one

[David Sterba]

On Tue, Jan 10, 2023 at 04:32:16PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 10, 2023 at 02:00:42PM +0100, David Sterba wrote:
> > On Tue, Jan 10, 2023 at 09:16:53AM +0100, Christoph Hellwig wrote:
> > > On Mon, Jan 09, 2023 at 08:53:09PM +0100, David Sterba wrote:
> > > > The btrfs patches were sent separately some time ago, now merged to
> > > > misc-next with updated changelogs and with the suggested switch to folio
> > > > API in the 2nd patch.
> > > 
> > > Yes, 7 weeks ago to be exact.  I wish we could just feed everything
> > > together now that we've missed the previous merge window, as that
> > > makes patch juggling for Andrew and Matthew a lot simpler.
> > 
> > The patches are not fixes so they should wait for the next merge window.
> 
> Agreed.  But it would be a lot simpler if we could queue them up in
> -mm with the other patches now that we've missed the previous merge
> window.

Ok then, to make it easier for the folio changes I'll send the two btrfs
patches next week. As the rest of the series does not depend on it,
I don't see the need to let them all go via the mm tree.

Re: remove write_one_page / folio_write_one

[Christoph Hellwig]

On Wed, Jan 11, 2023 at 08:20:27PM +0100, David Sterba wrote:
> Ok then, to make it easier for the folio changes I'll send the two btrfs
> patches next week. As the rest of the series does not depend on it,
> I don't see the need to let them all go via the mm tree.

The last patch depends on the btrfs changes as it removes write_one_page
entirely.