[PATCH v2 0/4] btrfs: btrfs: fix possible race between error handling and writeback

[PATCH v2 0/4] btrfs: btrfs: fix possible race between error handling and writeback

[Qu Wenruo]

[CHANGELOG]
v2:
- Add a new patch to explain the error handling better
  This makes the later nocow_one_range() error handling change easier to explain.

There are some rare kernel warning for experimental btrfs builds that
the DEBUG_WARN() can be triggered from btrfs_writepage_cow_fixup(),
mostly after some delalloc range failure.

The root cause is explained the the last patch, the TL;DR is we
shouldn't call btrfs_cleanup_ordered_extents() on folios that are
already unlocked.

Those unlocked folios can be under writeback, and if we cleared the
order flag just before the writeback thread entering
btrfs_writepage_cow_fixup(), we will trigger the warning.

The first patch enhance the error handling of run_delalloc_nocow(), with
proper comments and charts explaining the cleanup range.

The second patch is a small enhancement to the error messages, which
helps debugging.

The third patch is to make nocow_one_range() to do proper cleanup,
aligning itself to cow_file_range().

The last one is to fix the race window by keep folios of successful
ranges locked, so that we either unlock them manually at the end of
run_delalloc_nocow(), or get btrfs_cleanup_ordered_extents() called on
locked folios for error handling.


Qu Wenruo (4):
  btrfs: rework the error handling of run_delalloc_nocow()
  btrfs: enhance error messages for delalloc range failure
  btrfs: make nocow_one_range() to do cleanup on error
  btrfs: keep folios locked inside run_delalloc_nocow()

 fs/btrfs/inode.c | 259 +++++++++++++++++++++++------------------------
 1 file changed, 128 insertions(+), 131 deletions(-)

-- 
2.50.1

[PATCH v2 1/4] btrfs: rework the error handling of run_delalloc_nocow()

[Qu Wenruo]

Currently the error handling of run_delalloc_nocow() modifies
@cur_offset to handle different parts of the delalloc range.

However the error handling can always be split into 3 parts:

1) The range with ordered extents allocated (OE cleanup)
   We have to cleanup the ordered extents and unlock the folios.

2) The range that have been cleaned up (skip)
   For now it's only for the range of fallback_to_cow().

   We should not touch it at all.

3) The range that is not yet touched (untouched)
   We have to unlock the folios and clear any reserved space.

This 3 ranges split has the same principle as cow_file_range(), however
the NOCOW/COW handling makes the above 3 range split much more complex:

a) Failure immediately after a successful OE allocation
   Thus no @cow_start nor @cow_end set.

   start         cur_offset               end
   | OE cleanup  |       untouched        |

b) Failure after hitting a COW range but before calling
   fallback_to_cow()

   start        cow_start    cur_offset   end
   | OE Cleanup |       untouched         |

c) Failure to call fallback_to_cow()

   start        cow_start    cow_end      end
   | OE Cleanup |    skip    |  untouched |

Instead of modifying @cur_offset, do proper range calculation for
OE-cleanup and untouched ranges using above 3 cases with proper range
charts.

This avoid updating @cur_offset, as it will an extra output for debug
purposes later, and explain the behavior easier.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 793b1d520e8d..c7e2205c466f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2047,6 +2047,12 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	bool check_prev = true;
 	u64 ino = btrfs_ino(inode);
 	struct can_nocow_file_extent_args nocow_args = { 0 };
