[PATCH v3 0/4] btrfs: dev-replace: replace_task related cleanup

[PATCH v3 0/4] btrfs: dev-replace: replace_task related cleanup

[Qu Wenruo]

[CHANGELOG]
v3:
- More cleanup patches for dev-replace
  Most newer ones are minor, e.g. changing the message and its timing.
  But the error handling inside btrfs_dev_replace_finishing() is
  important for later replace_task cleanup.

- Directly modify replace_task when the replace is suspended
  We do not need any extra lock, since the only reader is accessing
  replace_task without lock anyway.

We have 4 errors paths that do not properly reset replace_task to NULL
after the replace is cancelled/finished/suspended.

This is reported by Sashiko.

And since we're here, also do extra cleanup related to dev-replace error
message and its timing.

Qu Wenruo (4):
  btrfs: dev-replace: fix the incorrect error message
  btrfs: dev-replace: concentrate all messages into
    btrfs_dev_replace_finishing()
  btrfs: dev-replace: fix the error handling in
    btrfs_dev_replace_finishing()
  btrfs: dev-replace: properly cleanup replace_task

 fs/btrfs/dev-replace.c | 79 +++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 35 deletions(-)

-- 
2.55.0

[PATCH v3 1/4] btrfs: dev-replace: fix the incorrect error message

[Qu Wenruo]

The error message for a failed dev-replace is:

 "btrfs_scrub_dev(%s, %llu, %s) failed %d"

Which is not showing if this is dev-replace or a regular scrub, and not
consistent with the finished or canceled dev-replace messages:

 "dev_replace from %s (devid %llu) to %s finished"
 "dev_replace from %s (devid %llu) to %s canceled"

