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

[Nikolay Borisov <nborisov@suse.com>]

On 7.11.18 г. 14:25 ч., Nikolay Borisov wrote:
> 
> 
> 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...

Ah, it builds on top of the previous patch which I still haven't
reviewed. So ignore this, my bad.
> 
>> +		}
>>  		break;
>>  	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
>>  		/*
>>