[PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[Qu Wenruo]

The function btrfs_buffered_write() is implementing all the complex
heavy-lifting work inside a huge while () loop, which makes later large
data folios work much harder.

The first patch is a patch that already submitted to the mailing list
recently, but all later reworks depends on that patch, thus it is
included in the series.

The core of the whole series is to introduce a helper function,
copy_one_range() to do the buffer copy into one single folio.

Patch 2 is a preparation that moves the error cleanup into the main loop,
so we do not have dedicated out-of-loop cleanup.

Patch 3 is another preparation that extract the space reservation code
into a helper, make the final refactor patch a little more smaller.

And patch 4 is the main dish, with all the refactoring happening inside
it.

Qu Wenruo (4):
  btrfs: remove force_page_uptodate variable from btrfs_buffered_write()
  btrfs: cleanup the reserved space inside the loop of
    btrfs_buffered_write()
  btrfs: extract the space reservation code from btrfs_buffered_write()
  btrfs: extract the main loop of btrfs_buffered_write() into a helper

 fs/btrfs/file.c | 434 ++++++++++++++++++++++++++----------------------
 1 file changed, 231 insertions(+), 203 deletions(-)

-- 
2.49.0

[PATCH 1/4] btrfs: remove force_page_uptodate variable from btrfs_buffered_write()

[Qu Wenruo]

Commit c87c299776e4 ("btrfs: make buffered write to copy one page a
time") changed how the variable @force_page_uptodate is updated.

Before that commit the variable is only initialized to false at the
beginning of the function, and after hitting a short copy, the next
retry on the same folio will force the foilio to be read from the disk.

But after the commit, the variable is always updated to false for each
iteration, causing prepare_one_folio() never to get a true value passed
in.

The change in behavior is not a huge deal, it only makes a difference
on how we handle short copies:

Old: Allow the buffer to be split

     The first short copy will be rejected, that's the same for both
     cases.

     But for the next retry, we require the folio to be read from disk.

     Then even if we hit a short copy again, since the folio is already
     uptodate, we do not need to handle partial uptodate range, and can
     continue, marking the short copied range as dirty and continue.

     This will split the buffer write into the folio as two buffered
     writes.

New: Do not allow the buffer to be split

     The first short copy will be rejected, that's the same for both
     cases.

     For the next retry, we do nothing special, thus if the short copy
     happened again, we reject it again, until either the short copy is
     gone, or we failed to fault in the buffer.

     This will mean the buffer write into the folio will either fail or
     success, no split will happen.

To me, either solution is fine, but the newer one makes it simpler and
requires no special handling, so I prefer that solution.

And since @force_page_uptodate is always false when passed into
prepare_one_folio(), we can just remove the variable.

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index c2648858772a..b7eb1f0164bb 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -800,7 +800,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
  * On success return a locked folio and 0
  */
 static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64 pos,
-				  u64 len, bool force_uptodate)
+				  u64 len)
 {
 	u64 clamp_start = max_t(u64, pos, folio_pos(folio));
 	u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
@@ -810,8 +810,7 @@ static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64
 	if (folio_test_uptodate(folio))
 		return 0;
 
-	if (!force_uptodate &&
-	    IS_ALIGNED(clamp_start, blocksize) &&
+	if (IS_ALIGNED(clamp_start, blocksize) &&
 	    IS_ALIGNED(clamp_end, blocksize))
 		return 0;
 
@@ -858,7 +857,7 @@ static gfp_t get_prepare_gfp_flags(struct inode *inode, bool nowait)
  */
 static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_ret,
 				      loff_t pos, size_t write_bytes,
-				      bool force_uptodate, bool nowait)
+				      bool nowait)
 {
 	unsigned long index = pos >> PAGE_SHIFT;
 	gfp_t mask = get_prepare_gfp_flags(inode, nowait);
@@ -881,7 +880,7 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
 		folio_put(folio);
 		return ret;
 	}
-	ret = prepare_uptodate_folio(inode, folio, pos, write_bytes, force_uptodate);
+	ret = prepare_uptodate_folio(inode, folio, pos, write_bytes);
 	if (ret) {
 		/* The folio is already unlocked. */
 		folio_put(folio);
@@ -1127,7 +1126,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		size_t num_sectors;
 		struct folio *folio = NULL;
 		int extents_locked;
-		bool force_page_uptodate = false;
 
 		/*
 		 * Fault pages before locking them in prepare_one_folio()
@@ -1196,8 +1194,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
-		ret = prepare_one_folio(inode, &folio, pos, write_bytes,
-					force_page_uptodate, false);
+		ret = prepare_one_folio(inode, &folio, pos, write_bytes, false);
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);
@@ -1240,12 +1237,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 					fs_info->sectorsize);
 		dirty_sectors = BTRFS_BYTES_TO_BLKS(fs_info, dirty_sectors);
 
-		if (copied == 0) {
-			force_page_uptodate = true;
+		if (copied == 0)
 			dirty_sectors = 0;
-		} else {
-			force_page_uptodate = false;
-		}
 
 		if (num_sectors > dirty_sectors) {
 			/* release everything except the sectors we dirtied */
-- 
2.49.0

[PATCH 2/4] btrfs: cleanup the reserved space inside the loop of btrfs_buffered_write()

[Qu Wenruo]

Inside the main loop of btrfs_buffered_write(), if something wrong
happened, there is a out-of-loop cleanup path to release the reserved
space.

This behavior saves some code lines, but makes it much harder to read,
as we need to check release_bytes to make sure when we need to do the
cleanup.

Extract the cleanup part into a helper, release_reserved_space(), to do
the cleanup inside the main loop, so that we can move @release_bytes
inside the loop.

This will make later refactor of the main loop much easier.

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index b7eb1f0164bb..f68846c14ed5 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
 	return 0;
 }
 
