diff mbox series

md: dm-writeback: add __noreturn to BUG-ging function

Message ID 20201113225228.20563-1-rdunlap@infradead.org (mailing list archive)
State Accepted, archived
Delegated to: Mike Snitzer
Headers show
Series md: dm-writeback: add __noreturn to BUG-ging function | expand

Commit Message

Randy Dunlap Nov. 13, 2020, 10:52 p.m. UTC 
Building on arch/s390/ flags this as an error, so add the
__noreturn attribute modifier to prevent the build error.

cc1: some warnings being treated as errors
../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]

Fixes: 48debafe4f2f ("dm: add writecache target")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: linux-s390@vger.kernel.org
---
 drivers/md/dm-writecache.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Christian Borntraeger Nov. 16, 2020, 7:30 a.m. UTC | #1 
On 13.11.20 23:52, Randy Dunlap wrote:
> Building on arch/s390/ flags this as an error, so add the
> __noreturn attribute modifier to prevent the build error.
> 
> cc1: some warnings being treated as errors
> ../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
> ../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]

ok with me, but I am asking why

the unreachable macro is not good enough. For x86 it obviously is.

form arch/s390/include/asm/bug.h
#define BUG() do {                                      \
        __EMIT_BUG(0);                                  \
        unreachable();                                  \
} while (0)


> 
> Fixes: 48debafe4f2f ("dm: add writecache target")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: dm-devel@redhat.com
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
>  drivers/md/dm-writecache.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-next-20201113.orig/drivers/md/dm-writecache.c
> +++ linux-next-20201113/drivers/md/dm-writecache.c
> @@ -317,7 +317,7 @@ err1:
>  	return r;
>  }
>  #else
> -static int persistent_memory_claim(struct dm_writecache *wc)
> +static int __noreturn persistent_memory_claim(struct dm_writecache *wc)
>  {
>  	BUG();
>  }
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Randy Dunlap Nov. 16, 2020, 11 p.m. UTC | #2 
On 11/15/20 11:30 PM, Christian Borntraeger wrote:
> 
> 
> On 13.11.20 23:52, Randy Dunlap wrote:
>> Building on arch/s390/ flags this as an error, so add the
>> __noreturn attribute modifier to prevent the build error.
>>
>> cc1: some warnings being treated as errors
>> ../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
>> ../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]
> 
> ok with me, but I am asking why
> 
> the unreachable macro is not good enough. For x86 it obviously is.
> 
> form arch/s390/include/asm/bug.h
> #define BUG() do {                                      \
>         __EMIT_BUG(0);                                  \
>         unreachable();                                  \
> } while (0)
> 

Hi Christian,

Good question.
I don't see any guidance about when to use one or the other etc.

I see __noreturn being used 109 times and unreachable();
being used 33 times, but only now that I look at them.
That had nothing to do with why I used __noreturn in the patch.

> 
>>
>> Fixes: 48debafe4f2f ("dm: add writecache target")
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>> Cc: Mikulas Patocka <mpatocka@redhat.com>
>> Cc: Alasdair Kergon <agk@redhat.com>
>> Cc: Mike Snitzer <snitzer@redhat.com>
>> Cc: dm-devel@redhat.com
>> Cc: Heiko Carstens <hca@linux.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: linux-s390@vger.kernel.org
>> ---
>>  drivers/md/dm-writecache.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- linux-next-20201113.orig/drivers/md/dm-writecache.c
>> +++ linux-next-20201113/drivers/md/dm-writecache.c
>> @@ -317,7 +317,7 @@ err1:
>>  	return r;
>>  }
>>  #else
>> -static int persistent_memory_claim(struct dm_writecache *wc)
>> +static int __noreturn persistent_memory_claim(struct dm_writecache *wc)
>>  {
>>  	BUG();
>>  }
>>

thanks.
Mike Snitzer Nov. 17, 2020, 4:31 p.m. UTC | #3 
On Mon, Nov 16 2020 at  6:00pm -0500,
Randy Dunlap <rdunlap@infradead.org> wrote:

> On 11/15/20 11:30 PM, Christian Borntraeger wrote:
> > 
> > 
> > On 13.11.20 23:52, Randy Dunlap wrote:
> >> Building on arch/s390/ flags this as an error, so add the
> >> __noreturn attribute modifier to prevent the build error.
> >>
> >> cc1: some warnings being treated as errors
> >> ../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
> >> ../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]
> > 
> > ok with me, but I am asking why
> > 
> > the unreachable macro is not good enough. For x86 it obviously is.
> > 
> > form arch/s390/include/asm/bug.h
> > #define BUG() do {                                      \
> >         __EMIT_BUG(0);                                  \
> >         unreachable();                                  \
> > } while (0)
> > 
> 
> Hi Christian,
> 
> Good question.
> I don't see any guidance about when to use one or the other etc.
> 
> I see __noreturn being used 109 times and unreachable();
> being used 33 times, but only now that I look at them.
> That had nothing to do with why I used __noreturn in the patch.

But doesn't that speak to the proper fix being needed in unreachable()?
Or at a minimum the fix is needed to arch/s390/include/asm/bug.h's BUG.

I really don't think we should be papering over that by sprinkling
__noreturn around the kernel's BUG() callers.

Maybe switch arch/s390/include/asm/bug.h's BUG to be like
arch/mips/include/asm/bug.h?  It itself uses __noreturn with a 'static
inline' function definition rather than #define.

Does that fix the issue?

Thanks,
Mike

p.s. you modified dm-writecache.c (not dm-writeback, wich doesn't
exist).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer Nov. 18, 2020, 3:49 p.m. UTC | #4 
On Tue, Nov 17 2020 at 11:31am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Nov 16 2020 at  6:00pm -0500,
> Randy Dunlap <rdunlap@infradead.org> wrote:
> 
> > On 11/15/20 11:30 PM, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 13.11.20 23:52, Randy Dunlap wrote:
> > >> Building on arch/s390/ flags this as an error, so add the
> > >> __noreturn attribute modifier to prevent the build error.
> > >>
> > >> cc1: some warnings being treated as errors
> > >> ../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
> > >> ../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]
> > > 
> > > ok with me, but I am asking why
> > > 
> > > the unreachable macro is not good enough. For x86 it obviously is.
> > > 
> > > form arch/s390/include/asm/bug.h
> > > #define BUG() do {                                      \
> > >         __EMIT_BUG(0);                                  \
> > >         unreachable();                                  \
> > > } while (0)
> > > 
> > 
> > Hi Christian,
> > 
> > Good question.
> > I don't see any guidance about when to use one or the other etc.
> > 
> > I see __noreturn being used 109 times and unreachable();
> > being used 33 times, but only now that I look at them.
> > That had nothing to do with why I used __noreturn in the patch.
> 
> But doesn't that speak to the proper fix being needed in unreachable()?
> Or at a minimum the fix is needed to arch/s390/include/asm/bug.h's BUG.
> 
> I really don't think we should be papering over that by sprinkling
> __noreturn around the kernel's BUG() callers.
> 
> Maybe switch arch/s390/include/asm/bug.h's BUG to be like
> arch/mips/include/asm/bug.h?  It itself uses __noreturn with a 'static
> inline' function definition rather than #define.
> 
> Does that fix the issue?
> 
> Thanks,
> Mike
> 
> p.s. you modified dm-writecache.c (not dm-writeback, wich doesn't
> exist).

I don't think my suggestion will help.. given it'd still leave
persistent_memory_claim() without a return statement.

Think it worthwhile to just add a dummy 'return 0;' after the BUG().

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer Nov. 18, 2020, 4:07 p.m. UTC | #5 
On Wed, Nov 18 2020 at 10:49am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> I don't think my suggestion will help.. given it'd still leave
> persistent_memory_claim() without a return statement.
> 
> Think it worthwhile to just add a dummy 'return 0;' after the BUG().

Decided to go with this, now staged for 5.11:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.11&id=a1e4865b4dda7071f3707f7e551289ead66e38b1

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christian Borntraeger Nov. 18, 2020, 4:35 p.m. UTC | #6 
On 18.11.20 17:07, Mike Snitzer wrote:
> On Wed, Nov 18 2020 at 10:49am -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> I don't think my suggestion will help.. given it'd still leave
>> persistent_memory_claim() without a return statement.
>>
>> Think it worthwhile to just add a dummy 'return 0;' after the BUG().
> 
> Decided to go with this, now staged for 5.11:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.11&id=a1e4865b4dda7071f3707f7e551289ead66e38b1
> 

