[PATCH 0/3] btrfs: btrfs_buffered_write() cleanups

[PATCH 0/3] btrfs: btrfs_buffered_write() cleanups

[Qu Wenruo]

This series is to cleanup btrfs_buffered_write() so that it's more
aligned to generic_perform_write(), and makes later migration much
easier.

All those changes are inside btrfs, and the last 2 should improve the
readablity, and prepare for the migration (since everything is now done
inside the loop).

Qu Wenruo (3):
  btrfs: make buffered write to respect fatal signals
  btrfs: cleanup the variables inside btrfs_buffered_write()
  btrfs: remove the out-of-loop cleanup from btrfs_buffered_write()

 fs/btrfs/file.c | 90 ++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 43 deletions(-)

-- 
2.47.0

[PATCH 1/3] btrfs: make buffered write to respect fatal signals

[Qu Wenruo]

This is to follow the behavior of generic_perform_write() which checks
the signal after faulting in the pages.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 10d51c8dd360..a0fa8c36a224 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1142,6 +1142,10 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			ret = -EFAULT;
 			break;
 		}
+		if (fatal_signal_pending(current)) {
+			ret = -EINTR;
+			break;
+		}
 
 		only_release_metadata = false;
 		sector_offset = pos & (fs_info->sectorsize - 1);
-- 
2.47.0

[PATCH 2/3] btrfs: cleanup the variables inside btrfs_buffered_write()

[Qu Wenruo]

We have too many variables with similar naming, which makes code much
harder to review/refactor. Some examples are:

- @offset and @sector_offset
  The former is the offset_in_page(), the latter is offset inside the
  sector.

- @dirty_sectors and @num_sectors
  The former is more or less common, the number of sectors for the
  copied range.
  Meanwhile the latter is the number of sectors for reserved bytes.

And we are mixing bytes and sectors:

- @dirty_sectors and @copied
  The former is the number of sectors for the block aligned range,
  meanwhile @copied is the unaligned length returned by
  copy_folio_from_iter_atomic()

Fix all those shenanigans by:

- Remove all number-of-sectors variables
  All calculation will now be based on bytes.
  The number-of-sectors calculation will be only done when necessary,
  and in fact there will be no call site requiring such result.

- Remove offset-in-page/offset-in-sector variables
  The offset-in-page result is only utilized once in calculating
  @write_bytes.
  So it can be open-coded.

  The offset-in-sector is utilized to calculate the number-of-sectors,
  which will be done in a more unified way.

- Calculate sector aligned length in-place using round_down() and
  round_up()
  Now for every sector aligned length, we go something like:

  	reserved_bytes = round_up(pos + write_bytes, sectorsize) -
			 round_down(pos, sectorsize);

  So no need for any offset-in-sector helper variable.

This removes the following variables:

- offset
- sector_offset
  All call sites are open-coded or replaced completely.

- dirty_sectors
  Replace by @dirty_bytes.

- num_sectors
  We use @dirty_bytes and @reserve_bytes to calculate which range needs
  to be release.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index a0fa8c36a224..5fba2e44c630 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1104,6 +1104,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 	const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
 	unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
 	bool only_release_metadata = false;
+	const u32 sectorsize = fs_info->sectorsize;
 
 	if (nowait)
 		ilock_flags |= BTRFS_ILOCK_TRY;
@@ -1123,13 +1124,10 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 	pos = iocb->ki_pos;
 	while (iov_iter_count(i) > 0) {
 		struct extent_state *cached_state = NULL;
-		size_t offset = offset_in_page(pos);
-		size_t sector_offset;
-		size_t write_bytes = min(iov_iter_count(i), PAGE_SIZE - offset);
+		size_t write_bytes = min(iov_iter_count(i), PAGE_SIZE - offset_in_page(pos));
 		size_t reserve_bytes;
+		size_t dirty_bytes;
 		size_t copied;
-		size_t dirty_sectors;
-		size_t num_sectors;
 		struct folio *folio = NULL;
 		int extents_locked;
 		bool force_page_uptodate = false;
@@ -1148,7 +1146,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		}
 
 		only_release_metadata = false;