+static void release_space(struct btrfs_inode *inode,
+			  struct extent_changeset *data_reserved,
+			  u64 start, u64 len,
+			  bool only_release_metadata)
+{
+	const struct btrfs_fs_info *fs_info = inode->root->fs_info;
+
+	if (!len)
+		return;
+
+	if (only_release_metadata) {
+		btrfs_check_nocow_unlock(inode);
+		btrfs_delalloc_release_metadata(inode, len, true);
+	} else {
+		btrfs_delalloc_release_space(inode,
+				data_reserved,
+				round_down(start, fs_info->sectorsize),
+				len, true);
+	}
+}
+
 ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 {
 	struct file *file = iocb->ki_filp;
@@ -1081,7 +1102,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;
@@ -1090,7 +1110,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;
 
 	if (nowait)
 		ilock_flags |= BTRFS_ILOCK_TRY;
@@ -1125,7 +1144,9 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		size_t dirty_sectors;
 		size_t num_sectors;
 		struct folio *folio = NULL;
+		u64 release_bytes = 0;
 		int extents_locked;
+		bool only_release_metadata = false;
 
 		/*
 		 * Fault pages before locking them in prepare_one_folio()
@@ -1136,7 +1157,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
-		only_release_metadata = false;
 		sector_offset = pos & (fs_info->sectorsize - 1);
 
 		extent_changeset_release(data_reserved);
@@ -1191,6 +1211,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		ret = balance_dirty_pages_ratelimited_flags(inode->i_mapping, bdp_flags);
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
+			release_space(BTRFS_I(inode), data_reserved,
+				      pos, release_bytes, only_release_metadata);
 			break;
 		}
 
@@ -1198,6 +1220,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);
+			release_space(BTRFS_I(inode), data_reserved,
+				      pos, release_bytes, only_release_metadata);
 			break;
 		}
 
@@ -1210,6 +1234,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);
+			release_space(BTRFS_I(inode), data_reserved,
+				      pos, release_bytes, only_release_metadata);
 			ret = extents_locked;
 			break;
 		}
@@ -1277,6 +1303,8 @@ 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);
+			release_space(BTRFS_I(inode), data_reserved,
+				      pos, release_bytes, only_release_metadata);
 			break;
 		}
 
@@ -1292,19 +1320,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.49.0

[PATCH 3/4] btrfs: extract the space reservation code from btrfs_buffered_write()

[Qu Wenruo]

Inside the main loop of btrfs_buffered_write(), we have a complex data
and metadata space reservation code, which tries to reserve space for
a COW write, if failed then fallback to check if we can do a NOCOW
write.

Extract that part of code into a dedicated helper, reserve_space(), to
make the main loop a little easier to read.

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f68846c14ed5..99580ef906a6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1095,6 +1095,64 @@ static void release_space(struct btrfs_inode *inode,
 	}
 }
 
+/*
+ * Reserve data and metadata space for the this buffered write range.
+ *
+ * Return >0 for the number of bytes reserved, which is always block aligned.
+ * Return <0 for error.
+ */
+static ssize_t reserve_space(struct btrfs_inode *inode,
+			 struct extent_changeset **data_reserved,
+			 u64 start, size_t *len, bool nowait,
+			 bool *only_release_metadata)
+{
+	const struct btrfs_fs_info *fs_info = inode->root->fs_info;
+	unsigned int block_offset = start & (fs_info->sectorsize - 1);
+	size_t reserve_bytes;
+	int ret;
+
+	ret = btrfs_check_data_free_space(inode, data_reserved, start, *len,
+					  nowait);
+	if (ret < 0) {
+		int can_nocow;
+
+		if (nowait && (ret == -ENOSPC || ret == -EAGAIN))
+			return -EAGAIN;
+
+		/*
+		 * If we don't have to COW at the offset, reserve
+		 * metadata only. write_bytes may get smaller than
+		 * requested here.
+		 */
+		can_nocow = btrfs_check_nocow_lock(inode, start, len, nowait);
+		if (can_nocow < 0)
+			ret = can_nocow;
+		if (can_nocow > 0)
+			ret = 0;
+		if (ret)
+			return ret;
+		*only_release_metadata = true;
+	}
+
+	reserve_bytes = round_up(*len + block_offset,
+				 fs_info->sectorsize);
+	WARN_ON(reserve_bytes == 0);
+	ret = btrfs_delalloc_reserve_metadata(inode, reserve_bytes,
+					      reserve_bytes, nowait);
+	if (ret) {
+		if (!*only_release_metadata)
+			btrfs_free_reserved_data_space(inode,
+					*data_reserved, start, *len);
+		else
+			btrfs_check_nocow_unlock(inode);
+
+		if (nowait && ret == -ENOSPC)
+			ret = -EAGAIN;
+		return ret;
+	}
+	return reserve_bytes;
+}
+
 ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 {
 	struct file *file = iocb->ki_filp;
@@ -1160,52 +1218,12 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 		sector_offset = pos & (fs_info->sectorsize - 1);
 
 		extent_changeset_release(data_reserved);
-		ret = btrfs_check_data_free_space(BTRFS_I(inode),
-						  &data_reserved, pos,
-						  write_bytes, nowait);
-		if (ret < 0) {
-			int can_nocow;
-
-			if (nowait && (ret == -ENOSPC || ret == -EAGAIN)) {
-				ret = -EAGAIN;
-				break;
-			}
-
-			/*
-			 * If we don't have to COW at the offset, reserve
-			 * metadata only. write_bytes may get smaller than
-			 * requested here.
-			 */
-			can_nocow = btrfs_check_nocow_lock(BTRFS_I(inode), pos,
-							   &write_bytes, nowait);
-			if (can_nocow < 0)
-				ret = can_nocow;
-			if (can_nocow > 0)
-				ret = 0;
-			if (ret)
-				break;
-			only_release_metadata = true;
-		}
-
-		reserve_bytes = round_up(write_bytes + sector_offset,
-					 fs_info->sectorsize);
-		WARN_ON(reserve_bytes == 0);
-		ret = btrfs_delalloc_reserve_metadata(BTRFS_I(inode),
-						      reserve_bytes,
-						      reserve_bytes, nowait);
-		if (ret) {
-			if (!only_release_metadata)
-				btrfs_free_reserved_data_space(BTRFS_I(inode),
-						data_reserved, pos,
-						write_bytes);
-			else
-				btrfs_check_nocow_unlock(BTRFS_I(inode));
-
-			if (nowait && ret == -ENOSPC)
-				ret = -EAGAIN;
+		ret = reserve_space(BTRFS_I(inode), &data_reserved, pos,
+				    &write_bytes, nowait,
+				    &only_release_metadata);
+		if (ret < 0)
 			break;
-		}
-
+		reserve_bytes = ret;
 		release_bytes = reserve_bytes;
 again:
 		ret = balance_dirty_pages_ratelimited_flags(inode->i_mapping, bdp_flags);
-- 
2.49.0

[PATCH 4/4] btrfs: extract the main loop of btrfs_buffered_write() into a helper

[Qu Wenruo]

Inside the main loop of btrfs_buffered_write() we are doing a lot of
heavy lift work inside a while () loop.

This makes it pretty hard to read, extract the content into a helper,
copy_one_range() to do the heavy lift work.

This has no functional change, but with some minor variable renames,
e.g. rename all "sector" into "block".

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 99580ef906a6..21b90ed3e0e4 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1153,21 +1153,161 @@ static ssize_t reserve_space(struct btrfs_inode *inode,
 	return reserve_bytes;
 }
 
+/*
+ * Do the heavy lift work to copy one range into one folio of the page cache.
+ *
+ * Return >=0 for the number of bytes copied. (Return 0 means no byte is copied,
+ * caller should retry the same range again).
+ * Return <0 for error.
+ */
+static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *i,
+			  struct extent_changeset **data_reserved,
+			  u64 start, bool nowait)
+{
+	struct btrfs_fs_info *fs_info = inode->root->fs_info;
+	struct extent_state *cached_state = NULL;
+	const size_t block_offset = start & (fs_info->sectorsize - 1);
+	size_t write_bytes = min(iov_iter_count(i), PAGE_SIZE - offset_in_page(start));
+	size_t reserve_bytes;
+	size_t copied;
+	size_t dirty_blocks;
+	size_t num_blocks;
+	struct folio *folio = NULL;
+	u64 release_bytes = 0;
+	int extents_locked;
+	u64 lockstart;
+	u64 lockend;
+	bool only_release_metadata = false;
+	const unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
+	int ret;
+
+	/*
+	 * Fault pages before locking them in prepare_one_folio()
+	 * to avoid recursive lock
+	 */
+	if (unlikely(fault_in_iov_iter_readable(i, write_bytes)))
+		return -EFAULT;
+	extent_changeset_release(*data_reserved);
+	ret = reserve_space(inode, data_reserved, start, &write_bytes, nowait,
+			    &only_release_metadata);
+	if (ret < 0)
+		return ret;
+	reserve_bytes = ret;
+	release_bytes = reserve_bytes;
+
+again:
+	ret = balance_dirty_pages_ratelimited_flags(inode->vfs_inode.i_mapping,
+						    bdp_flags);
+	if (ret) {
+		btrfs_delalloc_release_extents(inode, reserve_bytes);
+		release_space(inode, *data_reserved, start, release_bytes,
+			      only_release_metadata);
+		return ret;
+	}
+
+	ret = prepare_one_folio(&inode->vfs_inode, &folio, start, write_bytes, false);
+	if (ret) {
+		btrfs_delalloc_release_extents(inode, reserve_bytes);
+		release_space(inode, *data_reserved, start, release_bytes,
+			      only_release_metadata);
+		return ret;
+	}
+	extents_locked = lock_and_cleanup_extent_if_need(inode,
+					folio, start, write_bytes, &lockstart,
+					&lockend, nowait, &cached_state);
+	if (extents_locked < 0) {
+		if (!nowait && extents_locked == -EAGAIN)
+			goto again;
+
+		btrfs_delalloc_release_extents(inode, reserve_bytes);
+		release_space(inode, *data_reserved, start, release_bytes,
+			      only_release_metadata);
+		ret = extents_locked;
+		return ret;
+	}
+
+	copied = copy_folio_from_iter_atomic(folio,
+			offset_in_folio(folio, start), write_bytes, i);
+	flush_dcache_folio(folio);
+
+	/*
+	 * If we get a partial write, we can end up with partially
+	 * uptodate page. Although if sector size < page size we can
+	 * handle it, but if it's not sector aligned it can cause
+	 * a lot of complexity, so make sure they don't happen by
+	 * forcing retry this copy.
+	 */
+	if (unlikely(copied < write_bytes)) {
+		if (!folio_test_uptodate(folio)) {
+			iov_iter_revert(i, copied);
+			copied = 0;
+		}
+	}
+
+	num_blocks = BTRFS_BYTES_TO_BLKS(fs_info, reserve_bytes);
+	dirty_blocks = round_up(copied + block_offset, fs_info->sectorsize);
+	dirty_blocks = BTRFS_BYTES_TO_BLKS(fs_info, dirty_blocks);
+
+	if (copied == 0)
+		dirty_blocks = 0;
+
+	if (num_blocks > dirty_blocks) {
+		/* release everything except the sectors we dirtied */
+		release_bytes -= dirty_blocks << fs_info->sectorsize_bits;
+		if (only_release_metadata) {
+			btrfs_delalloc_release_metadata(inode,
+						release_bytes, true);
+		} else {
+			u64 release_start = round_up(start + copied,
+						     fs_info->sectorsize);
+			btrfs_delalloc_release_space(inode,
+					*data_reserved, release_start,
+					release_bytes, true);
+		}
+	}
+	release_bytes = round_up(copied + block_offset, fs_info->sectorsize);
+
+	ret = btrfs_dirty_folio(inode, folio, start, copied,
+				&cached_state, only_release_metadata);
+	/*
+	 * If we have not locked the extent range, because the range's
+	 * start offset is >= i_size, we might still have a non-NULL
+	 * cached extent state, acquired while marking the extent range
+	 * as delalloc through btrfs_dirty_page(). Therefore free any
+	 * possible cached extent state to avoid a memory leak.
+	 */
+	if (extents_locked)
+		unlock_extent(&inode->io_tree, lockstart, lockend,
+			      &cached_state);
+	else
+		free_extent_state(cached_state);
+
+	btrfs_delalloc_release_extents(inode, reserve_bytes);
+	if (ret) {
+		btrfs_drop_folio(fs_info, folio, start, copied);
+		release_space(inode, *data_reserved, start, release_bytes,
+			      only_release_metadata);
+		return ret;
+	}
+	if (only_release_metadata)
+		btrfs_check_nocow_unlock(inode);
+
+	btrfs_drop_folio(fs_info, folio, start, copied);
+	cond_resched();
+	return copied;
+}
+
 ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 {
 	struct file *file = iocb->ki_filp;
 	loff_t pos;
 	struct inode *inode = file_inode(file);
-	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
 	struct extent_changeset *data_reserved = NULL;
-	u64 lockstart;
-	u64 lockend;
 	size_t num_written = 0;
 	ssize_t ret;
 	loff_t old_isize;
 	unsigned int ilock_flags = 0;
 	const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
-	unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
 
 	if (nowait)
 		ilock_flags |= BTRFS_ILOCK_TRY;
@@ -1193,149 +1333,11 @@ 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 reserve_bytes;
-		size_t copied;
-		size_t dirty_sectors;
-		size_t num_sectors;
-		struct folio *folio = NULL;
-		u64 release_bytes = 0;
-		int extents_locked;
-		bool only_release_metadata = false;
-
-		/*
-		 * Fault pages before locking them in prepare_one_folio()
-		 * to avoid recursive lock
-		 */
-		if (unlikely(fault_in_iov_iter_readable(i, write_bytes))) {
-			ret = -EFAULT;
-			break;
-		}
-
-		sector_offset = pos & (fs_info->sectorsize - 1);
-
-		extent_changeset_release(data_reserved);
-		ret = reserve_space(BTRFS_I(inode), &data_reserved, pos,
-				    &write_bytes, nowait,
-				    &only_release_metadata);
+		ret = copy_one_range(BTRFS_I(inode), i, &data_reserved, pos, nowait);
 		if (ret < 0)
 			break;
-		reserve_bytes = ret;
-		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);
-			release_space(BTRFS_I(inode), data_reserved,
-				      pos, release_bytes, only_release_metadata);
-			break;
-		}
-
-		ret = prepare_one_folio(inode, &folio, pos, write_bytes, false);
-		if (ret) {
-			btrfs_delalloc_release_extents(BTRFS_I(inode),
-						       reserve_bytes);
-			release_space(BTRFS_I(inode), data_reserved,
-				      pos, release_bytes, only_release_metadata);
-			break;
-		}
-
-		extents_locked = lock_and_cleanup_extent_if_need(BTRFS_I(inode),
-						folio, pos, write_bytes, &lockstart,
-						&lockend, nowait, &cached_state);
-		if (extents_locked < 0) {
-			if (!nowait && extents_locked == -EAGAIN)
-				goto again;
-
-			btrfs_delalloc_release_extents(BTRFS_I(inode),
-						       reserve_bytes);
-			release_space(BTRFS_I(inode), data_reserved,
-				      pos, release_bytes, only_release_metadata);
-			ret = extents_locked;
-			break;
-		}
-
-		copied = copy_folio_from_iter_atomic(folio,
-				offset_in_folio(folio, pos), write_bytes, i);
-		flush_dcache_folio(folio);
-
-		/*
-		 * If we get a partial write, we can end up with partially
-		 * uptodate page. Although if sector size < page size we can
-		 * handle it, but if it's not sector aligned it can cause
-		 * a lot of complexity, so make sure they don't happen by
-		 * forcing retry this copy.
-		 */
-		if (unlikely(copied < write_bytes)) {
-			if (!folio_test_uptodate(folio)) {
-				iov_iter_revert(i, copied);
-				copied = 0;
-			}
-		}
-
-		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)
-			dirty_sectors = 0;
-
-		if (num_sectors > dirty_sectors) {
-			/* 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);
-			} 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);
-			}
-		}
-
-		release_bytes = round_up(copied + sector_offset,
-					fs_info->sectorsize);
-
-		ret = btrfs_dirty_folio(BTRFS_I(inode), folio, pos, copied,
-					&cached_state, only_release_metadata);
-
-		/*
-		 * If we have not locked the extent range, because the range's
-		 * start offset is >= i_size, we might still have a non-NULL
-		 * cached extent state, acquired while marking the extent range
-		 * as delalloc through btrfs_dirty_page(). Therefore free any
-		 * possible cached extent state to avoid a memory leak.
-		 */
-		if (extents_locked)
-			unlock_extent(&BTRFS_I(inode)->io_tree, lockstart,
-				      lockend, &cached_state);
-		else
-			free_extent_state(cached_state);
-
-		btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
-		if (ret) {
-			btrfs_drop_folio(fs_info, folio, pos, copied);
-			release_space(BTRFS_I(inode), data_reserved,
-				      pos, release_bytes, only_release_metadata);
-			break;
-		}
-
-		release_bytes = 0;
-		if (only_release_metadata)
-			btrfs_check_nocow_unlock(BTRFS_I(inode));
-
-		btrfs_drop_folio(fs_info, folio, pos, copied);
-
-		cond_resched();
-
-		pos += copied;
-		num_written += copied;
+		pos += ret;
+		num_written += ret;
 	}
 
 	extent_changeset_free(data_reserved);
