The variable async_registration is used to indicate whether or not
CONFIG_BCACHE_ASYNC_REGISTRATION is enabled, triggering a (false)
warning in Coverity about unreachable code. However, it seems clearer in
this case just to use an #ifdef, so let's do that instead.
Addresses-Coverity-ID: 1497746: Control flow issues (DEADCODE)
Signed-off-by: Alex Dewar <[email protected]>
---
drivers/md/bcache/super.c | 40 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 46a00134a36a..6d4127881c6a 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2504,11 +2504,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
struct cache_sb_disk *sb_disk;
struct block_device *bdev;
ssize_t ret;
- bool async_registration = false;
-
-#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
- async_registration = true;
-#endif
ret = -EBUSY;
err = "failed to reference bcache module";
@@ -2563,26 +2558,25 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
err = "failed to register device";
- if (async_registration) {
- /* register in asynchronous way */
- struct async_reg_args *args =
- kzalloc(sizeof(struct async_reg_args), GFP_KERNEL);
-
- if (!args) {
- ret = -ENOMEM;
- err = "cannot allocate memory";
- goto out_put_sb_page;
- }
+#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
+ /* register in asynchronous way */
+ struct async_reg_args *args =
+ kzalloc(sizeof(struct async_reg_args), GFP_KERNEL);
- args->path = path;
- args->sb = sb;
- args->sb_disk = sb_disk;
- args->bdev = bdev;
- register_device_aync(args);
- /* No wait and returns to user space */
- goto async_done;
+ if (!args) {
+ ret = -ENOMEM;
+ err = "cannot allocate memory";
+ goto out_put_sb_page;
}
+ args->path = path;
+ args->sb = sb;
+ args->sb_disk = sb_disk;
+ args->bdev = bdev;
+ register_device_aync(args);
+ /* No wait and returns to user space */
+ return size;
+#else
if (SB_IS_BDEV(sb)) {
struct cached_dev *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
@@ -2605,12 +2599,12 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (register_cache(sb, sb_disk, bdev, ca) != 0)
goto out_free_sb;
}
+#endif
done:
kfree(sb);
kfree(path);
module_put(THIS_MODULE);
-async_done:
return size;
out_put_sb_page:
--
2.28.0
On 09/10/2020 20.34, Alex Dewar wrote:
> The variable async_registration is used to indicate whether or not
> CONFIG_BCACHE_ASYNC_REGISTRATION is enabled, triggering a (false)
> warning in Coverity about unreachable code. However, it seems clearer in
> this case just to use an #ifdef, so let's do that instead.
>
> Addresses-Coverity-ID: 1497746: Control flow issues (DEADCODE)
I think that coverity check needs to be ignored. The kernel is full of
things that are supposed to be eliminated by the compiler, but still
checked for valid syntax etc. Often it's even more hidden than this,
something like
// some header
#ifdef CONFIG_FOO
int foo(void);
#else
static inline int foo(void) { return 0; }
#endif
// code
if (foo()) { ... // this goes away for CONFIG_FOO=n }
> Signed-off-by: Alex Dewar <[email protected]>
> ---
> drivers/md/bcache/super.c | 40 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 46a00134a36a..6d4127881c6a 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2504,11 +2504,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> struct cache_sb_disk *sb_disk;
> struct block_device *bdev;
> ssize_t ret;
> - bool async_registration = false;
> -
> -#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
> - async_registration = true;
> -#endif
If anything, this should simply be changed to
bool async_registration = IS_ENABLED(CONFIG_BCACHE_ASYNC_REGISTRATION);
Rasmus
On 2020/10/10 07:00, Rasmus Villemoes wrote:
> On 09/10/2020 20.34, Alex Dewar wrote:
>> The variable async_registration is used to indicate whether or not
>> CONFIG_BCACHE_ASYNC_REGISTRATION is enabled, triggering a (false)
>> warning in Coverity about unreachable code. However, it seems clearer in
>> this case just to use an #ifdef, so let's do that instead.
>>
>> Addresses-Coverity-ID: 1497746: Control flow issues (DEADCODE)
>
> I think that coverity check needs to be ignored. The kernel is full of
> things that are supposed to be eliminated by the compiler, but still
> checked for valid syntax etc. Often it's even more hidden than this,
> something like
>
> // some header
> #ifdef CONFIG_FOO
> int foo(void);
> #else
> static inline int foo(void) { return 0; }
> #endif
>
> // code
>
> if (foo()) { ... // this goes away for CONFIG_FOO=n }
>
>> Signed-off-by: Alex Dewar <[email protected]>
>> ---
>> drivers/md/bcache/super.c | 40 +++++++++++++++++----------------------
>> 1 file changed, 17 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index 46a00134a36a..6d4127881c6a 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -2504,11 +2504,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>> struct cache_sb_disk *sb_disk;
>> struct block_device *bdev;
>> ssize_t ret;
>> - bool async_registration = false;
>> -
>> -#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
>> - async_registration = true;
>> -#endif
>
> If anything, this should simply be changed to
>
> bool async_registration = IS_ENABLED(CONFIG_BCACHE_ASYNC_REGISTRATION);
>
> Rasmus
Hi Rasmus,
Yes, the above change might be better. But I don't suggest to spent
effort on this.
Hi Alex,
Indeed the code in v5.9 is quite similar to what your patch makes, and I
change it in this shape in v5.10 series. This piece of code may stay in
kernel for 2 or 3 versions at most, the purpose is to make it convenient
for people to test the async registration in production environment.
Once the new async registration behavior is verified to not break any
existing thing (which we don't know) it will be the (only) default
behavior and the CONFIG_BCACHE_ASYNC_REGISTRATION check will be removed.
Thank you all for looking at this.
Coly Li
On Sat, Oct 10, 2020 at 10:05:22AM +0800, Coly Li wrote:
> On 2020/10/10 07:00, Rasmus Villemoes wrote:
> > On 09/10/2020 20.34, Alex Dewar wrote:
> >> The variable async_registration is used to indicate whether or not
> >> CONFIG_BCACHE_ASYNC_REGISTRATION is enabled, triggering a (false)
> >> warning in Coverity about unreachable code. However, it seems clearer in
> >> this case just to use an #ifdef, so let's do that instead.
> >>
> >> Addresses-Coverity-ID: 1497746: Control flow issues (DEADCODE)
> >
> > I think that coverity check needs to be ignored. The kernel is full of
> > things that are supposed to be eliminated by the compiler, but still
> > checked for valid syntax etc. Often it's even more hidden than this,
> > something like
> >
> > // some header
> > #ifdef CONFIG_FOO
> > int foo(void);
> > #else
> > static inline int foo(void) { return 0; }
> > #endif
> >
> > // code
> >
> > if (foo()) { ... // this goes away for CONFIG_FOO=n }
> >
> >> Signed-off-by: Alex Dewar <[email protected]>
> >> ---
> >> drivers/md/bcache/super.c | 40 +++++++++++++++++----------------------
> >> 1 file changed, 17 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> >> index 46a00134a36a..6d4127881c6a 100644
> >> --- a/drivers/md/bcache/super.c
> >> +++ b/drivers/md/bcache/super.c
> >> @@ -2504,11 +2504,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> >> struct cache_sb_disk *sb_disk;
> >> struct block_device *bdev;
> >> ssize_t ret;
> >> - bool async_registration = false;
> >> -
> >> -#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
> >> - async_registration = true;
> >> -#endif
> >
> > If anything, this should simply be changed to
> >
> > bool async_registration = IS_ENABLED(CONFIG_BCACHE_ASYNC_REGISTRATION);
> >
> > Rasmus
>
> Hi Rasmus,
>
> Yes, the above change might be better. But I don't suggest to spent
> effort on this.
>
> Hi Alex,
>
> Indeed the code in v5.9 is quite similar to what your patch makes, and I
> change it in this shape in v5.10 series. This piece of code may stay in
> kernel for 2 or 3 versions at most, the purpose is to make it convenient
> for people to test the async registration in production environment.
> Once the new async registration behavior is verified to not break any
> existing thing (which we don't know) it will be the (only) default
> behavior and the CONFIG_BCACHE_ASYNC_REGISTRATION check will be removed.
>
> Thank you all for looking at this.
>
> Coly Li
Hi Coly,
That sounds sensible. There's not much point in introducing needless
churn.
Best,
Alex
Hi Alex,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20201009]
[cannot apply to linus/master v5.9-rc8 v5.9-rc7 v5.9-rc6 v5.9-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Alex-Dewar/bcache-Use-ifdef-instead-of-boolean-variable/20201010-023635
base: d67bc7812221606e1886620a357b13f906814af7
config: x86_64-randconfig-a004-20201009 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4cfc4025cc1433ca5ef1c526053fc9c4bfe64109)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/bb05520ea94f191cdee74e5b7829d0442a05eb86
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alex-Dewar/bcache-Use-ifdef-instead-of-boolean-variable/20201010-023635
git checkout bb05520ea94f191cdee74e5b7829d0442a05eb86
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/md/bcache/super.c:2563:25: warning: ISO C90 forbids mixing declarations and code [-Wdeclaration-after-statement]
struct async_reg_args *args =
^
1 warning generated.
vim +2563 drivers/md/bcache/super.c
9e23ccf8f0a22e Coly Li 2020-05-27 2497
cafe563591446c Kent Overstreet 2013-03-23 2498 static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
cafe563591446c Kent Overstreet 2013-03-23 2499 const char *buffer, size_t size)
cafe563591446c Kent Overstreet 2013-03-23 2500 {
50246693f81fe8 Christoph Hellwig 2020-01-24 2501 const char *err;
29cda393bcaad1 Coly Li 2020-01-24 2502 char *path = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2503 struct cache_sb *sb;
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2504 struct cache_sb_disk *sb_disk;
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2505 struct block_device *bdev;
50246693f81fe8 Christoph Hellwig 2020-01-24 2506 ssize_t ret;
cafe563591446c Kent Overstreet 2013-03-23 2507
50246693f81fe8 Christoph Hellwig 2020-01-24 2508 ret = -EBUSY;
29cda393bcaad1 Coly Li 2020-01-24 2509 err = "failed to reference bcache module";
cafe563591446c Kent Overstreet 2013-03-23 2510 if (!try_module_get(THIS_MODULE))
50246693f81fe8 Christoph Hellwig 2020-01-24 2511 goto out;
cafe563591446c Kent Overstreet 2013-03-23 2512
a59ff6ccc2bf2e Coly Li 2019-06-28 2513 /* For latest state of bcache_is_reboot */
a59ff6ccc2bf2e Coly Li 2019-06-28 2514 smp_mb();
29cda393bcaad1 Coly Li 2020-01-24 2515 err = "bcache is in reboot";
a59ff6ccc2bf2e Coly Li 2019-06-28 2516 if (bcache_is_reboot)
50246693f81fe8 Christoph Hellwig 2020-01-24 2517 goto out_module_put;
a59ff6ccc2bf2e Coly Li 2019-06-28 2518
50246693f81fe8 Christoph Hellwig 2020-01-24 2519 ret = -ENOMEM;
50246693f81fe8 Christoph Hellwig 2020-01-24 2520 err = "cannot allocate memory";
a56489d4b3c914 Florian Schmaus 2018-07-26 2521 path = kstrndup(buffer, size, GFP_KERNEL);
a56489d4b3c914 Florian Schmaus 2018-07-26 2522 if (!path)
50246693f81fe8 Christoph Hellwig 2020-01-24 2523 goto out_module_put;
a56489d4b3c914 Florian Schmaus 2018-07-26 2524
a56489d4b3c914 Florian Schmaus 2018-07-26 2525 sb = kmalloc(sizeof(struct cache_sb), GFP_KERNEL);
a56489d4b3c914 Florian Schmaus 2018-07-26 2526 if (!sb)
50246693f81fe8 Christoph Hellwig 2020-01-24 2527 goto out_free_path;
cafe563591446c Kent Overstreet 2013-03-23 2528
50246693f81fe8 Christoph Hellwig 2020-01-24 2529 ret = -EINVAL;
cafe563591446c Kent Overstreet 2013-03-23 2530 err = "failed to open device";
cafe563591446c Kent Overstreet 2013-03-23 2531 bdev = blkdev_get_by_path(strim(path),
cafe563591446c Kent Overstreet 2013-03-23 2532 FMODE_READ|FMODE_WRITE|FMODE_EXCL,
cafe563591446c Kent Overstreet 2013-03-23 2533 sb);
f59fce847fc848 Kent Overstreet 2013-05-15 2534 if (IS_ERR(bdev)) {
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2535 if (bdev == ERR_PTR(-EBUSY)) {
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2536 bdev = lookup_bdev(strim(path));
789d21dbd9d888 Jianjian Huo 2014-07-13 2537 mutex_lock(&bch_register_lock);
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2538 if (!IS_ERR(bdev) && bch_is_open(bdev))
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2539 err = "device already registered";
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2540 else
cafe563591446c Kent Overstreet 2013-03-23 2541 err = "device busy";
789d21dbd9d888 Jianjian Huo 2014-07-13 2542 mutex_unlock(&bch_register_lock);
4b758df21ee708 Jan Kara 2017-09-06 2543 if (!IS_ERR(bdev))
4b758df21ee708 Jan Kara 2017-09-06 2544 bdput(bdev);
d7076f21629f8f Gabriel de Perthuis 2015-11-29 2545 if (attr == &ksysfs_register_quiet)
50246693f81fe8 Christoph Hellwig 2020-01-24 2546 goto done;
a9dd53adbb84c1 Gabriel de Perthuis 2013-05-04 2547 }
50246693f81fe8 Christoph Hellwig 2020-01-24 2548 goto out_free_sb;
f59fce847fc848 Kent Overstreet 2013-05-15 2549 }
f59fce847fc848 Kent Overstreet 2013-05-15 2550
f59fce847fc848 Kent Overstreet 2013-05-15 2551 err = "failed to set blocksize";
f59fce847fc848 Kent Overstreet 2013-05-15 2552 if (set_blocksize(bdev, 4096))
50246693f81fe8 Christoph Hellwig 2020-01-24 2553 goto out_blkdev_put;
cafe563591446c Kent Overstreet 2013-03-23 2554
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2555 err = read_super(sb, bdev, &sb_disk);
cafe563591446c Kent Overstreet 2013-03-23 2556 if (err)
50246693f81fe8 Christoph Hellwig 2020-01-24 2557 goto out_blkdev_put;
cafe563591446c Kent Overstreet 2013-03-23 2558
cc40daf91bdddb Tang Junhui 2018-03-05 2559 err = "failed to register device";
a58e88bfdc4d52 Coly Li 2020-10-01 2560
bb05520ea94f19 Alex Dewar 2020-10-09 2561 #ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
9e23ccf8f0a22e Coly Li 2020-05-27 2562 /* register in asynchronous way */
9e23ccf8f0a22e Coly Li 2020-05-27 @2563 struct async_reg_args *args =
9e23ccf8f0a22e Coly Li 2020-05-27 2564 kzalloc(sizeof(struct async_reg_args), GFP_KERNEL);
9e23ccf8f0a22e Coly Li 2020-05-27 2565
9e23ccf8f0a22e Coly Li 2020-05-27 2566 if (!args) {
9e23ccf8f0a22e Coly Li 2020-05-27 2567 ret = -ENOMEM;
9e23ccf8f0a22e Coly Li 2020-05-27 2568 err = "cannot allocate memory";
9e23ccf8f0a22e Coly Li 2020-05-27 2569 goto out_put_sb_page;
9e23ccf8f0a22e Coly Li 2020-05-27 2570 }
9e23ccf8f0a22e Coly Li 2020-05-27 2571
9e23ccf8f0a22e Coly Li 2020-05-27 2572 args->path = path;
9e23ccf8f0a22e Coly Li 2020-05-27 2573 args->sb = sb;
9e23ccf8f0a22e Coly Li 2020-05-27 2574 args->sb_disk = sb_disk;
9e23ccf8f0a22e Coly Li 2020-05-27 2575 args->bdev = bdev;
9e23ccf8f0a22e Coly Li 2020-05-27 2576 register_device_aync(args);
9e23ccf8f0a22e Coly Li 2020-05-27 2577 /* No wait and returns to user space */
bb05520ea94f19 Alex Dewar 2020-10-09 2578 return size;
bb05520ea94f19 Alex Dewar 2020-10-09 2579 #else
2903381fce7100 Kent Overstreet 2013-04-11 2580 if (SB_IS_BDEV(sb)) {
cafe563591446c Kent Overstreet 2013-03-23 2581 struct cached_dev *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
1fae7cf05293d3 Coly Li 2018-08-11 2582
f59fce847fc848 Kent Overstreet 2013-05-15 2583 if (!dc)
50246693f81fe8 Christoph Hellwig 2020-01-24 2584 goto out_put_sb_page;
cafe563591446c Kent Overstreet 2013-03-23 2585
4fa03402cda2fa Kent Overstreet 2014-03-17 2586 mutex_lock(&bch_register_lock);
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2587 ret = register_bdev(sb, sb_disk, bdev, dc);
4fa03402cda2fa Kent Overstreet 2014-03-17 2588 mutex_unlock(&bch_register_lock);
bb6d355c2aff42 Coly Li 2019-04-25 2589 /* blkdev_put() will be called in cached_dev_free() */
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2590 if (ret < 0)
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2591 goto out_free_sb;
cafe563591446c Kent Overstreet 2013-03-23 2592 } else {
cafe563591446c Kent Overstreet 2013-03-23 2593 struct cache *ca = kzalloc(sizeof(*ca), GFP_KERNEL);
1fae7cf05293d3 Coly Li 2018-08-11 2594
f59fce847fc848 Kent Overstreet 2013-05-15 2595 if (!ca)
50246693f81fe8 Christoph Hellwig 2020-01-24 2596 goto out_put_sb_page;
cafe563591446c Kent Overstreet 2013-03-23 2597
bb6d355c2aff42 Coly Li 2019-04-25 2598 /* blkdev_put() will be called in bch_cache_release() */
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2599 if (register_cache(sb, sb_disk, bdev, ca) != 0)
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2600 goto out_free_sb;
50246693f81fe8 Christoph Hellwig 2020-01-24 2601 }
bb05520ea94f19 Alex Dewar 2020-10-09 2602 #endif
50246693f81fe8 Christoph Hellwig 2020-01-24 2603
50246693f81fe8 Christoph Hellwig 2020-01-24 2604 done:
f59fce847fc848 Kent Overstreet 2013-05-15 2605 kfree(sb);
f59fce847fc848 Kent Overstreet 2013-05-15 2606 kfree(path);
f59fce847fc848 Kent Overstreet 2013-05-15 2607 module_put(THIS_MODULE);
50246693f81fe8 Christoph Hellwig 2020-01-24 2608 return size;
f59fce847fc848 Kent Overstreet 2013-05-15 2609
50246693f81fe8 Christoph Hellwig 2020-01-24 2610 out_put_sb_page:
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2611 put_page(virt_to_page(sb_disk));
50246693f81fe8 Christoph Hellwig 2020-01-24 2612 out_blkdev_put:
cafe563591446c Kent Overstreet 2013-03-23 2613 blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
50246693f81fe8 Christoph Hellwig 2020-01-24 2614 out_free_sb:
50246693f81fe8 Christoph Hellwig 2020-01-24 2615 kfree(sb);
50246693f81fe8 Christoph Hellwig 2020-01-24 2616 out_free_path:
50246693f81fe8 Christoph Hellwig 2020-01-24 2617 kfree(path);
ae3cd299919af6 Coly Li 2020-01-24 2618 path = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2619 out_module_put:
50246693f81fe8 Christoph Hellwig 2020-01-24 2620 module_put(THIS_MODULE);
50246693f81fe8 Christoph Hellwig 2020-01-24 2621 out:
46f5aa8806e34f Joe Perches 2020-05-27 2622 pr_info("error %s: %s\n", path?path:"", err);
50246693f81fe8 Christoph Hellwig 2020-01-24 2623 return ret;
cafe563591446c Kent Overstreet 2013-03-23 2624 }
cafe563591446c Kent Overstreet 2013-03-23 2625
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]