To reduce confusion, change the error message to be more consistent with
the remaining two.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 72cba7fed942..a0bfc4e597a2 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -948,7 +948,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	} else {
 		if (scrub_ret != -ECANCELED)
 			btrfs_err(fs_info,
-				 "btrfs_scrub_dev(%s, %llu, %s) failed %d",
+				 "dev_replace from %s (devid %llu) to %s failed %d",
 				 btrfs_dev_name(src_device),
 				 src_device->devid,
 				 btrfs_dev_name(tgt_device), scrub_ret);
-- 
2.55.0

[PATCH v3 2/4] btrfs: dev-replace: concentrate all messages into btrfs_dev_replace_finishing()

[Qu Wenruo]

Currently we have 4 different messages when a dev-replace finished:

- "dev_replace from %s (devid %llu) to %s finished"
- "dev_replace from %s (devid %llu) to %s failed"
  These messages are from btrfs_dev_replace_finishing().

- "dev_replace from %s (devid %llu) to %s canceled"
  This message is from btrfs_dev_replace_cancel().

  But btrfs_dev_replace_cancel() itself doesn't immediately finish the
  running replace, but only increases the cancel request, and wait for
  the running one to finish.

  Furthermore, to avoid double messages btrfs_dev_replace_finishing() is
  checking -ECANCELED, making the handling a little more complex.

- "suspended dev_replace from %s (devid %llu) to %s canceled"
  This is a special one, suspended replace means it's already not
  running, thus will not hit btrfs_dev_replace_finishing().
  For this call site, we have to manually output the message.

Concentrate the first 3 messages into btrfs_dev_replace_finishing(),
which makes the message output in btrfs_dev_replace_finishing() a little
simpler.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index a0bfc4e597a2..be4031a6ce1d 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -946,7 +946,12 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 								src_device,
 								tgt_device);
 	} else {
-		if (scrub_ret != -ECANCELED)
+		if (scrub_ret == -ECANCELED)
+			btrfs_info(fs_info,
+				"dev_replace from %s (devid %llu) to %s canceled",
+				btrfs_dev_name(src_device), src_device->devid,
+				btrfs_dev_name(tgt_device));
+		else
 			btrfs_err(fs_info,
 				 "dev_replace from %s (devid %llu) to %s failed %d",
 				 btrfs_dev_name(src_device),
@@ -1112,19 +1117,10 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 		src_device = dev_replace->srcdev;
 		up_write(&dev_replace->rwsem);
 		ret = btrfs_scrub_cancel(fs_info);
-		if (ret < 0) {
+		if (ret < 0)
 			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
-		} else {
+		else
 			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
-			/*
-			 * btrfs_dev_replace_finishing() will handle the
-			 * cleanup part
-			 */
-			btrfs_info(fs_info,
-				"dev_replace from %s (devid %llu) to %s canceled",
-				btrfs_dev_name(src_device), src_device->devid,
-				btrfs_dev_name(tgt_device));
-		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		/*
-- 
2.55.0

[PATCH v3 3/4] btrfs: dev-replace: fix the error handling in btrfs_dev_replace_finishing()

[Qu Wenruo]

In btrfs_dev_replace_finishing(), we have the following error paths that
do not do any cleanup:

- btrfs_start_delalloc_roots() error
- btrfs_start_transaction() error

Both will error out directly, leaving the existing dev_replace
untouched, meaning the dev-replace will still be treated as running,
blocking future dev_replace.

Fix the problem by letting the above errors to go to a new out label,
which will do the common dev_replace cleanup, with an error message
showing that dev-replace has failed.

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

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index be4031a6ce1d..df048cf69621 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -893,8 +893,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	 */
 	ret = btrfs_start_delalloc_roots(fs_info, LONG_MAX, false);
 	if (ret) {
-		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
-		return ret;
+		mutex_lock(&fs_devices->device_list_mutex);
+		mutex_lock(&fs_info->chunk_mutex);
+		goto out;
 	}
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, NULL);
 
@@ -906,8 +907,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	while (1) {
 		trans = btrfs_start_transaction(root, 0);
 		if (IS_ERR(trans)) {
-			mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
-			return PTR_ERR(trans);
+			ret = PTR_ERR(trans);
+			mutex_lock(&fs_devices->device_list_mutex);
+			mutex_lock(&fs_info->chunk_mutex);
+			goto out;
 		}
 		ret = btrfs_commit_transaction(trans);
 		WARN_ON(ret);
@@ -925,10 +928,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		}
 	}
 
+out:
 	down_write(&dev_replace->rwsem);
-	dev_replace->replace_state =
-		scrub_ret ? BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED
-			  : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
 	dev_replace->tgtdev = NULL;
 	dev_replace->srcdev = NULL;
 	dev_replace->time_stopped = ktime_get_real_seconds();
@@ -938,14 +939,17 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	 * Update allocation state in the new device and replace the old device
 	 * with the new one in the mapping tree.
 	 */
-	if (!scrub_ret) {
-		scrub_ret = btrfs_set_target_alloc_state(src_device, tgt_device);
-		if (scrub_ret)
-			goto error;
-		btrfs_dev_replace_update_device_in_mapping_tree(fs_info,
-								src_device,
-								tgt_device);
-	} else {
+	if (!scrub_ret && !ret) {
+		ret = btrfs_set_target_alloc_state(src_device, tgt_device);
+		if (!ret)
+			btrfs_dev_replace_update_device_in_mapping_tree(fs_info,
+									src_device,
+									tgt_device);
+	}
+	dev_replace->replace_state =
+		(scrub_ret || ret) ? BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED
+				   : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
+	if (scrub_ret || ret) {
 		if (scrub_ret == -ECANCELED)
 			btrfs_info(fs_info,
 				"dev_replace from %s (devid %llu) to %s canceled",
@@ -956,8 +960,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				 "dev_replace from %s (devid %llu) to %s failed %d",
 				 btrfs_dev_name(src_device),
 				 src_device->devid,
-				 btrfs_dev_name(tgt_device), scrub_ret);
-error:
+				 btrfs_dev_name(tgt_device), scrub_ret ? scrub_ret : ret);
 		up_write(&dev_replace->rwsem);
 		mutex_unlock(&fs_info->chunk_mutex);
 		mutex_unlock(&fs_devices->device_list_mutex);
@@ -967,7 +970,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		btrfs_rm_dev_replace_unblocked(fs_info);
 		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 
-		return scrub_ret;
+		return scrub_ret ? scrub_ret : ret;
 	}
 
 	btrfs_info(fs_info,
-- 
2.55.0

[PATCH v3 4/4] btrfs: dev-replace: properly cleanup replace_task

[Qu Wenruo]

There are several involved error paths, that we can error out without
resetting replace_task back to NULL:

- A running dev-replace in btrfs_dev_replace_start()
  This is impossible to hit, as dev-replace is an exclusive operation,
  meaning new dev-replace request will fail immediately without reaching
  btrfs_dev_replace_by_ioctl().

- The btrfs_start_transaction() call failed in btrfs_dev_replace_start()
  Possible but not common

- The dev-replace is suspended in btrfs_dev_replace_finishing()
  This happens when remounting a btrfs read-only with a running replace,
  or during unmount.

  btrfs_dev_replace_suspend_for_unmount() is the only call site changing
  replace_state for a running replace, other than
  btrfs_dev_replace_finishing().

  btrfs_dev_replace_suspend_for_unmount() only changed replace_state to
  SUSPENDED, but the running replace is still running.
  It's the later btrfs_scrub_cancel() that really waits for the running
  replace to finish and handle the suspended case.

  But for this suspended case, we do not reset replace_task.

- A new error hit inside btrfs_dev_replace_finishing()
  We can hit errors inside btrfs_dev_replace_finishing() itself, like
  failed to run delalloc roots or start a new transaction.

  In that case, replace_task is only cleared when the replace finished
  without any error.

This can result in the replace_task check inside btrfs_map_block() to be
incorrectly triggered, not taking dev_replace->rwsem, thus not properly
protected against the running dev-replace state changes.

Fix this minor error handling problem by:

- Moving the replace_task assignment after replace_state check in
  btrfs_dev_replace_start()

- Reset replace_task to NULL if btrfs_start_transaction() failed
  in btrfs_dev_replace_start()

- Reset replace_task to NULL for btrfs_dev_replace_finishing() for
  suspended run

- Reset replace_task to NULL for btrfs_dev_replace_finishing() for
  cancelled/finished run

This is reported by Sashiko, which found the existing bug during review
of another patch, and since the bug is an existing one, it's not shown in
the summary, but only in the detail page.

Link: https://sashiko.dev/#/patchset/tencent_853134544C3CE88A219EEB21346E2510D308%40qq.com
Fixes: 8cca35cb29f8 ("btrfs: don't take dev_replace rwsem on task already holding it")
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index df048cf69621..daee306de43a 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -633,7 +633,6 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		goto leave;
 
 	down_write(&dev_replace->rwsem);
-	dev_replace->replace_task = current;
 	switch (dev_replace->replace_state) {
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
@@ -650,6 +649,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	dev_replace->cont_reading_from_srcdev_mode = read_src;
 	dev_replace->srcdev = src_device;
 	dev_replace->tgtdev = tgt_device;
+	dev_replace->replace_task = current;
 
 	btrfs_info(fs_info,
 		      "dev_replace from %s (devid %llu) to %s started",
@@ -693,6 +693,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
 		dev_replace->srcdev = NULL;
 		dev_replace->tgtdev = NULL;
+		dev_replace->replace_task = NULL;
 		up_write(&dev_replace->rwsem);
 		goto leave;
 	}
@@ -875,9 +876,19 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	mutex_lock(&dev_replace->lock_finishing_cancel_unmount);
 
 	down_read(&dev_replace->rwsem);
-	/* was the operation canceled, or is it finished? */
 	if (dev_replace->replace_state !=
 	    BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
+		/*
+		 * Only suspended replace can reach here. Regular cancelling
+		 * will not touch replace_state.
+		 *
+		 * Suspended replace won't reset tgt/src dev as it may be resumed
+		 * later.
+		 * And we do not need write rwsem lock to change replace_task.
+		 * The only reader in btrfs_map_block() doesn't acquire rwsem
+		 * to access replace_task either.
+		 */
+		dev_replace->replace_task = NULL;
 		up_read(&dev_replace->rwsem);
 		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 		return 0;
@@ -932,6 +943,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	down_write(&dev_replace->rwsem);
 	dev_replace->tgtdev = NULL;
 	dev_replace->srcdev = NULL;
+	dev_replace->replace_task = NULL;
 	dev_replace->time_stopped = ktime_get_real_seconds();
 	dev_replace->item_needs_writeback = 1;
 
@@ -994,8 +1006,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 
 	list_add(&tgt_device->dev_alloc_list, &fs_devices->alloc_list);
 	fs_devices->rw_devices++;
-
-	dev_replace->replace_task = NULL;
 	up_write(&dev_replace->rwsem);
 	btrfs_rm_dev_replace_blocked(fs_info);
 
-- 
2.55.0

Re: [PATCH v3 3/4] btrfs: dev-replace: fix the error handling in btrfs_dev_replace_finishing()

[David Sterba]

On Mon, Aug 24, 2026 at 02:04:07PM +0930, Qu Wenruo wrote:
> In btrfs_dev_replace_finishing(), we have the following error paths that
> do not do any cleanup:
> 
> - btrfs_start_delalloc_roots() error
> - btrfs_start_transaction() error
> 
> Both will error out directly, leaving the existing dev_replace
> untouched, meaning the dev-replace will still be treated as running,
> blocking future dev_replace.
> 
> Fix the problem by letting the above errors to go to a new out label,
> which will do the common dev_replace cleanup, with an error message
> showing that dev-replace has failed.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/dev-replace.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index be4031a6ce1d..df048cf69621 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -893,8 +893,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	 */
>  	ret = btrfs_start_delalloc_roots(fs_info, LONG_MAX, false);
>  	if (ret) {
> -		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
> -		return ret;
> +		mutex_lock(&fs_devices->device_list_mutex);
> +		mutex_lock(&fs_info->chunk_mutex);

The conditional locking inside an 'if' is not common, I don't see a
better way how to do it so suggestion below

> +		goto out;
>  	}
>  	btrfs_wait_ordered_roots(fs_info, U64_MAX, NULL);
>  
> @@ -906,8 +907,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	while (1) {
>  		trans = btrfs_start_transaction(root, 0);
>  		if (IS_ERR(trans)) {
> -			mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
> -			return PTR_ERR(trans);
> +			ret = PTR_ERR(trans);
> +			mutex_lock(&fs_devices->device_list_mutex);
> +			mutex_lock(&fs_info->chunk_mutex);
> +			goto out;
>  		}
>  		ret = btrfs_commit_transaction(trans);
>  		WARN_ON(ret);
> @@ -925,10 +928,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> +out:

Add lockdep_assert_held() so it's clear that all paths leading here will
have the mutexes taken.

>  	down_write(&dev_replace->rwsem);
> -	dev_replace->replace_state =
> -		scrub_ret ? BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED
> -			  : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
>  	dev_replace->tgtdev = NULL;
>  	dev_replace->srcdev = NULL;
>  	dev_replace->time_stopped = ktime_get_real_seconds();
> @@ -938,14 +939,17 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	 * Update allocation state in the new device and replace the old device
>  	 * with the new one in the mapping tree.
>  	 */
> -	if (!scrub_ret) {
> -		scrub_ret = btrfs_set_target_alloc_state(src_device, tgt_device);
> -		if (scrub_ret)
> -			goto error;
> -		btrfs_dev_replace_update_device_in_mapping_tree(fs_info,
> -								src_device,
> -								tgt_device);
> -	} else {
> +	if (!scrub_ret && !ret) {
> +		ret = btrfs_set_target_alloc_state(src_device, tgt_device);
> +		if (!ret)
> +			btrfs_dev_replace_update_device_in_mapping_tree(fs_info,
> +									src_device,
> +									tgt_device);
> +	}
> +	dev_replace->replace_state =
> +		(scrub_ret || ret) ? BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED
> +				   : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
> +	if (scrub_ret || ret) {
>  		if (scrub_ret == -ECANCELED)
>  			btrfs_info(fs_info,
>  				"dev_replace from %s (devid %llu) to %s canceled",
> @@ -956,8 +960,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  				 "dev_replace from %s (devid %llu) to %s failed %d",
>  				 btrfs_dev_name(src_device),
>  				 src_device->devid,
> -				 btrfs_dev_name(tgt_device), scrub_ret);
> -error:
> +				 btrfs_dev_name(tgt_device), scrub_ret ? scrub_ret : ret);
>  		up_write(&dev_replace->rwsem);
>  		mutex_unlock(&fs_info->chunk_mutex);
>  		mutex_unlock(&fs_devices->device_list_mutex);
> @@ -967,7 +970,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  		btrfs_rm_dev_replace_unblocked(fs_info);
>  		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
>  
> -		return scrub_ret;
> +		return scrub_ret ? scrub_ret : ret;

You can use the short form 'return scrub_ret ?: ret'

>  	}
>  
>  	btrfs_info(fs_info,
> -- 
> 2.55.0
>