+	/* The range that has ordered extent(s). */
+	u64 oe_cleanup_start;
+	u64 oe_cleanup_len = 0;
+	/* The range that is untouched. */
+	u64 untouched_start;
+	u64 untouched_len = 0;
 
 	/*
 	 * Normally on a zoned device we're only doing COW writes, but in case
@@ -2232,76 +2238,72 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	return 0;
 
 error:
-	/*
-	 * There are several error cases:
-	 *
-	 * 1) Failed without falling back to COW
-	 *    start         cur_offset             end
-	 *    |/////////////|                      |
-	 *
-	 *    In this case, cow_start should be (u64)-1.
-	 *
-	 *    For range [start, cur_offset) the folios are already unlocked (except
-	 *    @locked_folio), EXTENT_DELALLOC already removed.
-	 *    Need to clear the dirty flags and finish the ordered extents.
-	 *
-	 * 2) Failed with error before calling fallback_to_cow()
-	 *
-	 *    start         cow_start              end
-	 *    |/////////////|                      |
-	 *
-	 *    In this case, only @cow_start is set, @cur_offset is between
-	 *    [cow_start, end)
-	 *
-	 *    It's mostly the same as case 1), just replace @cur_offset with
-	 *    @cow_start.
-	 *
-	 * 3) Failed with error from fallback_to_cow()
-	 *
-	 *    start         cow_start   cow_end    end
-	 *    |/////////////|-----------|          |
-	 *
-	 *    In this case, both @cow_start and @cow_end is set.
-	 *
-	 *    For range [start, cow_start) it's the same as case 1).
-	 *    But for range [cow_start, cow_end), all the cleanup is handled by
-	 *    cow_file_range(), we should not touch anything in that range.
-	 *
-	 * So for all above cases, if @cow_start is set, cleanup ordered extents
-	 * for range [start, @cow_start), other wise cleanup range [start, @cur_offset).
-	 */
-	if (cow_start != (u64)-1)
-		cur_offset = cow_start;
-
-	if (cur_offset > start) {
-		btrfs_cleanup_ordered_extents(inode, start, cur_offset - start);
-		cleanup_dirty_folios(inode, locked_folio, start, cur_offset - 1, ret);
+	if (cow_start == (u64)-1) {
+		/*
+		 * case a)
+		 *    start           cur_offset               end
+		 *    |   OE cleanup  |       Untouched        |
+		 *
+		 * We finished a fallback_to_cow() or nocow_one_range() call, but
+		 * failed to check the next range.
+		 */
+		oe_cleanup_start = start;
+		oe_cleanup_len = cur_offset - start;
+		untouched_start = cur_offset;
+		untouched_len = end + 1 - untouched_start;
+	} else if (cow_start != (u64)-1 && cow_end == 0) {
+		/*
+		 * case b)
+		 *    start        cow_start    cur_offset   end
+		 *    | OE cleanup |        Untouched        |
+		 *
+		 * We got a range that needs COW, but before we hit the next NOCOW range,
+		 * thus [cow_start, cur_offset) doesn't yet have any OE.
+		 */
+		oe_cleanup_start = start;
+		oe_cleanup_len = cow_start - start;
+		untouched_start = cow_start;
+		untouched_len = end + 1 - untouched_start;
+	} else {
+		/*
+		 * case c)
+		 *    start        cow_start    cow_end      end
+		 *    | OE cleanup |   Skip     |  Untouched |
+		 *
+		 * fallback_to_cow() failed, and fallback_to_cow() will do the
+		 * cleanup for its range, we shouldn't touch the range
+		 * [cow_start, cow_end].
+		 */
+		ASSERT(cow_start != (u64)-1 && cow_end != 0);
+		oe_cleanup_start = start;
+		oe_cleanup_len = cow_start - start;
+		untouched_start = cow_end + 1;
+		untouched_len = end + 1 - untouched_start;
 	}
 
-	/*
-	 * If an error happened while a COW region is outstanding, cur_offset
-	 * needs to be reset to @cow_end + 1 to skip the COW range, as
-	 * cow_file_range() will do the proper cleanup at error.
-	 */
-	if (cow_end)
-		cur_offset = cow_end + 1;
+	if (oe_cleanup_len) {
+		btrfs_cleanup_ordered_extents(inode, oe_cleanup_start, oe_cleanup_len);
+		cleanup_dirty_folios(inode, locked_folio, oe_cleanup_start,
+				     oe_cleanup_start + oe_cleanup_len - 1, ret);
+	}
 
-	/*
-	 * We need to lock the extent here because we're clearing DELALLOC and
-	 * we're not locked at this point.
-	 */
-	if (cur_offset < end) {
+	if (untouched_len) {
 		struct extent_state *cached = NULL;
+		const u64 untouched_end = untouched_start + untouched_len - 1;
 
-		btrfs_lock_extent(&inode->io_tree, cur_offset, end, &cached);
-		extent_clear_unlock_delalloc(inode, cur_offset, end,
+		/*
+		 * We need to lock the extent here because we're clearing DELALLOC and
+		 * we're not locked at this point.
+		 */
+		btrfs_lock_extent(&inode->io_tree, untouched_start, untouched_end, &cached);
+		extent_clear_unlock_delalloc(inode, untouched_start, untouched_end,
 					     locked_folio, &cached,
 					     EXTENT_LOCKED | EXTENT_DELALLOC |
 					     EXTENT_DEFRAG |
 					     EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
 					     PAGE_START_WRITEBACK |
 					     PAGE_END_WRITEBACK);
-		btrfs_qgroup_free_data(inode, NULL, cur_offset, end - cur_offset + 1, NULL);
+		btrfs_qgroup_free_data(inode, NULL, untouched_start, untouched_len, NULL);
 	}
 	btrfs_free_path(path);
 	btrfs_err_rl(fs_info,
-- 
2.50.1

[PATCH v2 2/4] btrfs: enhance error messages for delalloc range failure

[Qu Wenruo]

When running emulated write error tests like generic/475, we can hit
error messages like this:

 BTRFS error (device dm-12 state EA): run_delalloc_nocow failed, root=596 inode=264 start=1605632 len=73728: -5
 BTRFS error (device dm-12 state EA): failed to run delalloc range, root=596 ino=264 folio=1605632 submit_bitmap=0-7 start=1605632 len=73728: -5

Which is normally buried by direct IO error messages.

However above error messages are not enough to determine which is the
real range that caused the error.
Considering we can have multiple different extents in one delalloc
range (e.g. some COW extents along with some NOCOW extents), just
outputting the error at the end of run_delalloc_nocow() is not enough.

To enhance the error messages:

- Remove the rate limit on the existing error messages
  In the generic/475 example, most error messages are from direct IO,
  not really from the delalloc range.
  Considering how useful the delalloc range error messages are, we don't
  want they to be rate limited.

- Add extra @cur_offset output for cow_file_range()
- Add extra variable output for run_delalloc_nocow()
  This is especially important for run_delalloc_nocow(), as there
  are extra error paths where we can hit error without into
  nocow_one_range() nor fallback_to_cow().

- Add an error message for nocow_one_range()
  That's the missing part.
  For fallback_to_cow(), we have error message from cow_file_range()
  already.

- Constify the @len and @end local variables for nocow_one_range()
  This makes it much easier to make sure @len and @end are not modified
  at runtime.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c7e2205c466f..e3063a001791 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1534,10 +1534,11 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size,
 				       end - start - cur_alloc_size + 1, NULL);
 	}
-	btrfs_err_rl(fs_info,
-		     "%s failed, root=%llu inode=%llu start=%llu len=%llu: %d",
-		     __func__, btrfs_root_id(inode->root),
-		     btrfs_ino(inode), orig_start, end + 1 - orig_start, ret);
+	btrfs_err(fs_info,
+		  "%s failed, root=%llu inode=%llu start=%llu len=%llu cur_offset=%llu cur_alloc_size=%llu: %d",
+		  __func__, btrfs_root_id(inode->root),
+		  btrfs_ino(inode), orig_start, end + 1 - orig_start,
+		  start, cur_alloc_size, ret);
 	return ret;
 }
 
@@ -1969,8 +1970,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 			   u64 file_pos, bool is_prealloc)
 {
 	struct btrfs_ordered_extent *ordered;
-	u64 len = nocow_args->file_extent.num_bytes;
-	u64 end = file_pos + len - 1;
+	const u64 len = nocow_args->file_extent.num_bytes;
+	const u64 end = file_pos + len - 1;
 	int ret = 0;
 
 	btrfs_lock_extent(&inode->io_tree, file_pos, end, cached);
@@ -2017,8 +2018,13 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 	 * We do not clear the folio Dirty flags because they are set and
 	 * cleaered by the caller.
 	 */
-	if (ret < 0)
+	if (ret < 0) {
 		btrfs_cleanup_ordered_extents(inode, file_pos, len);
+		btrfs_err(inode->root->fs_info,
+			  "%s failed, root=%lld inode=%llu start=%llu len=%llu: %d",
+			  __func__, btrfs_root_id(inode->root), btrfs_ino(inode),
+			  file_pos, len, ret);
+	}
 	return ret;
 }
 
