Re: [PATCH 5/9] btrfs: replace cancel is successful if scrub cancel is successful

[Nikolay Borisov <nborisov@suse.com>]

On 7.11.18 г. 13:43 ч., Anand Jain wrote:
> In btrfs_dev_replace_cancel() we should check if the
> btrfs_scrub_cancel() is successful. If the btrfs_scrub_cancel() fails, return
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED so that user can try to
> cancel the replace again.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 90c124edec76..c092ed559714 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -799,18 +799,22 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
>  		btrfs_dev_replace_write_unlock(dev_replace);
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
> -		result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
>  		tgt_device = dev_replace->tgtdev;
>  		src_device = dev_replace->srcdev;
>  		btrfs_dev_replace_write_unlock(dev_replace);
> -		btrfs_scrub_cancel(fs_info);
> -		/*
> -		 * btrfs_dev_replace_finishing() will handle the cleanup part
> -		 */
> -		btrfs_info_in_rcu(fs_info,
> -			"dev_replace from %s (devid %llu) to %s canceled",
> -			btrfs_dev_name(src_device), src_device->devid,
> -			btrfs_dev_name(tgt_device));
> +		ret = btrfs_scrub_cancel(fs_info);
> +		if (ret) {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED;
> +		} else {
> +			result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR;
> +			/*
> +			 * btrfs_dev_replace_finishing() will handle the cleanup part
> +			 */
> +			btrfs_info_in_rcu(fs_info,
> +				"dev_replace from %s (devid %llu) to %s canceled",
> +				btrfs_dev_name(src_device), src_device->devid,
> +				btrfs_dev_name(tgt_device));

This is identical to the btrfs_info_in_rcu several lines further down.
So if btrfs_scrub_cancel is successful you will print this messages
twice. Furthermore, there is already an unconditinal call to
btrfs_scrub_cancel. You are duplicating this function, this definitely
needs another look...

> +		}
>  		break;
>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>  		/*
>