[PATCH v2] btrfs: properly cleanup replace_task when the replace failed to start

[PATCH v2] btrfs: properly cleanup replace_task when the replace failed to start

[Qu Wenruo]

In the function btrfs_dev_replace_start(), we have several error paths
that assigns replace_task without reverting it back to NULL.

There are two involved error paths:

- There is already a running dev-replace
  Then replace_task is over-written to the current task.
  This is the one with long running effect.

- The btrfs_start_transaction() call failed
  This is much harder to hit though.

This can result the replace_task check inside btrfs_map_block() to be
incorrectly triggered, not taking dev_replace->rwsem, and may get an
incorrect/stale view on replace related structures.

Thankfully this bug is very hard to hit.

As dev-replace is an exclusive operation, thus if there is already
a running replace, a new one will be rejected early without reaching
btrfs_dev_replace_start().

The only remaining case is a suspended replace, which is much harder to
hit, e.g. requiring async dev-replace conflicting with another exclusive
operation, then a new replace is started.

Fix the 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 all paths of
  btrfs_dev_replace_finishing()

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")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Fix incorrect member names in the commit message

- Update the commit message to show that the bug is very hard to hit
  Mostly rejected by the exclusive operation, which rejects new replace
  early.

- Update btrfs_dev_replace_finishing() to reset replace_task
---
 fs/btrfs/dev-replace.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 72cba7fed942..c5e67524b417 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:
@@ -647,6 +646,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		goto leave;
 	}
 
+	dev_replace->replace_task = current;
 	dev_replace->cont_reading_from_srcdev_mode = read_src;
 	dev_replace->srcdev = src_device;
 	dev_replace->tgtdev = tgt_device;
@@ -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;
 	}
@@ -874,18 +875,20 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	/* don't allow cancel or unmount to disturb the finishing procedure */
 	mutex_lock(&dev_replace->lock_finishing_cancel_unmount);
 
-	down_read(&dev_replace->rwsem);
+	down_write(&dev_replace->rwsem);
+	dev_replace->replace_task = NULL;
+
 	/* was the operation canceled, or is it finished? */
 	if (dev_replace->replace_state !=
 	    BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
-		up_read(&dev_replace->rwsem);
+		up_write(&dev_replace->rwsem);
 		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
 		return 0;
 	}
 
 	tgt_device = dev_replace->tgtdev;
 	src_device = dev_replace->srcdev;