@@ -2306,10 +2312,11 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		btrfs_qgroup_free_data(inode, NULL, untouched_start, untouched_len, NULL);
 	}
 	btrfs_free_path(path);
-	btrfs_err_rl(fs_info,
-		     "%s failed, root=%llu inode=%llu start=%llu len=%llu: %d",
-		     __func__, btrfs_root_id(inode->root),
-		     btrfs_ino(inode), start, end + 1 - start, ret);
+	btrfs_err(fs_info,
+"%s failed, root=%llu inode=%llu start=%llu len=%llu cur_offset=%llu oe_cleanup=%llu oe_cleanup_len=%llu untouched_start=%llu untouched_len=%llu: %d",
+		  __func__, btrfs_root_id(inode->root), btrfs_ino(inode),
+		  start, end + 1 - start, cur_offset, oe_cleanup_start, oe_cleanup_len,
+		  untouched_start, untouched_len, ret);
 	return ret;
 }
 
-- 
2.50.1

[PATCH v2 3/4] btrfs: make nocow_one_range() to do cleanup on error

[Qu Wenruo]

Currently if we hit an error inside nocow_one_range(), we do not clear
the page dirty, and let the caller to handle it.

This is very different compared to fallback_to_cow(), when that function
failed, everything will be cleaned up by cow_file_range().

Enhance the situation by:

- Use a common error handling for nocow_one_range()
  If we failed anything, use the same btrfs_cleanup_ordered_extents()
  and extent_clear_unlock_delalloc().

  btrfs_cleanup_ordered_extents() is safe even if we haven't created new
  ordered extent, in that case there should be no OE and that function
  will do nothing.

  The same applies to extent_clear_unlock_delalloc(), and since we're
  passing PAGE_UNLOCK | PAGE_START_WRITEBACK | PAGE_END_WRITEBACK, it
  will also clear folio dirty flag during error handling.

- Avoid touching the failed range of nocow_one_range()
  As the failed range will be cleaned up and unlocked by that function.

  Here we introduce a new variable @nocow_end to record the failed range,
  so that we can skip it during the error handling of run_delalloc_nocow().

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e3063a001791..1466f4356826 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1982,8 +1982,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 		em = btrfs_create_io_em(inode, file_pos, &nocow_args->file_extent,
 					BTRFS_ORDERED_PREALLOC);
 		if (IS_ERR(em)) {
-			btrfs_unlock_extent(&inode->io_tree, file_pos, end, cached);
-			return PTR_ERR(em);
+			ret = PTR_ERR(em);
+			goto error;
 		}
 		btrfs_free_extent_map(em);
 	}
@@ -1995,8 +1995,8 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 	if (IS_ERR(ordered)) {
 		if (is_prealloc)
 			btrfs_drop_extent_map_range(inode, file_pos, end, false);
-		btrfs_unlock_extent(&inode->io_tree, file_pos, end, cached);
-		return PTR_ERR(ordered);
+		ret = PTR_ERR(ordered);
+		goto error;
 	}
 
 	if (btrfs_is_data_reloc_root(inode->root))
