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

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2026-08-20 at 07:44 +0930, Qu Wenruo wrote:
> 
> 在 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.
> 

Actually, this was Opus 5. I find that there is a fair amount of
randomness with these LLMs. Sometimes it just gets lucky!

> 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.
> 

Sounds good.

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