[PATCH 0/2] btrfs: raid56: minor fixes to rmw_rbio()

[PATCH 0/2] btrfs: raid56: minor fixes to rmw_rbio()

[Qu Wenruo]

I was checking the bio_list_lock usage as some call sites doesn't look
sane, e.g. spin_lock(); set_bit(); spin_unlock();.

But during the cleanup and reviewing, LLM exposed two new bugs:

- rbio_orig_end_io() can race with lock_stripe_add()
  And rbio_orig_end_io() didn't hold the bio_list_lock, under certain
  race condition, a bio can even be missing.

  Fixed by the first patch.

- Failed rbio can still be cached
  Fixed by the second patch.

Qu Wenruo (2):
  btrfs: fix racy bio_list changes for failed RMW rbio
  btrfs: don't cache rbio if an error is hit

 fs/btrfs/raid56.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

-- 
2.56.0

[PATCH 1/2] btrfs: fix racy bio_list changes for failed RMW rbio

[Qu Wenruo]

If an RMW work failed early, e.g. alloc_rbio_parity_pages() failed, it
won't get RBIO_RMW_LOCKED_BIT set, and get rbio_orig_end_io() called.

In that case, there can be a race between bio_list_get() on the rbio,
and new incoming bio merging into the rbio:

 A is the rbio that rbio_orig_end_io() is running on.
 a is the higher layer bio that generated the rbio A.

 B is the new rbio that will be merged into A.
 b is the higher layer bio that generated the rbio B.

   rbio_orig_end_io(A) worker         |       lock_stripe_add(B)
--------------------------------------+---------------------------------
rbio_orig_end_io(A)                   | lock_stripe_add(B)
|- cur = bio_list_get(A)              | |- rbio_can_merge(A, B)
|  |- bio = A->bio_list.head;         | |  which returned true
|  |  which bio a is returned.        | |
|  |                                  | |
|  |- A->bio_list.tail = NULL;        | |
|  |                                  | |
|  |                                  | |- merge_rbio(A, B)
|  |                                  | |  |- bio_list_merge_init()
|  |                                  | |     |- if (A->bio_list.tail)
|  |                                  | |     |  A->bio_list.tail is
|  |                                  | |     |  still NULL
|  |                                  | |     |
|  |                                  | |     |- A->bio_list.head = b
|  |                                  | |     |- A->bio_list.tail = b
|  |- A->bio_list.head = NULL;        | |- return 1;
|  |- return bio;                     |    Caller expect rbio A to
|     which is bio a.                 |    complete bio b.
|
|- extra = bio_list_get()
|  Which is NULL
|
|- rbio_endio_bio_list(cur);
   Which only ends bio a.

In above case, the bio_list race can get bio b lost, and never ended.
Thankfully this race window is very tiny, so not practical in the real
world.

Fix it by setting RBIO_RMW_LOCKED_BIT for error path before calling
rbio_orig_end_io().
And since we're ensuring all RMW rbios have RBIO_RMW_LOCKED_BIT bit set
before calling rbio_orig_end_io(), add two ASSERT()s for that and remove
the old comment.

Fixes: 5eb30ee26fa4 ("btrfs: raid56: introduce the main entrance for RMW path")
Assisted-by: LLM (find and analyze the bug)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 7dd937fa0c26..b24ec362909b 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -874,7 +874,10 @@ static void rbio_endio_bio_list(struct bio *cur, blk_status_t status)
 static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t status)
 {
 	struct bio *cur = bio_list_get(&rbio->bio_list);
-	struct bio *extra;
+
+	/* An RMW rbio must have RBIO_RMW_LOCKED_BIT set. */
+	if (rbio->operation == BTRFS_RBIO_WRITE)
+		ASSERT(test_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags));
 
 	kfree(rbio->csum_buf);
 	bitmap_free(rbio->csum_bitmap);