@@ -2008,23 +2008,24 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 		ret = btrfs_reloc_clone_csums(ordered);
 	btrfs_put_ordered_extent(ordered);
 
+	if (ret < 0)
+		goto error;
 	extent_clear_unlock_delalloc(inode, file_pos, end, locked_folio, cached,
 				     EXTENT_LOCKED | EXTENT_DELALLOC |
 				     EXTENT_CLEAR_DATA_RESV,
 				     PAGE_UNLOCK | PAGE_SET_ORDERED);
-	/*
-	 * On error, we need to cleanup the ordered extents we created.
-	 *
-	 * We do not clear the folio Dirty flags because they are set and
-	 * cleaered by the caller.
-	 */
-	if (ret < 0) {
-		btrfs_cleanup_ordered_extents(inode, file_pos, len);
-		btrfs_err(inode->root->fs_info,
-			  "%s failed, root=%lld inode=%llu start=%llu len=%llu: %d",
-			  __func__, btrfs_root_id(inode->root), btrfs_ino(inode),
-			  file_pos, len, ret);
-	}
+	return ret;
+error:
+	btrfs_cleanup_ordered_extents(inode, file_pos, len);
+	extent_clear_unlock_delalloc(inode, file_pos, end, locked_folio, cached,
+				     EXTENT_LOCKED | EXTENT_DELALLOC |
+				     EXTENT_CLEAR_DATA_RESV,
+				     PAGE_UNLOCK | PAGE_START_WRITEBACK |
+				     PAGE_END_WRITEBACK);
+	btrfs_err(inode->root->fs_info,
+		  "%s failed, root=%lld inode=%llu start=%llu len=%llu: %d",
+		  __func__, btrfs_root_id(inode->root), btrfs_ino(inode),
+		  file_pos, len, ret);
 	return ret;
 }
 
@@ -2046,8 +2047,12 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	/*
 	 * If not 0, represents the inclusive end of the last fallback_to_cow()
 	 * range. Only for error handling.
+	 *
+	 * The same for nocow_end, it's to avoid double cleaning up the range
+	 * already cleaned by nocow_one_range().
 	 */
 	u64 cow_end = 0;
+	u64 nocow_end = 0;
 	u64 cur_offset = start;
 	int ret;
 	bool check_prev = true;
@@ -2222,8 +2227,10 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 				      &nocow_args, cur_offset,
 				      extent_type == BTRFS_FILE_EXTENT_PREALLOC);
 		btrfs_dec_nocow_writers(nocow_bg);
-		if (ret < 0)
+		if (ret < 0) {
+			nocow_end = cur_offset + nocow_args.file_extent.num_bytes - 1;
 			goto error;
+		}
 		cur_offset = extent_end;
 	}
 	btrfs_release_path(path);
@@ -2250,12 +2257,22 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		 *    start           cur_offset               end
 		 *    |   OE cleanup  |       Untouched        |
 		 *
-		 * We finished a fallback_to_cow() or nocow_one_range() call, but
-		 * failed to check the next range.
+		 * We finished a fallback_to_cow() or nocow_one_range() call,
+		 * but failed to check the next range.
+		 *
+		 * or
+		 *    start           cur_offset   nocow_end   end
+		 *    |   OE cleanup  |   Skip     | Untouched |
+		 *
+		 * nocow_one_range() failed, the range [cur_offset, nocow_end] is
+		 * alread cleaned up.
 		 */
 		oe_cleanup_start = start;
 		oe_cleanup_len = cur_offset - start;