-- 
2.49.0

Re: [PATCH 1/4] btrfs: remove force_page_uptodate variable from btrfs_buffered_write()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/4] btrfs: cleanup the reserved space inside the loop of btrfs_buffered_write()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/4] btrfs: extract the space reservation code from btrfs_buffered_write()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/4] btrfs: extract the main loop of btrfs_buffered_write() into a helper

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/4] btrfs: remove force_page_uptodate variable from btrfs_buffered_write()

[Filipe Manana]

On Thu, Mar 20, 2025 at 5:36 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Commit c87c299776e4 ("btrfs: make buffered write to copy one page a
> time") changed how the variable @force_page_uptodate is updated.
>
> Before that commit the variable is only initialized to false at the
> beginning of the function, and after hitting a short copy, the next
> retry on the same folio will force the foilio to be read from the disk.

foilio -> folio

>
> But after the commit, the variable is always updated to false for each

I think saying "is always initialized to false at the beginning of the
loop's scope" is more clear.
When you say updated it gives the idea it was declared in an outer
scope, but that's not the case anymore.

> iteration, causing prepare_one_folio() never to get a true value passed
> in.
>
> The change in behavior is not a huge deal, it only makes a difference
> on how we handle short copies:
>
> Old: Allow the buffer to be split
>
>      The first short copy will be rejected, that's the same for both
>      cases.
>
>      But for the next retry, we require the folio to be read from disk.
>
>      Then even if we hit a short copy again, since the folio is already
>      uptodate, we do not need to handle partial uptodate range, and can
>      continue, marking the short copied range as dirty and continue.
>
>      This will split the buffer write into the folio as two buffered
>      writes.
>
> New: Do not allow the buffer to be split
>
>      The first short copy will be rejected, that's the same for both
>      cases.
>
>      For the next retry, we do nothing special, thus if the short copy
>      happened again, we reject it again, until either the short copy is
>      gone, or we failed to fault in the buffer.
>
>      This will mean the buffer write into the folio will either fail or
>      success, no split will happen.
>
> To me, either solution is fine, but the newer one makes it simpler and
> requires no special handling, so I prefer that solution.

Ok so this explanation of different behaviour is something that should
have been in the change log of commit c87c299776e4 ("btrfs: make
buffered write to copy one page a time").

With the new behaviour, when folios larger than 1 page are supported,
I wonder if we don't risk looping over the same subrange many times,
in case we keep needing to faultin due to memory pressure.

>
> And since @force_page_uptodate is always false when passed into
> prepare_one_folio(), we can just remove the variable.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Anyway, this change looks good, and at least with the typo fixed:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> ---
>  fs/btrfs/file.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index c2648858772a..b7eb1f0164bb 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -800,7 +800,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>   * On success return a locked folio and 0
>   */
>  static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64 pos,
> -                                 u64 len, bool force_uptodate)
> +                                 u64 len)
>  {
>         u64 clamp_start = max_t(u64, pos, folio_pos(folio));
>         u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
> @@ -810,8 +810,7 @@ static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64
>         if (folio_test_uptodate(folio))
>                 return 0;
>
> -       if (!force_uptodate &&
> -           IS_ALIGNED(clamp_start, blocksize) &&
> +       if (IS_ALIGNED(clamp_start, blocksize) &&
>             IS_ALIGNED(clamp_end, blocksize))
>                 return 0;
>
> @@ -858,7 +857,7 @@ static gfp_t get_prepare_gfp_flags(struct inode *inode, bool nowait)
>   */
>  static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_ret,
>                                       loff_t pos, size_t write_bytes,
> -                                     bool force_uptodate, bool nowait)
> +                                     bool nowait)
>  {
>         unsigned long index = pos >> PAGE_SHIFT;
>         gfp_t mask = get_prepare_gfp_flags(inode, nowait);
> @@ -881,7 +880,7 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
>                 folio_put(folio);
>                 return ret;
>         }
> -       ret = prepare_uptodate_folio(inode, folio, pos, write_bytes, force_uptodate);
> +       ret = prepare_uptodate_folio(inode, folio, pos, write_bytes);
>         if (ret) {
>                 /* The folio is already unlocked. */
>                 folio_put(folio);
> @@ -1127,7 +1126,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                 size_t num_sectors;
>                 struct folio *folio = NULL;
>                 int extents_locked;
> -               bool force_page_uptodate = false;
>
>                 /*
>                  * Fault pages before locking them in prepare_one_folio()
> @@ -1196,8 +1194,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                         break;
>                 }
>
> -               ret = prepare_one_folio(inode, &folio, pos, write_bytes,
> -                                       force_page_uptodate, false);
> +               ret = prepare_one_folio(inode, &folio, pos, write_bytes, false);
>                 if (ret) {
>                         btrfs_delalloc_release_extents(BTRFS_I(inode),
>                                                        reserve_bytes);
> @@ -1240,12 +1237,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                                         fs_info->sectorsize);
>                 dirty_sectors = BTRFS_BYTES_TO_BLKS(fs_info, dirty_sectors);
>
> -               if (copied == 0) {
> -                       force_page_uptodate = true;
> +               if (copied == 0)
>                         dirty_sectors = 0;
> -               } else {
> -                       force_page_uptodate = false;
> -               }
>
>                 if (num_sectors > dirty_sectors) {
>                         /* release everything except the sectors we dirtied */
> --
> 2.49.0
>
>

Re: [PATCH 2/4] btrfs: cleanup the reserved space inside the loop of btrfs_buffered_write()

[Filipe Manana]

On Thu, Mar 20, 2025 at 5:36 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Inside the main loop of btrfs_buffered_write(), if something wrong
> happened, there is a out-of-loop cleanup path to release the reserved
> space.
>
> This behavior saves some code lines, but makes it much harder to read,
> as we need to check release_bytes to make sure when we need to do the
> cleanup.
>
> Extract the cleanup part into a helper, release_reserved_space(), to do
> the cleanup inside the main loop, so that we can move @release_bytes
> inside the loop.
>
> This will make later refactor of the main loop much easier.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index b7eb1f0164bb..f68846c14ed5 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
>         return 0;
>  }
>
> +static void release_space(struct btrfs_inode *inode,
> +                         struct extent_changeset *data_reserved,
> +                         u64 start, u64 len,
> +                         bool only_release_metadata)
> +{
> +       const struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +
> +       if (!len)
> +               return;
> +
> +       if (only_release_metadata) {
> +               btrfs_check_nocow_unlock(inode);
> +               btrfs_delalloc_release_metadata(inode, len, true);
> +       } else {
> +               btrfs_delalloc_release_space(inode,
> +                               data_reserved,
> +                               round_down(start, fs_info->sectorsize),
> +                               len, true);
> +       }
> +}
> +
>  ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>  {
>         struct file *file = iocb->ki_filp;
> @@ -1081,7 +1102,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;
> @@ -1090,7 +1110,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;
>
>         if (nowait)
>                 ilock_flags |= BTRFS_ILOCK_TRY;
> @@ -1125,7 +1144,9 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                 size_t dirty_sectors;
>                 size_t num_sectors;
>                 struct folio *folio = NULL;
> +               u64 release_bytes = 0;

We don't actually need to initialize this anymore.

>                 int extents_locked;
> +               bool only_release_metadata = false;
>
>                 /*
>                  * Fault pages before locking them in prepare_one_folio()
> @@ -1136,7 +1157,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                         break;
>                 }
>
> -               only_release_metadata = false;
>                 sector_offset = pos & (fs_info->sectorsize - 1);
>
>                 extent_changeset_release(data_reserved);
> @@ -1191,6 +1211,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                 ret = balance_dirty_pages_ratelimited_flags(inode->i_mapping, bdp_flags);
>                 if (ret) {
>                         btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
> +                       release_space(BTRFS_I(inode), data_reserved,
> +                                     pos, release_bytes, only_release_metadata);
>                         break;
>                 }
>
> @@ -1198,6 +1220,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                 if (ret) {
>                         btrfs_delalloc_release_extents(BTRFS_I(inode),
>                                                        reserve_bytes);
> +                       release_space(BTRFS_I(inode), data_reserved,
> +                                     pos, release_bytes, only_release_metadata);
>                         break;
>                 }
>
> @@ -1210,6 +1234,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>
>                         btrfs_delalloc_release_extents(BTRFS_I(inode),
>                                                        reserve_bytes);
> +                       release_space(BTRFS_I(inode), data_reserved,
> +                                     pos, release_bytes, only_release_metadata);
>                         ret = extents_locked;
>                         break;
>                 }
> @@ -1277,6 +1303,8 @@ 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);
> +                       release_space(BTRFS_I(inode), data_reserved,
> +                                     pos, release_bytes, only_release_metadata);
>                         break;
>                 }

After this, we also don't need to set 'release_bytes' to 0 anymore either.

Anyway, these are small things, so:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

>
> @@ -1292,19 +1320,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.49.0
>
>

Re: [PATCH 3/4] btrfs: extract the space reservation code from btrfs_buffered_write()

[Filipe Manana]

On Thu, Mar 20, 2025 at 5:37 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Inside the main loop of btrfs_buffered_write(), we have a complex data
> and metadata space reservation code, which tries to reserve space for
> a COW write, if failed then fallback to check if we can do a NOCOW
> write.
>
> Extract that part of code into a dedicated helper, reserve_space(), to
> make the main loop a little easier to read.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c | 108 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 63 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index f68846c14ed5..99580ef906a6 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1095,6 +1095,64 @@ static void release_space(struct btrfs_inode *inode,
>         }
>  }
>
> +/*
> + * Reserve data and metadata space for the this buffered write range.

"for the this" -> for this

> + *
> + * Return >0 for the number of bytes reserved, which is always block aligned.
> + * Return <0 for error.
> + */
> +static ssize_t reserve_space(struct btrfs_inode *inode,
> +                        struct extent_changeset **data_reserved,
> +                        u64 start, size_t *len, bool nowait,
> +                        bool *only_release_metadata)
> +{
> +       const struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +       unsigned int block_offset = start & (fs_info->sectorsize - 1);

block_offset can also be const.

> +       size_t reserve_bytes;
> +       int ret;
> +
> +       ret = btrfs_check_data_free_space(inode, data_reserved, start, *len,
> +                                         nowait);
> +       if (ret < 0) {
> +               int can_nocow;
> +
> +               if (nowait && (ret == -ENOSPC || ret == -EAGAIN))
> +                       return -EAGAIN;
> +
> +               /*
> +                * If we don't have to COW at the offset, reserve
> +                * metadata only. write_bytes may get smaller than
> +                * requested here.

While here, we can make the line width closer to the 80 characters
limit, as these are a bit too short.

> +                */
> +               can_nocow = btrfs_check_nocow_lock(inode, start, len, nowait);
> +               if (can_nocow < 0)
> +                       ret = can_nocow;
> +               if (can_nocow > 0)
> +                       ret = 0;
> +               if (ret)
> +                       return ret;
> +               *only_release_metadata = true;
> +       }
> +
> +       reserve_bytes = round_up(*len + block_offset,
> +                                fs_info->sectorsize);

There's no need for line splitting here, it all fits within 75 characters.

Otherwise it looks good, and with these minor changes:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +       WARN_ON(reserve_bytes == 0);
> +       ret = btrfs_delalloc_reserve_metadata(inode, reserve_bytes,
> +                                             reserve_bytes, nowait);
> +       if (ret) {
> +               if (!*only_release_metadata)
> +                       btrfs_free_reserved_data_space(inode,
> +                                       *data_reserved, start, *len);
> +               else
> +                       btrfs_check_nocow_unlock(inode);
> +
> +               if (nowait && ret == -ENOSPC)
> +                       ret = -EAGAIN;
> +               return ret;
> +       }
> +       return reserve_bytes;
> +}
> +
>  ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>  {
>         struct file *file = iocb->ki_filp;
> @@ -1160,52 +1218,12 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>                 sector_offset = pos & (fs_info->sectorsize - 1);
>
>                 extent_changeset_release(data_reserved);
> -               ret = btrfs_check_data_free_space(BTRFS_I(inode),
> -                                                 &data_reserved, pos,
> -                                                 write_bytes, nowait);
> -               if (ret < 0) {
> -                       int can_nocow;
> -
> -                       if (nowait && (ret == -ENOSPC || ret == -EAGAIN)) {
> -                               ret = -EAGAIN;
> -                               break;
> -                       }
> -
> -                       /*
> -                        * If we don't have to COW at the offset, reserve
> -                        * metadata only. write_bytes may get smaller than
> -                        * requested here.
> -                        */
> -                       can_nocow = btrfs_check_nocow_lock(BTRFS_I(inode), pos,
> -                                                          &write_bytes, nowait);
> -                       if (can_nocow < 0)
> -                               ret = can_nocow;
> -                       if (can_nocow > 0)
> -                               ret = 0;
> -                       if (ret)
> -                               break;
> -                       only_release_metadata = true;
> -               }
> -
> -               reserve_bytes = round_up(write_bytes + sector_offset,
> -                                        fs_info->sectorsize);
> -               WARN_ON(reserve_bytes == 0);
> -               ret = btrfs_delalloc_reserve_metadata(BTRFS_I(inode),
> -                                                     reserve_bytes,
> -                                                     reserve_bytes, nowait);
> -               if (ret) {
> -                       if (!only_release_metadata)
> -                               btrfs_free_reserved_data_space(BTRFS_I(inode),
> -                                               data_reserved, pos,
> -                                               write_bytes);
> -                       else
> -                               btrfs_check_nocow_unlock(BTRFS_I(inode));
> -
> -                       if (nowait && ret == -ENOSPC)
> -                               ret = -EAGAIN;
> +               ret = reserve_space(BTRFS_I(inode), &data_reserved, pos,
> +                                   &write_bytes, nowait,
> +                                   &only_release_metadata);
> +               if (ret < 0)
>                         break;
> -               }
> -
> +               reserve_bytes = ret;
>                 release_bytes = reserve_bytes;
>  again:
>                 ret = balance_dirty_pages_ratelimited_flags(inode->i_mapping, bdp_flags);
> --
> 2.49.0
>
>

Re: [PATCH 4/4] btrfs: extract the main loop of btrfs_buffered_write() into a helper

[Filipe Manana]

On Thu, Mar 20, 2025 at 5:35 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Inside the main loop of btrfs_buffered_write() we are doing a lot of
> heavy lift work inside a while () loop.
>
> This makes it pretty hard to read, extract the content into a helper,
> copy_one_range() to do the heavy lift work.
>
> This has no functional change, but with some minor variable renames,
> e.g. rename all "sector" into "block".
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c | 292 ++++++++++++++++++++++++------------------------
>  1 file changed, 147 insertions(+), 145 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 99580ef906a6..21b90ed3e0e4 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1153,21 +1153,161 @@ static ssize_t reserve_space(struct btrfs_inode *inode,
>         return reserve_bytes;
>  }
>
> +/*
> + * Do the heavy lift work to copy one range into one folio of the page cache.
> + *
> + * Return >=0 for the number of bytes copied. (Return 0 means no byte is copied,
> + * caller should retry the same range again).

Return > 0 in case we copied all bytes or just some of them.
Return 0 if no bytes were copied, in which case the caller should retry.
Return < 0 on error.

> + * Return <0 for error.

Using space both before and after the comparison operator makes it
easier to the eyes and consistent with the code style.
I.e.  "< 0" instead of "<0".

> + */
> +static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *i,
> +                         struct extent_changeset **data_reserved,
> +                         u64 start, bool nowait)
> +{
> +       struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +       struct extent_state *cached_state = NULL;
> +       const size_t block_offset = start & (fs_info->sectorsize - 1);
> +       size_t write_bytes = min(iov_iter_count(i), PAGE_SIZE - offset_in_page(start));
> +       size_t reserve_bytes;
> +       size_t copied;
> +       size_t dirty_blocks;
> +       size_t num_blocks;
> +       struct folio *folio = NULL;
> +       u64 release_bytes = 0;
> +       int extents_locked;
> +       u64 lockstart;
> +       u64 lockend;
> +       bool only_release_metadata = false;
> +       const unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
> +       int ret;
> +
> +       /*
> +        * Fault pages before locking them in prepare_one_folio()
> +        * to avoid recursive lock
> +        */

Since we are moving this comment, we can take the chance to make the
line length much closer to the 80 character width limit.
Also finish the sentence with punctuation to make it consistent with
the style we prefer.

> +       if (unlikely(fault_in_iov_iter_readable(i, write_bytes)))
> +               return -EFAULT;
> +       extent_changeset_release(*data_reserved);
> +       ret = reserve_space(inode, data_reserved, start, &write_bytes, nowait,
> +                           &only_release_metadata);
> +       if (ret < 0)
> +               return ret;
> +       reserve_bytes = ret;
> +       release_bytes = reserve_bytes;
> +
> +again:
> +       ret = balance_dirty_pages_ratelimited_flags(inode->vfs_inode.i_mapping,
> +                                                   bdp_flags);
> +       if (ret) {
> +               btrfs_delalloc_release_extents(inode, reserve_bytes);
> +               release_space(inode, *data_reserved, start, release_bytes,
> +                             only_release_metadata);
> +               return ret;
> +       }
> +
> +       ret = prepare_one_folio(&inode->vfs_inode, &folio, start, write_bytes, false);
> +       if (ret) {
> +               btrfs_delalloc_release_extents(inode, reserve_bytes);
> +               release_space(inode, *data_reserved, start, release_bytes,
> +                             only_release_metadata);
> +               return ret;
> +       }
> +       extents_locked = lock_and_cleanup_extent_if_need(inode,
> +                                       folio, start, write_bytes, &lockstart,
> +                                       &lockend, nowait, &cached_state);
> +       if (extents_locked < 0) {
> +               if (!nowait && extents_locked == -EAGAIN)
> +                       goto again;
> +
> +               btrfs_delalloc_release_extents(inode, reserve_bytes);
> +               release_space(inode, *data_reserved, start, release_bytes,
> +                             only_release_metadata);
> +               ret = extents_locked;
> +               return ret;
> +       }
> +
> +       copied = copy_folio_from_iter_atomic(folio,
> +                       offset_in_folio(folio, start), write_bytes, i);
> +       flush_dcache_folio(folio);
> +
> +       /*
> +        * If we get a partial write, we can end up with partially
> +        * uptodate page. Although if sector size < page size we can
> +        * handle it, but if it's not sector aligned it can cause
> +        * a lot of complexity, so make sure they don't happen by
> +        * forcing retry this copy.
> +        */

Same here, can make the width closer to 80 characters.

> +       if (unlikely(copied < write_bytes)) {
> +               if (!folio_test_uptodate(folio)) {
> +                       iov_iter_revert(i, copied);
> +                       copied = 0;
> +               }
> +       }
> +
> +       num_blocks = BTRFS_BYTES_TO_BLKS(fs_info, reserve_bytes);
> +       dirty_blocks = round_up(copied + block_offset, fs_info->sectorsize);
> +       dirty_blocks = BTRFS_BYTES_TO_BLKS(fs_info, dirty_blocks);
> +
> +       if (copied == 0)
> +               dirty_blocks = 0;
> +
> +       if (num_blocks > dirty_blocks) {
> +               /* release everything except the sectors we dirtied */

Make the first letter capitalized and end with punctuation.

> +               release_bytes -= dirty_blocks << fs_info->sectorsize_bits;
> +               if (only_release_metadata) {
> +                       btrfs_delalloc_release_metadata(inode,
> +                                               release_bytes, true);
> +               } else {
> +                       u64 release_start = round_up(start + copied,
> +                                                    fs_info->sectorsize);

Also add a blank line to separate variable declaration from code.
This is the style we follow nowadays.
Could also be made const.

> +                       btrfs_delalloc_release_space(inode,
> +                                       *data_reserved, release_start,
> +                                       release_bytes, true);
> +               }
> +       }
> +       release_bytes = round_up(copied + block_offset, fs_info->sectorsize);
> +
> +       ret = btrfs_dirty_folio(inode, folio, start, copied,
> +                               &cached_state, only_release_metadata);
> +       /*
> +        * If we have not locked the extent range, because the range's
> +        * start offset is >= i_size, we might still have a non-NULL
> +        * cached extent state, acquired while marking the extent range
> +        * as delalloc through btrfs_dirty_page(). Therefore free any
> +        * possible cached extent state to avoid a memory leak.
> +        */
> +       if (extents_locked)
> +               unlock_extent(&inode->io_tree, lockstart, lockend,
> +                             &cached_state);
> +       else
> +               free_extent_state(cached_state);
> +
> +       btrfs_delalloc_release_extents(inode, reserve_bytes);
> +       if (ret) {
> +               btrfs_drop_folio(fs_info, folio, start, copied);
> +               release_space(inode, *data_reserved, start, release_bytes,
> +                             only_release_metadata);
> +               return ret;
> +       }
> +       if (only_release_metadata)
> +               btrfs_check_nocow_unlock(inode);
> +
> +       btrfs_drop_folio(fs_info, folio, start, copied);
> +       cond_resched();

It doesn't make sense to have the cond_resched() in this function - it
should be inside the while loop of the caller.

With these small things addressed:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +       return copied;
> +}
> +
>  ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>  {
>         struct file *file = iocb->ki_filp;
>         loff_t pos;
>         struct inode *inode = file_inode(file);
> -       struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
>         struct extent_changeset *data_reserved = NULL;
> -       u64 lockstart;
> -       u64 lockend;
>         size_t num_written = 0;
>         ssize_t ret;
>         loff_t old_isize;
>         unsigned int ilock_flags = 0;
>         const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
> -       unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
>
>         if (nowait)
>                 ilock_flags |= BTRFS_ILOCK_TRY;
> @@ -1193,149 +1333,11 @@ 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 reserve_bytes;
> -               size_t copied;
> -               size_t dirty_sectors;
> -               size_t num_sectors;
> -               struct folio *folio = NULL;
> -               u64 release_bytes = 0;
> -               int extents_locked;
> -               bool only_release_metadata = false;
> -
> -               /*
> -                * Fault pages before locking them in prepare_one_folio()
> -                * to avoid recursive lock
> -                */
> -               if (unlikely(fault_in_iov_iter_readable(i, write_bytes))) {
> -                       ret = -EFAULT;
> -                       break;
> -               }
> -
> -               sector_offset = pos & (fs_info->sectorsize - 1);
> -
> -               extent_changeset_release(data_reserved);
> -               ret = reserve_space(BTRFS_I(inode), &data_reserved, pos,
> -                                   &write_bytes, nowait,
> -                                   &only_release_metadata);
> +               ret = copy_one_range(BTRFS_I(inode), i, &data_reserved, pos, nowait);
>                 if (ret < 0)
>                         break;
> -               reserve_bytes = ret;
> -               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);
> -                       release_space(BTRFS_I(inode), data_reserved,
> -                                     pos, release_bytes, only_release_metadata);
> -                       break;
> -               }
> -
> -               ret = prepare_one_folio(inode, &folio, pos, write_bytes, false);
> -               if (ret) {
> -                       btrfs_delalloc_release_extents(BTRFS_I(inode),
> -                                                      reserve_bytes);
> -                       release_space(BTRFS_I(inode), data_reserved,
> -                                     pos, release_bytes, only_release_metadata);
> -                       break;
> -               }
> -
> -               extents_locked = lock_and_cleanup_extent_if_need(BTRFS_I(inode),
> -                                               folio, pos, write_bytes, &lockstart,
> -                                               &lockend, nowait, &cached_state);
> -               if (extents_locked < 0) {
> -                       if (!nowait && extents_locked == -EAGAIN)
> -                               goto again;
> -
> -                       btrfs_delalloc_release_extents(BTRFS_I(inode),
> -                                                      reserve_bytes);
> -                       release_space(BTRFS_I(inode), data_reserved,
> -                                     pos, release_bytes, only_release_metadata);
> -                       ret = extents_locked;
> -                       break;
> -               }
> -
> -               copied = copy_folio_from_iter_atomic(folio,
> -                               offset_in_folio(folio, pos), write_bytes, i);
> -               flush_dcache_folio(folio);
> -
> -               /*
> -                * If we get a partial write, we can end up with partially
> -                * uptodate page. Although if sector size < page size we can
> -                * handle it, but if it's not sector aligned it can cause
> -                * a lot of complexity, so make sure they don't happen by
> -                * forcing retry this copy.
> -                */
> -               if (unlikely(copied < write_bytes)) {
> -                       if (!folio_test_uptodate(folio)) {
> -                               iov_iter_revert(i, copied);
> -                               copied = 0;
> -                       }
> -               }
> -
> -               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)
> -                       dirty_sectors = 0;
> -
> -               if (num_sectors > dirty_sectors) {
> -                       /* 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);
> -                       } 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);
> -                       }
> -               }
> -
> -               release_bytes = round_up(copied + sector_offset,
> -                                       fs_info->sectorsize);
> -
> -               ret = btrfs_dirty_folio(BTRFS_I(inode), folio, pos, copied,
> -                                       &cached_state, only_release_metadata);
> -
> -               /*
> -                * If we have not locked the extent range, because the range's
> -                * start offset is >= i_size, we might still have a non-NULL
> -                * cached extent state, acquired while marking the extent range
> -                * as delalloc through btrfs_dirty_page(). Therefore free any
> -                * possible cached extent state to avoid a memory leak.
> -                */
> -               if (extents_locked)
> -                       unlock_extent(&BTRFS_I(inode)->io_tree, lockstart,
> -                                     lockend, &cached_state);
> -               else
> -                       free_extent_state(cached_state);
> -
> -               btrfs_delalloc_release_extents(BTRFS_I(inode), reserve_bytes);
> -               if (ret) {
> -                       btrfs_drop_folio(fs_info, folio, pos, copied);
> -                       release_space(BTRFS_I(inode), data_reserved,
> -                                     pos, release_bytes, only_release_metadata);
> -                       break;
> -               }
> -
> -               release_bytes = 0;
> -               if (only_release_metadata)
> -                       btrfs_check_nocow_unlock(BTRFS_I(inode));
> -
> -               btrfs_drop_folio(fs_info, folio, pos, copied);
> -
> -               cond_resched();
> -
> -               pos += copied;
> -               num_written += copied;
> +               pos += ret;
> +               num_written += ret;
>         }
>
>         extent_changeset_free(data_reserved);
> --
> 2.49.0
>
>