@@ -888,21 +891,11 @@ static void rbio_orig_end_io(struct btrfs_raid_bio *rbio, blk_status_t status)
 	 */
 	bitmap_clear(&rbio->dbitmap, 0, rbio->stripe_nsectors);
 
-	/*
-	 * At this moment, rbio->bio_list is empty, however since rbio does not
-	 * always have RBIO_RMW_LOCKED_BIT set and rbio is still linked on the
-	 * hash list, rbio may be merged with others so that rbio->bio_list
-	 * becomes non-empty.
-	 * Once unlock_stripe() is done, rbio->bio_list will not be updated any
-	 * more and we can call bio_endio() on all queued bios.
-	 */
 	unlock_stripe(rbio);
-	extra = bio_list_get(&rbio->bio_list);
+	ASSERT(bio_list_empty(&rbio->bio_list));
 	free_raid_bio(rbio);
 
 	rbio_endio_bio_list(cur, status);
-	if (extra)
-		rbio_endio_bio_list(extra, status);
 }
 
 /*
@@ -2464,6 +2457,21 @@ static void rmw_rbio(struct btrfs_raid_bio *rbio)
 		}
 	}
 out:
+	/*
+	 * If we hit an error early, the rbio won't get
+	 * RBIO_RMW_LOCKED_BIT bit set, and can cause unexpected
+	 * merge during rbio_orig_end_io().
+	 *
+	 * So set that flag before calling rbio_orig_end_io(), and
+	 * we have to hold the bio_list_lock, to make sure
+	 * rbio_can_merge() and merge_rbio() are seeing a stable
+	 * flag.
+	 */
+	if (ret < 0) {
+		spin_lock(&rbio->bio_list_lock);
+		set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags);
+		spin_unlock(&rbio->bio_list_lock);
+	}
 	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
 }
 
-- 
2.56.0

[PATCH 2/2] btrfs: don't cache rbio if an error is hit

[Qu Wenruo]

If rmw_rbio() hit an error after cache_rbio_pages() call, the rbio still
have RBIO_CACHE_READY_BIT set, thus can be utilized for future page
stealing.

But that rbio didn't really reach the disk, thus using the pages for
future steal can cause data inconsistency between in-memory cache and
on-disk data.

Fix the problem by clearing the RBIO_CACHE_READY_BIT on the error path
of rmw_rbio().

Fixes: 5eb30ee26fa4 ("btrfs: raid56: introduce the main entrance for RMW path")
Assisted-by: LLM (find the bug)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/raid56.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index b24ec362909b..a8a8004bb14c 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2471,6 +2471,9 @@ static void rmw_rbio(struct btrfs_raid_bio *rbio)
 		spin_lock(&rbio->bio_list_lock);
 		set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags);
 		spin_unlock(&rbio->bio_list_lock);
+
+		/* Failed write, don't cache for page stealing. */
+		clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
 	}
 	rbio_orig_end_io(rbio, errno_to_blk_status(ret));
 }
-- 
2.56.0

Re: [PATCH 0/2] btrfs: raid56: minor fixes to rmw_rbio()

[Filipe Manana]

On Wed, Oct 28, 2026 at 6:31 AM Qu Wenruo <wqu@suse.com> wrote:
>
> I was checking the bio_list_lock usage as some call sites doesn't look
> sane, e.g. spin_lock(); set_bit(); spin_unlock();.
>
> But during the cleanup and reviewing, LLM exposed two new bugs:
>
> - rbio_orig_end_io() can race with lock_stripe_add()
>   And rbio_orig_end_io() didn't hold the bio_list_lock, under certain
>   race condition, a bio can even be missing.
>
>   Fixed by the first patch.
>
> - Failed rbio can still be cached
>   Fixed by the second patch.
>
> Qu Wenruo (2):
>   btrfs: fix racy bio_list changes for failed RMW rbio
>   btrfs: don't cache rbio if an error is hit

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

Thanks.

>
>  fs/btrfs/raid56.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> --
> 2.56.0
>
>