-		untouched_start = cur_offset;
+		if (nocow_end)
+			untouched_start = nocow_end + 1;
+		else
+			untouched_start = cur_offset;
 		untouched_len = end + 1 - untouched_start;
 	} else if (cow_start != (u64)-1 && cow_end == 0) {
 		/*
-- 
2.50.1

[PATCH v2 4/4] btrfs: keep folios locked inside run_delalloc_nocow()

[Qu Wenruo]

[BUG]
There is a very low chance that DEBUG_WARN() inside
btrfs_writepage_cow_fixup() can be triggered when
CONFIG_BTRFS_EXPERIMENTAL is enabled.

This only happens after run_delalloc_nocow() failed.

Unfortunately I haven't hit it for a while thus no real world dmesg for
now.

[CAUSE]
There is a race window where after run_delalloc_nocow() failed, error
handling can race with writeback thread.

Before we hit run_delalloc_nocow(), there is an inode with the following
dirty pages: (4K page size, 4K block size, no large folio)

  0         4K          8K          12K          16K
  |/////////|///////////|///////////|////////////|

The inode also have NODATACOW flag, and the above dirty range will go
through different extents during run_delalloc_range():

  0         4K          8K          12K          16K
  |  NOCOW  |    COW    |    COW    |   NOCOW    |

The race happen like this:

    writeback thread A            |        writeback thread B
----------------------------------+--------------------------------------
Writeback for folio 0             |
run_delalloc_nocow()              |
|- nocow_one_range()              |
|  For range [0, 4K), ret = 0     |
|                                 |
|- fallback_to_cow()              |
|  For range [4K, 8K), ret = 0    |
|  Folio 4K *UNLOCKED*            |
|                                 | Writeback for folio 4K
|- fallback_to_cow()              | extent_writepage()
|  For range [8K, 12K), failure   | |- writepage_delalloc()
|				  | |
|- btrfs_cleanup_ordered_extents()| |
   |- btrfs_folio_clear_ordered() | |
   |  Folio 0 still locked, safe  | |
   |                              | |  Ordered extent already allocated.
   |                              | |  Nothing to do.
   |                              | |- extent_writepage_io()
   |                              |    |- btrfs_writepage_cow_fixup()
   |- btrfs_folio_clear_ordered() |    |
      Folio 4K hold by thread B,  |    |
      UNSAFE!                     |    |- btrfs_test_ordered()
                                  |    |  Cleared by thread A,
				  |    |
                                  |    |- DEBUG_WARN();

This is only possible after run_delalloc_nocow() failure, as
cow_file_range() will keep all folios and io tree range locked, until
everything is finished or after error handling.

The root cause is we allow fallback_to_cow() and nocow_one_range() to
unlock the folios after a successful run, so that during error handling
we're no longer safe to use btrfs_cleanup_ordered_extents() as the
folios are already unlocked.

[FIX]
- Make fallback_to_cow() and nocow_one_range() to keep folios locked
  after a successful run

  For fallback_to_cow() we can pass COW_FILE_RANGE_KEEP_LOCKED flag
  into cow_file_range().

  For nocow_one_range() we have to remove the PAGE_UNLOCK flag from
  extent_clear_unlock_delalloc().

- Unlock folios if everything is fine in run_delalloc_nocow()

- Use extent_clear_unlock_delalloc() to handle range [@start,
  @cur_offset) inside run_delalloc_nocow()
  Since folios are still locked, we do not need
  cleanup_dirty_folios() to do the cleanup.

  extent_clear_unlock_delalloc() with "PAGE_START_WRIBACK |
  PAGE_END_WRITEBACK" will clear the dirty flags.

- Remove cleanup_dirty_folios()

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1466f4356826..902a1d03d20e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1772,9 +1772,15 @@ static int fallback_to_cow(struct btrfs_inode *inode,
 	 * Don't try to create inline extents, as a mix of inline extent that
 	 * is written out and unlocked directly and a normal NOCOW extent
 	 * doesn't work.
+	 *
+	 * And here we do not unlock the folio after a successful run.
+	 * The folios will be unlocked after everything is finished, or by error handling.
+	 *
+	 * This is to ensure error handling won't need to clear dirty/ordered flags without
+	 * a locked folio, which can race with writeback.
 	 */
 	ret = cow_file_range(inode, locked_folio, start, end, NULL,
-			     COW_FILE_RANGE_NO_INLINE);
+			     COW_FILE_RANGE_NO_INLINE | COW_FILE_RANGE_KEEP_LOCKED);
 	ASSERT(ret != 1);
 	return ret;
 }
@@ -1917,53 +1923,6 @@ static int can_nocow_file_extent(struct btrfs_path *path,
 	return ret < 0 ? ret : can_nocow;
 }
 
-/*
- * Cleanup the dirty folios which will never be submitted due to error.
- *
- * When running a delalloc range, we may need to split the ranges (due to
- * fragmentation or NOCOW). If we hit an error in the later part, we will error
- * out and previously successfully executed range will never be submitted, thus
- * we have to cleanup those folios by clearing their dirty flag, starting and
- * finishing the writeback.
- */
-static void cleanup_dirty_folios(struct btrfs_inode *inode,
-				 struct folio *locked_folio,
-				 u64 start, u64 end, int error)
-{
-	struct btrfs_fs_info *fs_info = inode->root->fs_info;
-	struct address_space *mapping = inode->vfs_inode.i_mapping;
-	pgoff_t start_index = start >> PAGE_SHIFT;
-	pgoff_t end_index = end >> PAGE_SHIFT;
-	u32 len;
-
-	ASSERT(end + 1 - start < U32_MAX);
-	ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&
-	       IS_ALIGNED(end + 1, fs_info->sectorsize));
-	len = end + 1 - start;
-
-	/*
-	 * Handle the locked folio first.
-	 * The btrfs_folio_clamp_*() helpers can handle range out of the folio case.
-	 */
-	btrfs_folio_clamp_finish_io(fs_info, locked_folio, start, len);
-
-	for (pgoff_t index = start_index; index <= end_index; index++) {
-		struct folio *folio;
-
-		/* Already handled at the beginning. */
-		if (index == locked_folio->index)
-			continue;
-		folio = __filemap_get_folio(mapping, index, FGP_LOCK, GFP_NOFS);
-		/* Cache already dropped, no need to do any cleanup. */
-		if (IS_ERR(folio))
-			continue;
-		btrfs_folio_clamp_finish_io(fs_info, locked_folio, start, len);
-		folio_unlock(folio);
-		folio_put(folio);
-	}
-	mapping_set_error(mapping, error);
-}
-
 static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio,
 			   struct extent_state **cached,
 			   struct can_nocow_file_extent_args *nocow_args,