Looks like a sane solution. Thank you for following up.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Randy Dunlap Nov. 18, 2020, 4:38 p.m. UTC | #7 
On 11/18/20 8:35 AM, Christian Borntraeger wrote:
> 
> 
> On 18.11.20 17:07, Mike Snitzer wrote:
>> On Wed, Nov 18 2020 at 10:49am -0500,
>> Mike Snitzer <snitzer@redhat.com> wrote:
>>
>>> I don't think my suggestion will help.. given it'd still leave
>>> persistent_memory_claim() without a return statement.
>>>
>>> Think it worthwhile to just add a dummy 'return 0;' after the BUG().
>>
>> Decided to go with this, now staged for 5.11:
>> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.11&id=a1e4865b4dda7071f3707f7e551289ead66e38b1
>>
> 
> Looks like a sane solution. Thank you for following up.

Yes, thanks again.
Mikulas Patocka Nov. 18, 2020, 9:24 p.m. UTC | #8 
On Wed, 18 Nov 2020, Mike Snitzer wrote:

> On Wed, Nov 18 2020 at 10:49am -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > I don't think my suggestion will help.. given it'd still leave
> > persistent_memory_claim() without a return statement.
> > 
> > Think it worthwhile to just add a dummy 'return 0;' after the BUG().
> 
> Decided to go with this, now staged for 5.11:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.11&id=a1e4865b4dda7071f3707f7e551289ead66e38b1

Hi

I would just use "return -EOPNOTSUPP;" and drop the "#ifdef 
DM_WRITECACHE_HAS_PMEM" that you added.

That BUG/return -EOPNOTSUPP code can't happen at all - if 
DM_WRITECACHE_HAS_PMEM is not defined, WC_MODE_PMEM(wc) always returns 
false - so persistent_memory_claim and BUG() can't ever be called. And if 
it can't be called, you don't need to add a code that prints an error in 
that case.

If we don't have DM_WRITECACHE_HAS_PMEM, the compiler optimizer will 
remove all the code guarded with if (WC_MODE_PMEM(wc)) as unreachable.

Mikulas


From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] dm writecache: remove BUG() and fail gracefully insteadfor-nextdm-5.11

Building on arch/s390/ results in this build error:

cc1: some warnings being treated as errors
../drivers/md/dm-writecache.c: In function 'persistent_memory_claim':
../drivers/md/dm-writecache.c:323:1: error: no return statement in function returning non-void [-Werror=return-type]

Fix this by replacing the BUG() with a -EOPNOTSUPP return.

Fixes: 48debafe4f2f ("dm: add writecache target")
Cc: stable@vger.kernel.org	# v4.18+
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Index: linux-2.6/drivers/md/dm-writecache.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-writecache.c
+++ linux-2.6/drivers/md/dm-writecache.c
@@ -319,7 +319,7 @@ err1:
 #else
 static int persistent_memory_claim(struct dm_writecache *wc)
 {
-	BUG();
+	return -EOPNOTSUPP;
 }
 #endif
 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer Nov. 20, 2020, 2:08 p.m. UTC | #9 
On Wed, Nov 18 2020 at  4:24pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 18 Nov 2020, Mike Snitzer wrote:
> 
> > On Wed, Nov 18 2020 at 10:49am -0500,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > I don't think my suggestion will help.. given it'd still leave
> > > persistent_memory_claim() without a return statement.
> > > 
> > > Think it worthwhile to just add a dummy 'return 0;' after the BUG().
> > 
> > Decided to go with this, now staged for 5.11:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.11&id=a1e4865b4dda7071f3707f7e551289ead66e38b1
> 
> Hi
> 
> I would just use "return -EOPNOTSUPP;" and drop the "#ifdef 
> DM_WRITECACHE_HAS_PMEM" that you added.
> 
> That BUG/return -EOPNOTSUPP code can't happen at all - if 
> DM_WRITECACHE_HAS_PMEM is not defined, WC_MODE_PMEM(wc) always returns 
> false - so persistent_memory_claim and BUG() can't ever be called. And if 
> it can't be called, you don't need to add a code that prints an error in 
> that case.
> 
> If we don't have DM_WRITECACHE_HAS_PMEM, the compiler optimizer will 
> remove all the code guarded with if (WC_MODE_PMEM(wc)) as unreachable.
> 
> Mikulas

Fair enough.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

--- linux-next-20201113.orig/drivers/md/dm-writecache.c
+++ linux-next-20201113/drivers/md/dm-writecache.c
@@ -317,7 +317,7 @@  err1:
 	return r;
 }
 #else
-static int persistent_memory_claim(struct dm_writecache *wc)
+static int __noreturn persistent_memory_claim(struct dm_writecache *wc)
 {
 	BUG();
 }