Re: [PATCH 1/4] btrfs: remove force_page_uptodate variable from btrfs_buffered_write()

[Qu Wenruo]

在 2025/3/21 22:30, Filipe Manana 写道:
> On Thu, Mar 20, 2025 at 5:36 AM Qu Wenruo <wqu@suse.com> wrote:
[...]
>>
>> To me, either solution is fine, but the newer one makes it simpler and
>> requires no special handling, so I prefer that solution.
> 
> Ok so this explanation of different behaviour is something that should
> have been in the change log of commit c87c299776e4 ("btrfs: make
> buffered write to copy one page a time").

Yeah, that's in fact an unexpected behavior change, or you can call it a 
bug in this case.

> 
> With the new behaviour, when folios larger than 1 page are supported,
> I wonder if we don't risk looping over the same subrange many times,
> in case we keep needing to faultin due to memory pressure.

There is a small save there, that when we fault in the iov_iter, any 
bytes not faulted in will immediately trigger an -EFAULT error.

So in that case, we should hit -EFAULT more, other than looping over the 
same range again and again.

Although this may be a problem for future iomap migration, as iomap only 
requires to fault in part of the iov_iter.

Thanks,
Qu

> 
>>
>> And since @force_page_uptodate is always false when passed into
>> prepare_one_folio(), we can just remove the variable.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Anyway, this change looks good, and at least with the typo fixed:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
>> ---
>>   fs/btrfs/file.c | 19 ++++++-------------
>>   1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index c2648858772a..b7eb1f0164bb 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -800,7 +800,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>>    * On success return a locked folio and 0
>>    */
>>   static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64 pos,
>> -                                 u64 len, bool force_uptodate)
>> +                                 u64 len)
>>   {
>>          u64 clamp_start = max_t(u64, pos, folio_pos(folio));
>>          u64 clamp_end = min_t(u64, pos + len, folio_pos(folio) + folio_size(folio));
>> @@ -810,8 +810,7 @@ static int prepare_uptodate_folio(struct inode *inode, struct folio *folio, u64
>>          if (folio_test_uptodate(folio))
>>                  return 0;
>>
>> -       if (!force_uptodate &&
>> -           IS_ALIGNED(clamp_start, blocksize) &&
>> +       if (IS_ALIGNED(clamp_start, blocksize) &&
>>              IS_ALIGNED(clamp_end, blocksize))
>>                  return 0;
>>
>> @@ -858,7 +857,7 @@ static gfp_t get_prepare_gfp_flags(struct inode *inode, bool nowait)
>>    */
>>   static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_ret,
>>                                        loff_t pos, size_t write_bytes,
>> -                                     bool force_uptodate, bool nowait)
>> +                                     bool nowait)
>>   {
>>          unsigned long index = pos >> PAGE_SHIFT;
>>          gfp_t mask = get_prepare_gfp_flags(inode, nowait);
>> @@ -881,7 +880,7 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
>>                  folio_put(folio);
>>                  return ret;
>>          }
>> -       ret = prepare_uptodate_folio(inode, folio, pos, write_bytes, force_uptodate);
>> +       ret = prepare_uptodate_folio(inode, folio, pos, write_bytes);
>>          if (ret) {
>>                  /* The folio is already unlocked. */
>>                  folio_put(folio);
>> @@ -1127,7 +1126,6 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>>                  size_t num_sectors;
>>                  struct folio *folio = NULL;
>>                  int extents_locked;
>> -               bool force_page_uptodate = false;
>>
>>                  /*
>>                   * Fault pages before locking them in prepare_one_folio()
>> @@ -1196,8 +1194,7 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>>                          break;
>>                  }
>>
>> -               ret = prepare_one_folio(inode, &folio, pos, write_bytes,
>> -                                       force_page_uptodate, false);
>> +               ret = prepare_one_folio(inode, &folio, pos, write_bytes, false);
>>                  if (ret) {
>>                          btrfs_delalloc_release_extents(BTRFS_I(inode),
>>                                                         reserve_bytes);
>> @@ -1240,12 +1237,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
>>                                          fs_info->sectorsize);
>>                  dirty_sectors = BTRFS_BYTES_TO_BLKS(fs_info, dirty_sectors);
>>
>> -               if (copied == 0) {
>> -                       force_page_uptodate = true;
>> +               if (copied == 0)
>>                          dirty_sectors = 0;
>> -               } else {
>> -                       force_page_uptodate = false;
>> -               }
>>
>>                  if (num_sectors > dirty_sectors) {
>>                          /* release everything except the sectors we dirtied */
>> --
>> 2.49.0
>>
>>

Re: [PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[David Sterba]

On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote:
> The function btrfs_buffered_write() is implementing all the complex
> heavy-lifting work inside a huge while () loop, which makes later large
> data folios work much harder.
> 
> The first patch is a patch that already submitted to the mailing list
> recently, but all later reworks depends on that patch, thus it is
> included in the series.
> 
> The core of the whole series is to introduce a helper function,
> copy_one_range() to do the buffer copy into one single folio.
> 
> Patch 2 is a preparation that moves the error cleanup into the main loop,
> so we do not have dedicated out-of-loop cleanup.
> 
> Patch 3 is another preparation that extract the space reservation code
> into a helper, make the final refactor patch a little more smaller.
> 
> And patch 4 is the main dish, with all the refactoring happening inside
> it.
> 
> Qu Wenruo (4):
>   btrfs: remove force_page_uptodate variable from btrfs_buffered_write()
>   btrfs: cleanup the reserved space inside the loop of
>     btrfs_buffered_write()
>   btrfs: extract the space reservation code from btrfs_buffered_write()
>   btrfs: extract the main loop of btrfs_buffered_write() into a helper

I'm looking at the committed patches in for-next and there are still too
many whitespace and formatting issues, atop those pointed out in the
mail discussion. It's probably because the code moved and inherited the
formatting but this is one of the oportunities to fix it in the final
version.

I fixed what I saw, but plase try to reformat the code according to the
best pratices. No big deal if something slips, I'd rather you focus on
the code than on formattig but in this patchset it looked like a
systematic error.

In case of factoring out code and moving it around I suggest to do it in
two steps, first move the code, make sure it's correct etc, commit, and
then open the changed code in editor in diff mode. If you're using
fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code
and doing the styling and formatting pass is quite easy.

Re: [PATCH 4/4] btrfs: extract the main loop of btrfs_buffered_write() into a helper

[David Sterba]

On Thu, Mar 20, 2025 at 04:04:11PM +1030, Qu Wenruo wrote:
> Inside the main loop of btrfs_buffered_write() we are doing a lot of
> heavy lift work inside a while () loop.
> 
> This makes it pretty hard to read, extract the content into a helper,
> copy_one_range() to do the heavy lift work.
> 
> This has no functional change, but with some minor variable renames,
> e.g. rename all "sector" into "block".
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c | 292 ++++++++++++++++++++++++------------------------
>  1 file changed, 147 insertions(+), 145 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 99580ef906a6..21b90ed3e0e4 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1153,21 +1153,161 @@ static ssize_t reserve_space(struct btrfs_inode *inode,
>  	return reserve_bytes;
>  }
>  
> +/*
> + * Do the heavy lift work to copy one range into one folio of the page cache.
> + *
> + * Return >=0 for the number of bytes copied. (Return 0 means no byte is copied,
> + * caller should retry the same range again).
> + * Return <0 for error.
> + */
> +static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *i,

'struct iov_iter *i'  was probably copied from btrfs_buffered_write()
prototype bug uh naming 'i' such parameter is quite an anti-pattern.
I've renamed it to 'iter', and will fix the other one too so it does not
get accidentally copied somewhere else.

Re: [PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[Qu Wenruo]

在 2025/3/28 03:17, David Sterba 写道:
> On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote:
>> The function btrfs_buffered_write() is implementing all the complex
>> heavy-lifting work inside a huge while () loop, which makes later large
>> data folios work much harder.
>>
>> The first patch is a patch that already submitted to the mailing list
>> recently, but all later reworks depends on that patch, thus it is
>> included in the series.
>>
>> The core of the whole series is to introduce a helper function,
>> copy_one_range() to do the buffer copy into one single folio.
>>
>> Patch 2 is a preparation that moves the error cleanup into the main loop,
>> so we do not have dedicated out-of-loop cleanup.
>>
>> Patch 3 is another preparation that extract the space reservation code
>> into a helper, make the final refactor patch a little more smaller.
>>
>> And patch 4 is the main dish, with all the refactoring happening inside
>> it.
>>
>> Qu Wenruo (4):
>>    btrfs: remove force_page_uptodate variable from btrfs_buffered_write()
>>    btrfs: cleanup the reserved space inside the loop of
>>      btrfs_buffered_write()
>>    btrfs: extract the space reservation code from btrfs_buffered_write()
>>    btrfs: extract the main loop of btrfs_buffered_write() into a helper
>
> I'm looking at the committed patches in for-next and there are still too
> many whitespace and formatting issues, atop those pointed out in the
> mail discussion. It's probably because the code moved and inherited the
> formatting but this is one of the oportunities to fix it in the final
> version.
>
> I fixed what I saw, but plase try to reformat the code according to the
> best pratices. No big deal if something slips, I'd rather you focus on
> the code than on formattig but in this patchset it looked like a
> systematic error.
>
> In case of factoring out code and moving it around I suggest to do it in
> two steps, first move the code, make sure it's correct etc, commit, and
> then open the changed code in editor in diff mode. If you're using
> fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code
> and doing the styling and formatting pass is quite easy.
>
This is a little weird, IIRC the workflow hooks should detect those
whitespace errors at commit time, e.g:

$ git commit  -a --amend
ERROR:TRAILING_WHITESPACE: trailing whitespace
#9: FILE: fs/btrfs/file.c:2207:
+^I$

total: 1 errors, 0 warnings, 7 lines checked
Checkpatch found errors, would you like to fix them up? (Yn)

But it was never triggered at any of the code move.

I know I missed a lot of style changes when moving the code, but I
didn't expect any whitespace errors.

Mind to provide some examples where the git hooks didn't catch them?

Thanks,
Qu

Re: [PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[David Sterba]

On Fri, Mar 28, 2025 at 07:17:39AM +1030, Qu Wenruo wrote:
> 在 2025/3/28 03:17, David Sterba 写道:
> > On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote:
> >> The function btrfs_buffered_write() is implementing all the complex
> >> heavy-lifting work inside a huge while () loop, which makes later large
> >> data folios work much harder.
> >>
> >> The first patch is a patch that already submitted to the mailing list
> >> recently, but all later reworks depends on that patch, thus it is
> >> included in the series.
> >>
> >> The core of the whole series is to introduce a helper function,
> >> copy_one_range() to do the buffer copy into one single folio.
> >>
> >> Patch 2 is a preparation that moves the error cleanup into the main loop,
> >> so we do not have dedicated out-of-loop cleanup.
> >>
> >> Patch 3 is another preparation that extract the space reservation code
> >> into a helper, make the final refactor patch a little more smaller.
> >>
> >> And patch 4 is the main dish, with all the refactoring happening inside
> >> it.
> >>
> >> Qu Wenruo (4):
> >>    btrfs: remove force_page_uptodate variable from btrfs_buffered_write()
> >>    btrfs: cleanup the reserved space inside the loop of
> >>      btrfs_buffered_write()
> >>    btrfs: extract the space reservation code from btrfs_buffered_write()
> >>    btrfs: extract the main loop of btrfs_buffered_write() into a helper
> >
> > I'm looking at the committed patches in for-next and there are still too
> > many whitespace and formatting issues, atop those pointed out in the
> > mail discussion. It's probably because the code moved and inherited the
> > formatting but this is one of the oportunities to fix it in the final
> > version.
> >
> > I fixed what I saw, but plase try to reformat the code according to the
> > best pratices. No big deal if something slips, I'd rather you focus on
> > the code than on formattig but in this patchset it looked like a
> > systematic error.
> >
> > In case of factoring out code and moving it around I suggest to do it in
> > two steps, first move the code, make sure it's correct etc, commit, and
> > then open the changed code in editor in diff mode. If you're using
> > fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code
> > and doing the styling and formatting pass is quite easy.
> >
> This is a little weird, IIRC the workflow hooks should detect those
> whitespace errors at commit time, e.g:
> 
> $ git commit  -a --amend
> ERROR:TRAILING_WHITESPACE: trailing whitespace
> #9: FILE: fs/btrfs/file.c:2207:
> +^I$
> 
> total: 1 errors, 0 warnings, 7 lines checked
> Checkpatch found errors, would you like to fix them up? (Yn)
> 
> But it was never triggered at any of the code move.
> 
> I know I missed a lot of style changes when moving the code, but I
> didn't expect any whitespace errors.
> 
> Mind to provide some examples where the git hooks didn't catch them?

I don't use the git hooks for checks so I'll copy it from the patches:

https://lore.kernel.org/linux-btrfs/b0bd320dba85d72a34a4f7e5ba6b6c42caedbe41.1742443383.git.wqu@suse.com/

@@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
 	return 0;
 }
 
+static void release_space(struct btrfs_inode *inode,
+			  struct extent_changeset *data_reserved,
+			  u64 start, u64 len,
+			  bool only_release_metadata)
+{
+	const struct btrfs_fs_info *fs_info = inode->root->fs_info;
+
+	if (!len)
+		return;
+
+	if (only_release_metadata) {
+		btrfs_check_nocow_unlock(inode);
+		btrfs_delalloc_release_metadata(inode, len, true);
+	} else {
+		btrfs_delalloc_release_space(inode,
+				data_reserved,
+				round_down(start, fs_info->sectorsize),
+				len, true);
+	}
+}
---

The parameters of btrfs_delalloc_release_space(), matching its origin:

-	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);
-		}
-	}
---

Maybe the checks ignore possible whitespace adjustments in moved code,
e.g. comparing the - and + change literally and not taking the new
location into account.

Basically the same pattern in https://lore.kernel.org/linux-btrfs/522bfb923444f08b2c68c51a05cb5ca8b3ac7a77.1742443383.git.wqu@suse.com/

code moved to reserve_space() from btrfs_buffered_write(), parameters passed to
btrfs_free_reserved_data_space().

I remember fixing more code like this, but hopefully the two examples would be
sufficient to test the hook checks.

Re: [PATCH 0/4] btrfs: refactor btrfs_buffered_write() for the incoming large data folios

[Qu Wenruo]

在 2025/3/29 05:42, David Sterba 写道:
> On Fri, Mar 28, 2025 at 07:17:39AM +1030, Qu Wenruo wrote:
>> 在 2025/3/28 03:17, David Sterba 写道:
>>> On Thu, Mar 20, 2025 at 04:04:07PM +1030, Qu Wenruo wrote:
>>>> The function btrfs_buffered_write() is implementing all the complex
>>>> heavy-lifting work inside a huge while () loop, which makes later large
>>>> data folios work much harder.
>>>>
>>>> The first patch is a patch that already submitted to the mailing list
>>>> recently, but all later reworks depends on that patch, thus it is
>>>> included in the series.
>>>>
>>>> The core of the whole series is to introduce a helper function,
>>>> copy_one_range() to do the buffer copy into one single folio.
>>>>
>>>> Patch 2 is a preparation that moves the error cleanup into the main loop,
>>>> so we do not have dedicated out-of-loop cleanup.
>>>>
>>>> Patch 3 is another preparation that extract the space reservation code
>>>> into a helper, make the final refactor patch a little more smaller.
>>>>
>>>> And patch 4 is the main dish, with all the refactoring happening inside
>>>> it.
>>>>
>>>> Qu Wenruo (4):
>>>>     btrfs: remove force_page_uptodate variable from btrfs_buffered_write()
>>>>     btrfs: cleanup the reserved space inside the loop of
>>>>       btrfs_buffered_write()
>>>>     btrfs: extract the space reservation code from btrfs_buffered_write()
>>>>     btrfs: extract the main loop of btrfs_buffered_write() into a helper
>>>
>>> I'm looking at the committed patches in for-next and there are still too
>>> many whitespace and formatting issues, atop those pointed out in the
>>> mail discussion. It's probably because the code moved and inherited the
>>> formatting but this is one of the oportunities to fix it in the final
>>> version.
>>>
>>> I fixed what I saw, but plase try to reformat the code according to the
>>> best pratices. No big deal if something slips, I'd rather you focus on
>>> the code than on formattig but in this patchset it looked like a
>>> systematic error.
>>>
>>> In case of factoring out code and moving it around I suggest to do it in
>>> two steps, first move the code, make sure it's correct etc, commit, and
>>> then open the changed code in editor in diff mode. If you're using
>>> fugitive.vim the command ":Gdiff HEAD^" clearly shows the changed code
>>> and doing the styling and formatting pass is quite easy.
>>>
>> This is a little weird, IIRC the workflow hooks should detect those
>> whitespace errors at commit time, e.g:
>>
>> $ git commit  -a --amend
>> ERROR:TRAILING_WHITESPACE: trailing whitespace
>> #9: FILE: fs/btrfs/file.c:2207:
>> +^I$
>>
>> total: 1 errors, 0 warnings, 7 lines checked
>> Checkpatch found errors, would you like to fix them up? (Yn)
>>
>> But it was never triggered at any of the code move.
>>
>> I know I missed a lot of style changes when moving the code, but I
>> didn't expect any whitespace errors.
>>
>> Mind to provide some examples where the git hooks didn't catch them?
> 
> I don't use the git hooks for checks so I'll copy it from the patches:
> 
> https://lore.kernel.org/linux-btrfs/b0bd320dba85d72a34a4f7e5ba6b6c42caedbe41.1742443383.git.wqu@suse.com/
> 
> @@ -1074,6 +1074,27 @@ int btrfs_write_check(struct kiocb *iocb, size_t count)
>   	return 0;
>   }
>   
> +static void release_space(struct btrfs_inode *inode,
> +			  struct extent_changeset *data_reserved,
> +			  u64 start, u64 len,
> +			  bool only_release_metadata)
> +{
> +	const struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +
> +	if (!len)
> +		return;
> +
> +	if (only_release_metadata) {
> +		btrfs_check_nocow_unlock(inode);
> +		btrfs_delalloc_release_metadata(inode, len, true);
> +	} else {
> +		btrfs_delalloc_release_space(inode,
> +				data_reserved,
> +				round_down(start, fs_info->sectorsize),
> +				len, true);
> +	}
> +}
> ---
> 
> The parameters of btrfs_delalloc_release_space(), matching its origin:
> 
> -	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);
> -		}
> -	}
> ---
> 
> Maybe the checks ignore possible whitespace adjustments in moved code,
> e.g. comparing the - and + change literally and not taking the new
> location into account.

