* [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()
@ 2017-06-28 8:04 Hannes Reinecke
2017-06-28 17:18 ` Mike Snitzer
2017-06-29 14:09 ` Heinz Mauelshagen
0 siblings, 2 replies; 5+ messages in thread
From: Hannes Reinecke @ 2017-06-28 8:04 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, Hannes Reinecke
__rdev_sectors() might be called for an invalid/non-existing
RAID set during raid_ctr(), which is perfectly fine and no
reason to panic.
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
drivers/md/dm-raid.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 7d89322..9bbb596 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs)
return rdev->sectors;
}
- BUG(); /* Constructor ensures we got some. */
+ /* No valid raid devices */
+ return 0;
}
/* Calculate the sectors per device and per array used for @rs */
--
1.8.5.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()
2017-06-28 8:04 [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors() Hannes Reinecke
@ 2017-06-28 17:18 ` Mike Snitzer
2017-06-29 14:09 ` Heinz Mauelshagen
1 sibling, 0 replies; 5+ messages in thread
From: Mike Snitzer @ 2017-06-28 17:18 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Heinz Mauelshagen, dm-devel
On Wed, Jun 28 2017 at 4:04P -0400,
Hannes Reinecke <hare@suse.de> wrote:
> __rdev_sectors() might be called for an invalid/non-existing
> RAID set during raid_ctr(), which is perfectly fine and no
> reason to panic.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/md/dm-raid.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 7d89322..9bbb596 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs)
> return rdev->sectors;
> }
>
> - BUG(); /* Constructor ensures we got some. */
> + /* No valid raid devices */
> + return 0;
> }
>
> /* Calculate the sectors per device and per array used for @rs */
> --
> 1.8.5.6
>
The use of BUG() is certainly suspect. BUG() is very rarely the right
answer.
But I don't think returning 0 makes sense for all __rdev_sectors()
callers, e.g.: rs_setup_recovery()
__rdev_sectors() could easily be made to check if ti->error is passed as
a non-NULL pointer.. if so set *error, return 0, have ctr check for 0,
and gracefully fail ctr by returning the ti->error that __rdev_sectors()
set. If the error pointer passed to __rdev_sectors() is NULL, resort to
BUG() still? Would really like to avoid BUG().. but I'll defer to Heinz
on what might be a better response.
Alternatively, you could look to see where "Constructor ensures we got
some." and fix the fact that it clearly isn't ensuring as much for your
case?
Mike
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()
2017-06-28 8:04 [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors() Hannes Reinecke
2017-06-28 17:18 ` Mike Snitzer
@ 2017-06-29 14:09 ` Heinz Mauelshagen
2017-06-30 7:33 ` Johannes Thumshirn
1 sibling, 1 reply; 5+ messages in thread
From: Heinz Mauelshagen @ 2017-06-29 14:09 UTC (permalink / raw)
To: dm-devel
Hi Hannes,
thanks for your patch.
BUG() is intentional there to catch what I've already fixed with patch
"[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended
superblock format"
on 06/23/2017.
IOW: returning 0 is never right and would hide bugs like that one fixed.
Heinz
On 06/28/2017 10:04 AM, Hannes Reinecke wrote:
> __rdev_sectors() might be called for an invalid/non-existing
> RAID set during raid_ctr(), which is perfectly fine and no
> reason to panic.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/md/dm-raid.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 7d89322..9bbb596 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs)
> return rdev->sectors;
> }
>
> - BUG(); /* Constructor ensures we got some. */
> + /* No valid raid devices */
> + return 0;
> }
>
> /* Calculate the sectors per device and per array used for @rs */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()
2017-06-29 14:09 ` Heinz Mauelshagen
@ 2017-06-30 7:33 ` Johannes Thumshirn
2017-06-30 12:33 ` Heinz Mauelshagen
0 siblings, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2017-06-30 7:33 UTC (permalink / raw)
To: Heinz Mauelshagen; +Cc: dm-devel
On Thu, Jun 29, 2017 at 04:09:38PM +0200, Heinz Mauelshagen wrote:
>
> Hi Hannes,
>
> thanks for your patch.
>
> BUG() is intentional there to catch what I've already fixed with patch
>
> "[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended superblock
> format"
>
> on 06/23/2017.
>
> IOW: returning 0 is never right and would hide bugs like that one fixed.
OTOH panicing systems to catch errors which could've been mitigted is
never right as well. Not a single time. Never. BUG() and BUG_ON() are not
for error handling.
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()
2017-06-30 7:33 ` Johannes Thumshirn
@ 2017-06-30 12:33 ` Heinz Mauelshagen
0 siblings, 0 replies; 5+ messages in thread
From: Heinz Mauelshagen @ 2017-06-30 12:33 UTC (permalink / raw)
To: Johannes Thumshirn; +Cc: dm-devel
On 06/30/2017 09:33 AM, Johannes Thumshirn wrote:
> On Thu, Jun 29, 2017 at 04:09:38PM +0200, Heinz Mauelshagen wrote:
>> Hi Hannes,
>>
>> thanks for your patch.
>>
>> BUG() is intentional there to catch what I've already fixed with patch
>>
>> "[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended superblock
>> format"
>>
>> on 06/23/2017.
>>
>> IOW: returning 0 is never right and would hide bugs like that one fixed.
> OTOH panicing systems to catch errors which could've been mitigted is
> never right as well. Not a single time. Never. BUG() and BUG_ON() are not
> for error handling.
Point taken. We'll change that.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-30 12:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-28 8:04 [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors() Hannes Reinecke
2017-06-28 17:18 ` Mike Snitzer
2017-06-29 14:09 ` Heinz Mauelshagen
2017-06-30 7:33 ` Johannes Thumshirn
2017-06-30 12:33 ` Heinz Mauelshagen
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.