-	up_read(&dev_replace->rwsem);
+	up_write(&dev_replace->rwsem);
 
 	/*
 	 * flush all outstanding I/O and inode extent mappings before the
@@ -986,8 +989,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.54.0

Re: [PATCH v2] btrfs: properly cleanup replace_task when the replace failed to start

[Johannes Thumshirn]

Oops my bad,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2] btrfs: properly cleanup replace_task when the replace failed to start

[Jeff Layton]

Claude had some more comments on this one:

This moves the dev_replace->replace_task assignment in
btrfs_dev_replace_start() below the replace_state switch, clears it when
btrfs_start_transaction() fails there, and clears it at the top of
btrfs_dev_replace_finishing() so every exit from that function drops it.

> In the function btrfs_dev_replace_start(), we have several error paths
> that assigns replace_task without reverting it back to NULL.
>
> There are two involved error paths:
>
> - There is already a running dev-replace
>   Then replace_task is over-written to the current task.
>   This is the one with long running effect.
>
> - The btrfs_start_transaction() call failed
>   This is much harder to hit though.

[ ... ]

> Thankfully this bug is very hard to hit.
>
> As dev-replace is an exclusive operation, thus if there is already
> a running replace, a new one will be rejected early without reaching
> btrfs_dev_replace_start().
>
> The only remaining case is a suspended replace, which is much harder to
> hit, e.g. requiring async dev-replace conflicting with another exclusive
> operation, then a new replace is started.

Is "very hard to hit" still accurate now that btrfs_dev_replace_finishing()
is fixed too?

A plain "btrfs replace cancel" leaves replace_task set every time.
btrfs_dev_replace_cancel() calls btrfs_scrub_cancel() while replace_state is
BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED, so btrfs_scrub_dev() returns
-ECANCELED to btrfs_dev_replace_start(), which passes it straight into
btrfs_dev_replace_finishing():

fs/btrfs/dev-replace.c:btrfs_dev_replace_start() {
	...
	ret = btrfs_scrub_dev(fs_info, src_device->devid, 0, ...);

	ret = btrfs_dev_replace_finishing(fs_info, ret);
	...
}

fs/btrfs/dev-replace.c:btrfs_dev_replace_finishing() {
	...
	} else {
		if (scrub_ret != -ECANCELED)
			btrfs_err(fs_info, ...);
error:
		up_write(&dev_replace->rwsem);
		...
		return scrub_ret;
	}
	...
}

That exit never reaches the replace_task = NULL at the end of the function,
so the ioctl returns to userspace with replace_task still pointing at the
task that ran it.  Any scrub failure, not only -ECANCELED, lands there as
well.

The subject line says "when the replace failed to start", which does not
cover the cancel or scrub-error case either.  Could the subject and the
changelog describe those paths too?

> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 72cba7fed942..c5e67524b417 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c

[ ... ]

> @@ -874,18 +875,20 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	/* don't allow cancel or unmount to disturb the finishing procedure */
>  	mutex_lock(&dev_replace->lock_finishing_cancel_unmount);
>
> -	down_read(&dev_replace->rwsem);
> +	down_write(&dev_replace->rwsem);
> +	dev_replace->replace_task = NULL;
> +
>  	/* was the operation canceled, or is it finished? */
>  	if (dev_replace->replace_state !=
>  	    BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
> -		up_read(&dev_replace->rwsem);
> +		up_write(&dev_replace->rwsem);
>  		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
>  		return 0;
>  	}
>
>  	tgt_device = dev_replace->tgtdev;
>  	src_device = dev_replace->srcdev;
> -	up_read(&dev_replace->rwsem);
> +	up_write(&dev_replace->rwsem);

Is clearing replace_task this early intentional?

At this point replace_state is still BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED,
and btrfs_start_delalloc_roots() plus the btrfs_start_transaction() /
btrfs_commit_transaction() loop are still ahead.  For all of that window the
replace task goes back to taking the rwsem in btrfs_map_block():

fs/btrfs/volumes.c:btrfs_map_block() {
	...
	if (dev_replace->replace_task != current)
		down_read(&dev_replace->rwsem);

	dev_replace_is_ongoing = btrfs_dev_replace_is_ongoing(dev_replace);
	...
}

which is the exemption 8cca35cb29f8 ("btrfs: don't take dev_replace rwsem on
task already holding it") added.

The down_write() further down already covers both the success exit and the
error: exit, so clearing replace_task next to the replace_state update there,
plus the three early returns, would leave the ongoing window unchanged.

The changelog says only:

>   - Reset replace_task to NULL for all paths of
>     btrfs_dev_replace_finishing()

Could it also mention that the read lock at the head of the function becomes
a write lock?


-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] btrfs: properly cleanup replace_task when the replace failed to start

[Qu Wenruo]

在 2026/8/19 21:58, Jeff Layton 写道:
> Claude had some more comments on this one:
> 
> This moves the dev_replace->replace_task assignment in
> btrfs_dev_replace_start() below the replace_state switch, clears it when
> btrfs_start_transaction() fails there, and clears it at the top of
> btrfs_dev_replace_finishing() so every exit from that function drops it.
> 
>> In the function btrfs_dev_replace_start(), we have several error paths
>> that assigns replace_task without reverting it back to NULL.
>>
>> There are two involved error paths:
>>
>> - There is already a running dev-replace
>>    Then replace_task is over-written to the current task.
>>    This is the one with long running effect.
>>
>> - The btrfs_start_transaction() call failed
>>    This is much harder to hit though.
> 
> [ ... ]
> 
>> Thankfully this bug is very hard to hit.
>>
>> As dev-replace is an exclusive operation, thus if there is already
>> a running replace, a new one will be rejected early without reaching
>> btrfs_dev_replace_start().
>>
>> The only remaining case is a suspended replace, which is much harder to
>> hit, e.g. requiring async dev-replace conflicting with another exclusive
>> operation, then a new replace is started.
> 
> Is "very hard to hit" still accurate now that btrfs_dev_replace_finishing()
> is fixed too?

