* [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
@ 2017-09-27 22:13 Liu Bo
2017-09-28 1:57 ` Guoqing Jiang
2017-11-03 16:13 ` Liu Bo
0 siblings, 2 replies; 6+ messages in thread
From: Liu Bo @ 2017-09-27 22:13 UTC (permalink / raw)
To: linux-block
MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
badblocks are disabled, otherwise, rdev_set_badblocks() will record
superblock changes and return success in that case and md will fail to
report an IO error which it should.
This bug has existed since badblocks were introduced in commit
9e0e252a048b ("badblocks: Add core badblock management code").
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
block/badblocks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 43c7116..91f7bcf 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -178,7 +178,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
if (bb->shift < 0)
/* badblocks are disabled */
- return 0;
+ return 1;
if (bb->shift) {
/* round the start down, and the end up */
--
2.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
2017-09-27 22:13 [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled Liu Bo
@ 2017-09-28 1:57 ` Guoqing Jiang
2017-09-28 18:45 ` Liu Bo
2017-11-03 16:13 ` Liu Bo
1 sibling, 1 reply; 6+ messages in thread
From: Guoqing Jiang @ 2017-09-28 1:57 UTC (permalink / raw)
To: Liu Bo, linux-block; +Cc: NeilBrown
On 09/28/2017 06:13 AM, Liu Bo wrote:
> MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
> badblocks are disabled, otherwise, rdev_set_badblocks() will record
> superblock changes and return success in that case and md will fail to
> report an IO error which it should.
>
> This bug has existed since badblocks were introduced in commit
> 9e0e252a048b ("badblocks: Add core badblock management code").
I don't think we need to change it since it originally was return 0 in
original
commit.
commit 2230dfe4ccc3add340dc6d437965b2de1d269fde
Author: NeilBrown <neilb@suse.de>
Date: Thu Jul 28 11:31:46 2011 +1000
md: beginnings of bad block management.
This the first step in allowing md to track bad-blocks per-device so
that we can fail individual blocks rather than the whole device.
This patch just adds a data structure for recording bad blocks, with
routines to add, remove, search the list.
Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Namhyung Kim <namhyung@gmail.com>
+static int md_set_badblocks(struct badblocks *bb, sector_t s, int sectors,
+ int acknowledged)
+{
+ u64 *p;
+ int lo, hi;
+ int rv = 1;
+
+ if (bb->shift < 0)
+ /* badblocks are disabled */
+ return 0;
After a quick glance, I guess we need to fix it inside md, since below
change
seems not correct in fc974ee2bffdde47d1e4b220cf326952cc2c4794.
@@ -8734,114 +8485,19 @@ int rdev_set_badblocks(struct md_rdev *rdev,
sector_t s, int sectors,
s += rdev->new_data_offset;
else
s += rdev->data_offset;
- rv = md_set_badblocks(&rdev->badblocks,
- s, sectors, 0);
- if (rv) {
+ rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
+ if (rv == 0) {
So we need to correct it like:
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0ff1bbf..245fe52 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8905,7 +8905,7 @@ int rdev_set_badblocks(struct md_rdev *rdev,
sector_t s, int sectors,
else
s += rdev->data_offset;
rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
- if (rv == 0) {
+ if (rv) {
Thanks,
Guoqing
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
2017-09-28 1:57 ` Guoqing Jiang
@ 2017-09-28 18:45 ` Liu Bo
2017-09-29 1:04 ` Guoqing Jiang
0 siblings, 1 reply; 6+ messages in thread
From: Liu Bo @ 2017-09-28 18:45 UTC (permalink / raw)
To: Guoqing Jiang; +Cc: linux-block, NeilBrown
On Thu, Sep 28, 2017 at 09:57:41AM +0800, Guoqing Jiang wrote:
>
>
> On 09/28/2017 06:13 AM, Liu Bo wrote:
> > MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
> > badblocks are disabled, otherwise, rdev_set_badblocks() will record
> > superblock changes and return success in that case and md will fail to
> > report an IO error which it should.
> >
> > This bug has existed since badblocks were introduced in commit
> > 9e0e252a048b ("badblocks: Add core badblock management code").
>
> I don't think we need to change it since it originally was return 0 in
> original
> commit.
The difference is that the original md_set_badblocks() returns 0 for
both "being disabled" and "no room", while now badblocks_set() returns
1 for "no room" as a failure but 0 for "being disabled".
thanks,
-liubo
>
> commit 2230dfe4ccc3add340dc6d437965b2de1d269fde
> Author: NeilBrown <neilb@suse.de>
> Date: Thu Jul 28 11:31:46 2011 +1000
>
> md: beginnings of bad block management.
>
> This the first step in allowing md to track bad-blocks per-device so
> that we can fail individual blocks rather than the whole device.
>
> This patch just adds a data structure for recording bad blocks, with
> routines to add, remove, search the list.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> Reviewed-by: Namhyung Kim <namhyung@gmail.com>
> +static int md_set_badblocks(struct badblocks *bb, sector_t s, int sectors,
> + int acknowledged)
> +{
> + u64 *p;
> + int lo, hi;
> + int rv = 1;
> +
> + if (bb->shift < 0)
> + /* badblocks are disabled */
> + return 0;
>
> After a quick glance, I guess we need to fix it inside md, since below
> change
> seems not correct in fc974ee2bffdde47d1e4b220cf326952cc2c4794.
>
> @@ -8734,114 +8485,19 @@ int rdev_set_badblocks(struct md_rdev *rdev,
> sector_t s, int sectors,
> s += rdev->new_data_offset;
> else
> s += rdev->data_offset;
> - rv = md_set_badblocks(&rdev->badblocks,
> - s, sectors, 0);
> - if (rv) {
> + rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
> + if (rv == 0) {
>
>
> So we need to correct it like:
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0ff1bbf..245fe52 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8905,7 +8905,7 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t
> s, int sectors,
> else
> s += rdev->data_offset;
> rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
> - if (rv == 0) {
> + if (rv) {
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
2017-09-28 18:45 ` Liu Bo
@ 2017-09-29 1:04 ` Guoqing Jiang
0 siblings, 0 replies; 6+ messages in thread
From: Guoqing Jiang @ 2017-09-29 1:04 UTC (permalink / raw)
To: bo.li.liu, Guoqing Jiang; +Cc: linux-block, NeilBrown
On 09/29/2017 02:45 AM, Liu Bo wrote:
> On Thu, Sep 28, 2017 at 09:57:41AM +0800, Guoqing Jiang wrote:
>>
>> On 09/28/2017 06:13 AM, Liu Bo wrote:
>>> MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
>>> badblocks are disabled, otherwise, rdev_set_badblocks() will record
>>> superblock changes and return success in that case and md will fail to
>>> report an IO error which it should.
>>>
>>> This bug has existed since badblocks were introduced in commit
>>> 9e0e252a048b ("badblocks: Add core badblock management code").
>> I don't think we need to change it since it originally was return 0 in
>> original
>> commit.
> The difference is that the original md_set_badblocks() returns 0 for
> both "being disabled" and "no room", while now badblocks_set() returns
> 1 for "no room" as a failure but 0 for "being disabled".
Thanks for point it out, you are right, and the below comments said:
* Return:
* 0: success
* 1: failed to set badblocks (out of space)
Maybe it is better to add "badblocks are disabled" to above comment.
Anyway, Acked-by: Guoqing Jiang <gqjiang@suse.com>
Regards,
Guoqing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
2017-09-27 22:13 [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled Liu Bo
2017-09-28 1:57 ` Guoqing Jiang
@ 2017-11-03 16:13 ` Liu Bo
2017-11-03 18:01 ` Shaohua Li
1 sibling, 1 reply; 6+ messages in thread
From: Liu Bo @ 2017-11-03 16:13 UTC (permalink / raw)
To: linux-block; +Cc: Shaohua Li
Hi Shaohua,
Given it's related to md, can you please take this thru your tree?
Thanks,
-liubo
On Wed, Sep 27, 2017 at 04:13:17PM -0600, Liu Bo wrote:
> MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
> badblocks are disabled, otherwise, rdev_set_badblocks() will record
> superblock changes and return success in that case and md will fail to
> report an IO error which it should.
>
> This bug has existed since badblocks were introduced in commit
> 9e0e252a048b ("badblocks: Add core badblock management code").
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> block/badblocks.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 43c7116..91f7bcf 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -178,7 +178,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>
> if (bb->shift < 0)
> /* badblocks are disabled */
> - return 0;
> + return 1;
>
> if (bb->shift) {
> /* round the start down, and the end up */
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled
2017-11-03 16:13 ` Liu Bo
@ 2017-11-03 18:01 ` Shaohua Li
0 siblings, 0 replies; 6+ messages in thread
From: Shaohua Li @ 2017-11-03 18:01 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-block, Shaohua Li
On Fri, Nov 03, 2017 at 10:13:38AM -0600, Liu Bo wrote:
> Hi Shaohua,
>
> Given it's related to md, can you please take this thru your tree?
Yes, the patch makes sense. Can you resend the patch to me? I can't find it in my inbox
Thanks,
Shaohua
> Thanks,
>
> -liubo
>
> On Wed, Sep 27, 2017 at 04:13:17PM -0600, Liu Bo wrote:
> > MD's rdev_set_badblocks() expects that badblocks_set() returns 1 if
> > badblocks are disabled, otherwise, rdev_set_badblocks() will record
> > superblock changes and return success in that case and md will fail to
> > report an IO error which it should.
> >
> > This bug has existed since badblocks were introduced in commit
> > 9e0e252a048b ("badblocks: Add core badblock management code").
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > block/badblocks.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/badblocks.c b/block/badblocks.c
> > index 43c7116..91f7bcf 100644
> > --- a/block/badblocks.c
> > +++ b/block/badblocks.c
> > @@ -178,7 +178,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> >
> > if (bb->shift < 0)
> > /* badblocks are disabled */
> > - return 0;
> > + return 1;
> >
> > if (bb->shift) {
> > /* round the start down, and the end up */
> > --
> > 2.9.4
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-03 18:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-27 22:13 [PATCH] badblocks: fix wrong return value in badblocks_set if badblocks are disabled Liu Bo
2017-09-28 1:57 ` Guoqing Jiang
2017-09-28 18:45 ` Liu Bo
2017-09-29 1:04 ` Guoqing Jiang
2017-11-03 16:13 ` Liu Bo
2017-11-03 18:01 ` Shaohua Li
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.