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

在 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?
> 
>