* [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed
@ 2014-09-25 10:28 Eryu Guan
2014-10-10 7:13 ` Eryu Guan
0 siblings, 1 reply; 5+ messages in thread
From: Eryu Guan @ 2014-09-25 10:28 UTC (permalink / raw)
To: linux-btrfs; +Cc: Eryu Guan
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@gmail.com>
---
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);
- return 0;
+ return ret;
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed
2014-09-25 10:28 [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed Eryu Guan
@ 2014-10-10 7:13 ` Eryu Guan
2014-10-10 8:24 ` Miao Xie
0 siblings, 1 reply; 5+ messages in thread
From: Eryu Guan @ 2014-10-10 7:13 UTC (permalink / raw)
To: linux-btrfs
On Thu, Sep 25, 2014 at 06:28:14PM +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@gmail.com>
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);
>
> - return 0;
> + return ret;
>
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed
2014-10-10 7:13 ` Eryu Guan
@ 2014-10-10 8:24 ` Miao Xie
2014-10-11 6:45 ` Eryu Guan
0 siblings, 1 reply; 5+ messages in thread
From: Miao Xie @ 2014-10-10 8:24 UTC (permalink / raw)
To: Eryu Guan, linux-btrfs
On Fri, 10 Oct 2014 15:13:31 +0800, Eryu Guan wrote:
> On Thu, Sep 25, 2014 at 06:28:14PM +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@gmail.com>
>
> 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).
>>
>> - 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.
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed
2014-10-10 8:24 ` Miao Xie
@ 2014-10-11 6:45 ` Eryu Guan
2014-10-13 1:41 ` Miao Xie
0 siblings, 1 reply; 5+ messages in thread
From: Eryu Guan @ 2014-10-11 6:45 UTC (permalink / raw)
To: Miao Xie; +Cc: linux-btrfs
On Fri, Oct 10, 2014 at 04:24:26PM +0800, Miao Xie wrote:
> On Fri, 10 Oct 2014 15:13:31 +0800, Eryu Guan wrote:
> > On Thu, Sep 25, 2014 at 06:28:14PM +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@gmail.com>
> >
> > 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)
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.
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@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed
2014-10-11 6:45 ` Eryu Guan
@ 2014-10-13 1:41 ` Miao Xie
0 siblings, 0 replies; 5+ messages in thread
From: Miao Xie @ 2014-10-13 1:41 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-btrfs
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@gmail.com>
>>>
>>> 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@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-13 1:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-25 10:28 [PATCH] Btrfs: return failure if btrfs_dev_replace_finishing() failed Eryu Guan
2014-10-10 7:13 ` Eryu Guan
2014-10-10 8:24 ` Miao Xie
2014-10-11 6:45 ` Eryu Guan
2014-10-13 1:41 ` Miao Xie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.