-		sector_offset = pos & (fs_info->sectorsize - 1);
 
 		extent_changeset_release(data_reserved);
 		ret = btrfs_check_data_free_space(BTRFS_I(inode),
@@ -1178,8 +1175,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			only_release_metadata = true;
 		}
 
-		reserve_bytes = round_up(write_bytes + sector_offset,
-					 fs_info->sectorsize);
+		reserve_bytes = round_up(pos + write_bytes, sectorsize) -
+				round_down(pos, sectorsize);
 		WARN_ON(reserve_bytes == 0);
 		ret = btrfs_delalloc_reserve_metadata(BTRFS_I(inode),
 						      reserve_bytes,
@@ -1244,35 +1241,29 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			}
 		}
 
-		num_sectors = BTRFS_BYTES_TO_BLKS(fs_info, reserve_bytes);
-		dirty_sectors = round_up(copied + sector_offset,
-					fs_info->sectorsize);
-		dirty_sectors = BTRFS_BYTES_TO_BLKS(fs_info, dirty_sectors);
-
-		if (copied == 0) {
+		if (copied == 0)
 			force_page_uptodate = true;
-			dirty_sectors = 0;
-		} else {
+		else
 			force_page_uptodate = false;
-		}
 
-		if (num_sectors > dirty_sectors) {
+		dirty_bytes = round_up(pos + copied, sectorsize) -
+			      round_down(pos, sectorsize);
+
+		if (reserve_bytes > dirty_bytes) {
 			/* release everything except the sectors we dirtied */
-			release_bytes -= dirty_sectors << fs_info->sectorsize_bits;
 			if (only_release_metadata) {
 				btrfs_delalloc_release_metadata(BTRFS_I(inode),
-							release_bytes, true);
+						reserve_bytes - dirty_bytes, true);
 			} else {
 				u64 release_start = round_up(pos + copied,
 							     fs_info->sectorsize);
 				btrfs_delalloc_release_space(BTRFS_I(inode),
 						data_reserved, release_start,
-						release_bytes, true);
+						reserve_bytes - dirty_bytes, true);
 			}
 		}
 
-		release_bytes = round_up(copied + sector_offset,
-					fs_info->sectorsize);
+		release_bytes = dirty_bytes;
 
 		ret = btrfs_dirty_folio(BTRFS_I(inode), folio, pos, copied,
 					&cached_state, only_release_metadata);
-- 
2.47.0

[PATCH 3/3] btrfs: remove the out-of-loop cleanup from btrfs_buffered_write()

[Qu Wenruo]

Inside btrfs_buffered_write() we have an out-of-loop cleanup, which is
only affecting the following call sites:

- balance_dirty_pages_ratelimited_flags() failure
- prepare_one_folio() failure
- lock_and_cleanup_extent_if_need() failure
- btrfs_dirty_folio() failure

This patch extracts the out-of-loop cleanup into a helper,
buffered_write_release() so that above call sites can call that helper
before break, and remove the out-of-loop cleanup.

This allows us to:

- Remove @release_bytes completely
  Now every call site can directly use @pos, @write_bytes (or @copied)
  at the cleanup helper.

- Move @only_release_metadata into the loop

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 51 +++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 5fba2e44c630..5bfa040d3856 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1087,6 +1087,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
 	return 0;
 }
 
