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 <[email protected]>
Cc: Mikulas Patocka <[email protected]>
Cc: Alasdair Kergon <[email protected]>
Cc: Mike Snitzer <[email protected]>
Cc: [email protected]
Cc: Heiko Carstens <[email protected]>
Cc: Vasily Gorbik <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: [email protected]
---
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();
}
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 <[email protected]>
> Cc: Mikulas Patocka <[email protected]>
> Cc: Alasdair Kergon <[email protected]>
> Cc: Mike Snitzer <[email protected]>
> Cc: [email protected]
> Cc: Heiko Carstens <[email protected]>
> Cc: Vasily Gorbik <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Cc: [email protected]
> ---
> 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();
> }
>
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 <[email protected]>
>> Cc: Mikulas Patocka <[email protected]>
>> Cc: Alasdair Kergon <[email protected]>
>> Cc: Mike Snitzer <[email protected]>
>> Cc: [email protected]
>> Cc: Heiko Carstens <[email protected]>
>> Cc: Vasily Gorbik <[email protected]>
>> Cc: Christian Borntraeger <[email protected]>
>> Cc: [email protected]
>> ---
>> 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.
--
~Randy
On Mon, Nov 16 2020 at 6:00pm -0500,
Randy Dunlap <[email protected]> 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).
On Tue, Nov 17 2020 at 11:31am -0500,
Mike Snitzer <[email protected]> wrote:
> On Mon, Nov 16 2020 at 6:00pm -0500,
> Randy Dunlap <[email protected]> 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
On Wed, Nov 18 2020 at 10:49am -0500,
Mike Snitzer <[email protected]> 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
On 18.11.20 17:07, Mike Snitzer wrote:
> On Wed, Nov 18 2020 at 10:49am -0500,
> Mike Snitzer <[email protected]> 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.
On Wed, 18 Nov 2020, Mike Snitzer wrote:
> On Wed, Nov 18 2020 at 10:49am -0500,
> Mike Snitzer <[email protected]> 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 <[email protected]>
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: [email protected] # v4.18+
Reported-by: Randy Dunlap <[email protected]>
Signed-off-by: Mikulas Patocka <[email protected]>
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
On Wed, Nov 18 2020 at 4:24pm -0500,
Mikulas Patocka <[email protected]> wrote:
>
>
> On Wed, 18 Nov 2020, Mike Snitzer wrote:
>
> > On Wed, Nov 18 2020 at 10:49am -0500,
> > Mike Snitzer <[email protected]> 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.