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

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 <[email protected]>