+static void buffered_write_release(struct btrfs_inode *binode,
+		struct extent_changeset *data_reserved, u64 pos, u64 len,
+		bool only_release_metadata)
+{
+	const u32 sectorsize = binode->root->fs_info->sectorsize;
+	u64 aligned_start;
+	u64 aligned_len;
+
+	if (!len)
+		return;
+	aligned_start = round_down(pos, sectorsize);
+	aligned_len = round_up(pos + len, sectorsize) - aligned_start;
+
+	if (only_release_metadata) {
+		btrfs_check_nocow_unlock(binode);
+		btrfs_delalloc_release_metadata(binode, aligned_len, true);
+	} else {
+		btrfs_delalloc_release_space(binode, data_reserved, pos, len, true);
+	}
+}
+
 ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 {
 	struct file *file = iocb->ki_filp;
@@ -1094,7 +1115,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
 	struct extent_changeset *data_reserved = NULL;
-	u64 release_bytes = 0;
 	u64 lockstart;
 	u64 lockend;
 	size_t num_written = 0;
@@ -1103,7 +1123,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 	unsigned int ilock_flags = 0;
 	const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
 	unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
-	bool only_release_metadata = false;
 	const u32 sectorsize = fs_info->sectorsize;
 
 	if (nowait)
@@ -1131,6 +1150,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		struct folio *folio = NULL;
 		int extents_locked;
 		bool force_page_uptodate = false;
+		bool only_release_metadata = false;
 
 		/*
 		 * Fault pages before locking them in prepare_one_folio()
@@ -1145,8 +1165,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
-		only_release_metadata = false;
-
 		extent_changeset_release(data_reserved);
 		ret = btrfs_check_data_free_space(BTRFS_I(inode),
 						  &data_reserved, pos,
@@ -1194,11 +1212,12 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
-		release_bytes = reserve_bytes;
 again:
 		ret = balance_dirty_pages_ratelimited_flags(inode->i_mapping, bdp_flags);
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
+			buffered_write_release(BTRFS_I(inode), data_reserved,
+					       pos, write_bytes, only_release_metadata);
 			break;
 		}
 
@@ -1207,6 +1226,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);
+			buffered_write_release(BTRFS_I(inode), data_reserved,
+					       pos, write_bytes, only_release_metadata);
 			break;
 		}
 
@@ -1219,6 +1240,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);
+			buffered_write_release(BTRFS_I(inode), data_reserved,
+					       pos, write_bytes, only_release_metadata);
 			ret = extents_locked;
 			break;
 		}
@@ -1263,8 +1286,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			}
 		}
 
-		release_bytes = dirty_bytes;
-
 		ret = btrfs_dirty_folio(BTRFS_I(inode), folio, pos, copied,
 					&cached_state, only_release_metadata);
 
@@ -1284,10 +1305,11 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
 		if (ret) {
 			btrfs_drop_folio(fs_info, folio, pos, copied);
+			buffered_write_release(BTRFS_I(inode), data_reserved,
+					       pos, copied, only_release_metadata);
 			break;
 		}
 
-		release_bytes = 0;
 		if (only_release_metadata)
 			btrfs_check_nocow_unlock(BTRFS_I(inode));
 
@@ -1299,19 +1321,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		num_written += copied;
 	}
 
-	if (release_bytes) {
-		if (only_release_metadata) {
-			btrfs_check_nocow_unlock(BTRFS_I(inode));
-			btrfs_delalloc_release_metadata(BTRFS_I(inode),
-					release_bytes, true);
-		} else {
-			btrfs_delalloc_release_space(BTRFS_I(inode),
-					data_reserved,
-					round_down(pos, fs_info->sectorsize),
-					release_bytes, true);
-		}
-	}
-
 	extent_changeset_free(data_reserved);
 	if (num_written > 0) {
 		pagecache_isize_extended(inode, old_isize, iocb->ki_pos);
-- 
2.47.0

Re: [PATCH 0/3] btrfs: btrfs_buffered_write() cleanups

[Qu Wenruo]

在 2024/11/11 14:29, Qu Wenruo 写道:
> This series is to cleanup btrfs_buffered_write() so that it's more
> aligned to generic_perform_write(), and makes later migration much
> easier.

AlThe generic_perform_write() is kinda deprecated and we should go iomap 
anyway, so there will be no such migration.

But the improvement on the readability should still be worthy, thus the 
patches are still good.

Thanks,
Qu
> 
> All those changes are inside btrfs, and the last 2 should improve the
> readablity, and prepare for the migration (since everything is now done
> inside the loop).
> 
> Qu Wenruo (3):
>    btrfs: make buffered write to respect fatal signals
>    btrfs: cleanup the variables inside btrfs_buffered_write()
>    btrfs: remove the out-of-loop cleanup from btrfs_buffered_write()
> 
>   fs/btrfs/file.c | 90 ++++++++++++++++++++++++++-----------------------
>   1 file changed, 47 insertions(+), 43 deletions(-)
>