Sorry, I just compared the patch and the commit inside for-next branch, 
but I didn't see any white-space related changes.

Only the fs_info grabbing is moved into the else branch inside 
release_space(), and moving the indent of parameter to align with the 
'(' for btrfs_delalloc_release_space().

If by white-space, you mean strict indent for all parameters, that's not 
(and I guess it will never) be implemented by check-patch.pl.

> 
> Basically the same pattern in https://lore.kernel.org/linux-btrfs/522bfb923444f08b2c68c51a05cb5ca8b3ac7a77.1742443383.git.wqu@suse.com/

The same indent pattern you modified.

Personally speaking, I do not think the indent alignment to '(' should 
be mandatory.

The indent different is small, won't change the reader's ability to know 
it's a parameter, and there are both types of indent usages across the 
filesystems.

To be honest, aligning to '(' will only make future function rename 
harder, thus I prefer fixed tab indent other than strong alignment.

But if you insist, I can follow it in the future, but since it's not 
included in checkpatch.pl, there will be missing ones here and there.

Although I really prefer to not have such a mandatory alignment 
requirement, to reduce the burden on you and all other developers.

Thanks,
Qu

> 
> code moved to reserve_space() from btrfs_buffered_write(), parameters passed to
> btrfs_free_reserved_data_space().
> 
> I remember fixing more code like this, but hopefully the two examples would be
> sufficient to test the hook checks.