[PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[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

Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[Mike Snitzer]

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

Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[Heinz Mauelshagen]

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 */

Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[Johannes Thumshirn]

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

Re: [PATCH RESEND] dm-raid: Do not call BUG() in __rdev_sectors()

[Heinz Mauelshagen]

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.

>