Guan
On Sat, 11 Oct 2014 14:45:29 +0800, Eryu Guan wrote:
>>>> device replace could fail due to another running scrub process, but this
>>>> failure doesn't get returned to userspace.
>>>>
>>>> The following steps could reproduce this issue
>>>>
>>>> mkfs -t btrfs -f /dev/sdb1 /dev/sdb2
>>>> mount /dev/sdb1 /mnt/btrfs
>>>> while true; do
>>>> btrfs scrub start -B /mnt/btrfs >/dev/null 2>&1
>>>> done &
>>>> btrfs replace start -Bf /dev/sdb2 /dev/sdb3 /mnt/btrfs
>>>> # if this replace succeeded, do the following and repeat until
>>>> # you see this log in dmesg
>>>> # BTRFS: btrfs_scrub_dev(/dev/sdb2, 2, /dev/sdb3) failed -115
>>>> #btrfs replace start -Bf /dev/sdb3 /dev/sdb2 /mnt/btrfs
>>>>
>>>> # once you see the error log in dmesg, check return value of
>>>> # replace
>>>> echo $?
>>>>
>>>> Also only WARN_ON if the return code is not -EINPROGRESS.
>>>>
>>>> Signed-off-by: Eryu Guan <guaneryu@xxxxxxxxx>
>>>
>>> Ping, any comments on this patch?
>>>
>>> Thanks,
>>> Eryu
>>>> ---
>>>> fs/btrfs/dev-replace.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>>> index eea26e1..44d32ab 100644
>>>> --- a/fs/btrfs/dev-replace.c
>>>> +++ b/fs/btrfs/dev-replace.c
>>>> @@ -418,9 +418,11 @@ int btrfs_dev_replace_start(struct btrfs_root *root,
>>>> &dev_replace->scrub_progress, 0, 1);
>>>>
>>>> ret = btrfs_dev_replace_finishing(root->fs_info, ret);
>>>> - WARN_ON(ret);
>>>> + /* don't warn if EINPROGRESS, someone else might be running scrub */
>>>> + if (ret != -EINPROGRESS)
>>>> + WARN_ON(ret);
>>
>> picky comment
>>
>> I prefer WARN_ON(ret && ret != -EINPROGRESS).
>
> Yes, this is simpler :)
>>
>>>>
>>>> - return 0;
>>>> + return ret;
>>
>> here we will return -EINPROGRESS if scrub is running, I think it better that
>> we assign some special number to args->result, and then return 0, just like
>> the case the device replace is running.
>
> Seems that requires a new result type, say,
>
> #define BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS 3
>
> and assign this result to args->result if btrfs_scrub_dev() returned -EINPROGRESS
>
> But I don't think returning 0 unconditionally is a good idea, since
> btrfs_dev_replace_finishing() could return other errors too, that way
> these errors will be lost, and userspace still won't catch the
> errors ($? is 0)
Of course.
Maybe the above explanation of mine was not so clear. In fact, I just talked about
the EINPROGRESS case, for the other case, returning error code is better.
> What I'm thinking about is something like:
>
> ret = btrfs_scrub_dev(...);
> ret = btrfs_dev_replace_finishing(root->fs_info, ret);
> if (ret == -EINPROGRESS) {
> args->result = BTRFS_IOCTL_DEV_REPLACE_RESULT_SCRUB_INPROGRESS;
> ret = 0;
> } else {
> WARN_ON(ret);
> }
>
> return ret;
>
> What do you think? If no objection I'll work on v2.
I like it.
Thanks
Miao
> Thanks for your review!
>
> Eryu
>>
>> Thanks
>> Miao
>>
>>>>
>>>> leave:
>>>> dev_replace->srcdev = NULL;
>>>> @@ -538,7 +540,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>>> btrfs_destroy_dev_replace_tgtdev(fs_info, tgt_device);
>>>> mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
>>>>
>>>> - return 0;
>>>> + return scrub_ret;
>>>> }
>>>>
>>>> printk_in_rcu(KERN_INFO
>>>> --
>>>> 1.8.3.1
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