@@ -2013,7 +1972,7 @@ static int nocow_one_range(struct btrfs_inode *inode, struct folio *locked_folio
 	extent_clear_unlock_delalloc(inode, file_pos, end, locked_folio, cached,
 				     EXTENT_LOCKED | EXTENT_DELALLOC |
 				     EXTENT_CLEAR_DATA_RESV,
-				     PAGE_UNLOCK | PAGE_SET_ORDERED);
+				     PAGE_SET_ORDERED);
 	return ret;
 error:
 	btrfs_cleanup_ordered_extents(inode, file_pos, len);
@@ -2247,6 +2206,14 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 		cow_start = (u64)-1;
 	}
 
+	/*
+	 * Everything is finished without an error, can unlock the folios now.
+	 *
+	 * No need to touch the io tree range nor set folio ordered flag, as
+	 * fallback_to_cow() and nocow_one_range() have already handled them.
+	 */
+	extent_clear_unlock_delalloc(inode, start, end, locked_folio, NULL, 0, PAGE_UNLOCK);
+
 	btrfs_free_path(path);
 	return 0;
 
@@ -2305,9 +2272,13 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	}
 
 	if (oe_cleanup_len) {
+		const u64 oe_cleanup_end = oe_cleanup_start + oe_cleanup_len - 1;
 		btrfs_cleanup_ordered_extents(inode, oe_cleanup_start, oe_cleanup_len);
-		cleanup_dirty_folios(inode, locked_folio, oe_cleanup_start,
-				     oe_cleanup_start + oe_cleanup_len - 1, ret);
+		extent_clear_unlock_delalloc(inode, oe_cleanup_start, oe_cleanup_end,
+					     locked_folio, NULL,
+					     EXTENT_LOCKED | EXTENT_DELALLOC,
+					     PAGE_UNLOCK | PAGE_START_WRITEBACK |
+					     PAGE_END_WRITEBACK);
 	}
 
 	if (untouched_len) {
-- 
2.50.1

Re: [PATCH v2 4/4] btrfs: keep folios locked inside run_delalloc_nocow()

[Wang Yugui]

Hi,

> [BUG]
> There is a very low chance that DEBUG_WARN() inside
> btrfs_writepage_cow_fixup() can be triggered when
> CONFIG_BTRFS_EXPERIMENTAL is enabled.
> 
> This only happens after run_delalloc_nocow() failed.
> 
> Unfortunately I haven't hit it for a while thus no real world dmesg for
> now.
> 
> [CAUSE]
> There is a race window where after run_delalloc_nocow() failed, error
> handling can race with writeback thread.
> 
> Before we hit run_delalloc_nocow(), there is an inode with the following
> dirty pages: (4K page size, 4K block size, no large folio)
> 
>   0         4K          8K          12K          16K
>   |/////////|///////////|///////////|////////////|
> 
> The inode also have NODATACOW flag, and the above dirty range will go
> through different extents during run_delalloc_range():
> 
>   0         4K          8K          12K          16K
>   |  NOCOW  |    COW    |    COW    |   NOCOW    |
> 
> The race happen like this:
> 
>     writeback thread A            |        writeback thread B
> ----------------------------------+--------------------------------------
> Writeback for folio 0             |
> run_delalloc_nocow()              |
> |- nocow_one_range()              |
> |  For range [0, 4K), ret = 0     |
> |                                 |
> |- fallback_to_cow()              |
> |  For range [4K, 8K), ret = 0    |
> |  Folio 4K *UNLOCKED*            |
> |                                 | Writeback for folio 4K
> |- fallback_to_cow()              | extent_writepage()
> |  For range [8K, 12K), failure   | |- writepage_delalloc()
> |				  | |
> |- btrfs_cleanup_ordered_extents()| |
>    |- btrfs_folio_clear_ordered() | |
>    |  Folio 0 still locked, safe  | |
>    |                              | |  Ordered extent already allocated.
>    |                              | |  Nothing to do.
>    |                              | |- extent_writepage_io()
>    |                              |    |- btrfs_writepage_cow_fixup()
>    |- btrfs_folio_clear_ordered() |    |
>       Folio 4K hold by thread B,  |    |
>       UNSAFE!                     |    |- btrfs_test_ordered()
>                                   |    |  Cleared by thread A,
> 				  |    |
>                                   |    |- DEBUG_WARN();
> 
> This is only possible after run_delalloc_nocow() failure, as
> cow_file_range() will keep all folios and io tree range locked, until
> everything is finished or after error handling.
> 
> The root cause is we allow fallback_to_cow() and nocow_one_range() to
> unlock the folios after a successful run, so that during error handling
> we're no longer safe to use btrfs_cleanup_ordered_extents() as the
> folios are already unlocked.
> 
> [FIX]
> - Make fallback_to_cow() and nocow_one_range() to keep folios locked
>   after a successful run
> 
>   For fallback_to_cow() we can pass COW_FILE_RANGE_KEEP_LOCKED flag
>   into cow_file_range().
> 
>   For nocow_one_range() we have to remove the PAGE_UNLOCK flag from
>   extent_clear_unlock_delalloc().
> 
> - Unlock folios if everything is fine in run_delalloc_nocow()
> 
> - Use extent_clear_unlock_delalloc() to handle range [@start,
>   @cur_offset) inside run_delalloc_nocow()
>   Since folios are still locked, we do not need
>   cleanup_dirty_folios() to do the cleanup.
> 
>   extent_clear_unlock_delalloc() with "PAGE_START_WRIBACK |
>   PAGE_END_WRITEBACK" will clear the dirty flags.
> 
> - Remove cleanup_dirty_folios()
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

We need a 'Fix:' tag for kernel stable branch.

It seems that this problem does not happen (or less frequency) on  kernel
6.6.y/6.1.y.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2025/08/05

Re: [PATCH v2 4/4] btrfs: keep folios locked inside run_delalloc_nocow()

[Qu Wenruo]

在 2025/8/5 08:06, Wang Yugui 写道:
> Hi,
> 
>> [BUG]
>> There is a very low chance that DEBUG_WARN() inside
>> btrfs_writepage_cow_fixup() can be triggered when
>> CONFIG_BTRFS_EXPERIMENTAL is enabled.
>>
>> This only happens after run_delalloc_nocow() failed.
>>
>> Unfortunately I haven't hit it for a while thus no real world dmesg for
>> now.
>>
>> [CAUSE]
>> There is a race window where after run_delalloc_nocow() failed, error
>> handling can race with writeback thread.
>>
>> Before we hit run_delalloc_nocow(), there is an inode with the following
>> dirty pages: (4K page size, 4K block size, no large folio)
>>
>>    0         4K          8K          12K          16K
>>    |/////////|///////////|///////////|////////////|
>>
>> The inode also have NODATACOW flag, and the above dirty range will go
>> through different extents during run_delalloc_range():
>>
>>    0         4K          8K          12K          16K
>>    |  NOCOW  |    COW    |    COW    |   NOCOW    |
>>
>> The race happen like this:
>>
>>      writeback thread A            |        writeback thread B
>> ----------------------------------+--------------------------------------
>> Writeback for folio 0             |
>> run_delalloc_nocow()              |
>> |- nocow_one_range()              |
>> |  For range [0, 4K), ret = 0     |
>> |                                 |
>> |- fallback_to_cow()              |
>> |  For range [4K, 8K), ret = 0    |
>> |  Folio 4K *UNLOCKED*            |
>> |                                 | Writeback for folio 4K
>> |- fallback_to_cow()              | extent_writepage()
>> |  For range [8K, 12K), failure   | |- writepage_delalloc()
>> |				  | |
>> |- btrfs_cleanup_ordered_extents()| |
>>     |- btrfs_folio_clear_ordered() | |
>>     |  Folio 0 still locked, safe  | |
>>     |                              | |  Ordered extent already allocated.
>>     |                              | |  Nothing to do.
>>     |                              | |- extent_writepage_io()
>>     |                              |    |- btrfs_writepage_cow_fixup()
>>     |- btrfs_folio_clear_ordered() |    |
>>        Folio 4K hold by thread B,  |    |
>>        UNSAFE!                     |    |- btrfs_test_ordered()
>>                                    |    |  Cleared by thread A,
>> 				  |    |
>>                                    |    |- DEBUG_WARN();
>>
>> This is only possible after run_delalloc_nocow() failure, as
>> cow_file_range() will keep all folios and io tree range locked, until
>> everything is finished or after error handling.
>>
>> The root cause is we allow fallback_to_cow() and nocow_one_range() to
>> unlock the folios after a successful run, so that during error handling
>> we're no longer safe to use btrfs_cleanup_ordered_extents() as the
>> folios are already unlocked.
>>
>> [FIX]
>> - Make fallback_to_cow() and nocow_one_range() to keep folios locked
>>    after a successful run
>>
>>    For fallback_to_cow() we can pass COW_FILE_RANGE_KEEP_LOCKED flag
>>    into cow_file_range().
>>
>>    For nocow_one_range() we have to remove the PAGE_UNLOCK flag from
>>    extent_clear_unlock_delalloc().
>>
>> - Unlock folios if everything is fine in run_delalloc_nocow()
>>
>> - Use extent_clear_unlock_delalloc() to handle range [@start,
>>    @cur_offset) inside run_delalloc_nocow()
>>    Since folios are still locked, we do not need
>>    cleanup_dirty_folios() to do the cleanup.
>>
>>    extent_clear_unlock_delalloc() with "PAGE_START_WRIBACK |
>>    PAGE_END_WRITEBACK" will clear the dirty flags.
>>
>> - Remove cleanup_dirty_folios()
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> We need a 'Fix:' tag for kernel stable branch.

The problem is, the original error handling behavior is not correct from 
the very beginning.

Thus I can not locate a good offending commit.

> 
> It seems that this problem does not happen (or less frequency) on  kernel
> 6.6.y/6.1.y.

Unfortunately those kernels do not have the proper error handling either.

Thanks,
Qu

> 
> Best Regards
> Wang Yugui (wangyugui@e16-tech.com)
> 2025/08/05
> 
> 
> 

Re: [PATCH v2 0/4] btrfs: btrfs: fix possible race between error handling and writeback

[David Sterba]

On Mon, Jul 28, 2025 at 05:57:53PM +0930, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Add a new patch to explain the error handling better
>   This makes the later nocow_one_range() error handling change easier to explain.
> 
> There are some rare kernel warning for experimental btrfs builds that
> the DEBUG_WARN() can be triggered from btrfs_writepage_cow_fixup(),
> mostly after some delalloc range failure.
> 
> The root cause is explained the the last patch, the TL;DR is we
> shouldn't call btrfs_cleanup_ordered_extents() on folios that are
> already unlocked.
> 
> Those unlocked folios can be under writeback, and if we cleared the
> order flag just before the writeback thread entering
> btrfs_writepage_cow_fixup(), we will trigger the warning.
> 
> The first patch enhance the error handling of run_delalloc_nocow(), with
> proper comments and charts explaining the cleanup range.
> 
> The second patch is a small enhancement to the error messages, which
> helps debugging.
> 
> The third patch is to make nocow_one_range() to do proper cleanup,
> aligning itself to cow_file_range().
> 
> The last one is to fix the race window by keep folios of successful
> ranges locked, so that we either unlock them manually at the end of
> run_delalloc_nocow(), or get btrfs_cleanup_ordered_extents() called on
> locked folios for error handling.
> 
> 
> Qu Wenruo (4):
>   btrfs: rework the error handling of run_delalloc_nocow()
>   btrfs: enhance error messages for delalloc range failure
>   btrfs: make nocow_one_range() to do cleanup on error
>   btrfs: keep folios locked inside run_delalloc_nocow()

The patches have been in linux-next for some time, feel free to add them
to for-next proper so they can be merged. Related to that, you can
remove the cow fixup code in this development cycle if you want.

Re: [PATCH v2 0/4] btrfs: btrfs: fix possible race between error handling and writeback

[Qu Wenruo]

在 2025/8/19 01:18, David Sterba 写道:
> On Mon, Jul 28, 2025 at 05:57:53PM +0930, Qu Wenruo wrote:
>> [CHANGELOG]
>> v2:
>> - Add a new patch to explain the error handling better
>>    This makes the later nocow_one_range() error handling change easier to explain.
>>
>> There are some rare kernel warning for experimental btrfs builds that
>> the DEBUG_WARN() can be triggered from btrfs_writepage_cow_fixup(),
>> mostly after some delalloc range failure.
>>
>> The root cause is explained the the last patch, the TL;DR is we
>> shouldn't call btrfs_cleanup_ordered_extents() on folios that are
>> already unlocked.
>>
>> Those unlocked folios can be under writeback, and if we cleared the
>> order flag just before the writeback thread entering
>> btrfs_writepage_cow_fixup(), we will trigger the warning.
>>
>> The first patch enhance the error handling of run_delalloc_nocow(), with
>> proper comments and charts explaining the cleanup range.
>>
>> The second patch is a small enhancement to the error messages, which
>> helps debugging.
>>
>> The third patch is to make nocow_one_range() to do proper cleanup,
>> aligning itself to cow_file_range().
>>
>> The last one is to fix the race window by keep folios of successful
>> ranges locked, so that we either unlock them manually at the end of
>> run_delalloc_nocow(), or get btrfs_cleanup_ordered_extents() called on
>> locked folios for error handling.
>>
>>
>> Qu Wenruo (4):
>>    btrfs: rework the error handling of run_delalloc_nocow()
>>    btrfs: enhance error messages for delalloc range failure
>>    btrfs: make nocow_one_range() to do cleanup on error
>>    btrfs: keep folios locked inside run_delalloc_nocow()
> 
> The patches have been in linux-next for some time, feel free to add them
> to for-next proper so they can be merged. Related to that, you can
> remove the cow fixup code in this development cycle if you want.
> 

Unfortunately even with this series applies, I'm still hitting COW fixup 
warning at a very low chance for g/475.

So I'm afraid we're not yet done about all the possible error paths, 
thus no removal of cow fixup yet.

Thanks,
Qu

Re: [PATCH v2 0/4] btrfs: btrfs: fix possible race between error handling and writeback

[David Sterba]

On Tue, Aug 19, 2025 at 07:17:28AM +0930, Qu Wenruo wrote:
> 在 2025/8/19 01:18, David Sterba 写道:
> > On Mon, Jul 28, 2025 at 05:57:53PM +0930, Qu Wenruo wrote:
> >> Qu Wenruo (4):
> >>    btrfs: rework the error handling of run_delalloc_nocow()
> >>    btrfs: enhance error messages for delalloc range failure
> >>    btrfs: make nocow_one_range() to do cleanup on error
> >>    btrfs: keep folios locked inside run_delalloc_nocow()
> > 
> > The patches have been in linux-next for some time, feel free to add them
> > to for-next proper so they can be merged. Related to that, you can
> > remove the cow fixup code in this development cycle if you want.
> > 
> 
> Unfortunately even with this series applies, I'm still hitting COW fixup 
> warning at a very low chance for g/475.
> 
> So I'm afraid we're not yet done about all the possible error paths, 
> thus no removal of cow fixup yet.

I see. Alternatively the cow fixup can be turned to proper debugging
mechanism so you can still use it to catch bugs but it does not need to
have the whole fixup mechanism. If we can get rid of the fixup worker
kthread it would still be a good incremental step as it's just idling
and consuming rerources.