That "very hard to hit" was for the old code before the fix, but indeed 
Claude found an extra path that cancel can always leave that stale 
replace_task.


> 
> A plain "btrfs replace cancel" leaves replace_task set every time.
> btrfs_dev_replace_cancel() calls btrfs_scrub_cancel() while replace_state is
> BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED, so btrfs_scrub_dev() returns
> -ECANCELED to btrfs_dev_replace_start(), which passes it straight into
> btrfs_dev_replace_finishing():
> 
> fs/btrfs/dev-replace.c:btrfs_dev_replace_start() {
> 	...
> 	ret = btrfs_scrub_dev(fs_info, src_device->devid, 0, ...);
> 
> 	ret = btrfs_dev_replace_finishing(fs_info, ret);
> 	...
> }
> 
> fs/btrfs/dev-replace.c:btrfs_dev_replace_finishing() {
> 	...
> 	} else {
> 		if (scrub_ret != -ECANCELED)
> 			btrfs_err(fs_info, ...);
> error:
> 		up_write(&dev_replace->rwsem);
> 		...
> 		return scrub_ret;
> 	}
> 	...
> }
> 
> That exit never reaches the replace_task = NULL at the end of the function,
> so the ioctl returns to userspace with replace_task still pointing at the
> task that ran it.  Any scrub failure, not only -ECANCELED, lands there as
> well.
> 
> The subject line says "when the replace failed to start", which does not
> cover the cancel or scrub-error case either.  Could the subject and the
> changelog describe those paths too?
> 
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 72cba7fed942..c5e67524b417 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
> 
> [ ... ]
> 
>> @@ -874,18 +875,20 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>   	/* don't allow cancel or unmount to disturb the finishing procedure */
>>   	mutex_lock(&dev_replace->lock_finishing_cancel_unmount);
>>
>> -	down_read(&dev_replace->rwsem);
>> +	down_write(&dev_replace->rwsem);
>> +	dev_replace->replace_task = NULL;
>> +
>>   	/* was the operation canceled, or is it finished? */
>>   	if (dev_replace->replace_state !=
>>   	    BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED) {
>> -		up_read(&dev_replace->rwsem);
>> +		up_write(&dev_replace->rwsem);
>>   		mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
>>   		return 0;
>>   	}
>>
>>   	tgt_device = dev_replace->tgtdev;
>>   	src_device = dev_replace->srcdev;
>> -	up_read(&dev_replace->rwsem);
>> +	up_write(&dev_replace->rwsem);
> 
> Is clearing replace_task this early intentional?

Yes.

The real work of dev-replace has all finished.
> 
> At this point replace_state is still BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED,
> and btrfs_start_delalloc_roots() plus the btrfs_start_transaction() /
> btrfs_commit_transaction() loop are still ahead.  For all of that window the
> replace task goes back to taking the rwsem in btrfs_map_block():
> 
> fs/btrfs/volumes.c:btrfs_map_block() {
> 	...
> 	if (dev_replace->replace_task != current)
> 		down_read(&dev_replace->rwsem);
> 
> 	dev_replace_is_ongoing = btrfs_dev_replace_is_ongoing(dev_replace);
> 	...
> }
> 
> which is the exemption 8cca35cb29f8 ("btrfs: don't take dev_replace rwsem on
> task already holding it") added.
> 
> The down_write() further down already covers both the success exit and the
> error: exit, so clearing replace_task next to the replace_state update there,
> plus the three early returns, would leave the ongoing window unchanged.

Alright, I guess the model you're using is digging deeper than opus.

It looks like we should not reset replace_task until the replace_state 
is also updated.

Or during the transaction commit, it will lead to the same problem.

We rely on btrfs_dev_replace_is_ongoing() to return false to release 
rwsem early and avoid the deadlock.

So the early reset leaves a window we can deadlock again.

Will send an update to this patch.

Thanks,
Qu

> 
> The changelog says only:
> 
>>    - Reset replace_task to NULL for all paths of
>>      btrfs_dev_replace_finishing()
> 
> Could it also mention that the read lock at the head of the function becomes
> a write lock?
> 
>