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.j...@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:
>> /*
>>
