* [PATCH 0/2] bcache: Fix block device claiming
@ 2023-06-21 16:23 Jan Kara
2023-06-21 16:23 ` [PATCH 1/2] bcache: Alloc holder object before async registration Jan Kara
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
0 siblings, 2 replies; 12+ messages in thread
From: Jan Kara @ 2023-06-21 16:23 UTC (permalink / raw
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, Coly Li, linux-bcache, Jan Kara
Hello,
these two patches fix block device claiming for bcache broken by recent
Christoph's changes to blkdev_get_*() functions and also cleans up the layering
violation inside bcache which was the underlying cause of the breakage.
Honza
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] bcache: Alloc holder object before async registration
2023-06-21 16:23 [PATCH 0/2] bcache: Fix block device claiming Jan Kara
@ 2023-06-21 16:23 ` Jan Kara
2023-06-21 17:56 ` Kent Overstreet
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
1 sibling, 1 reply; 12+ messages in thread
From: Jan Kara @ 2023-06-21 16:23 UTC (permalink / raw
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, Coly Li, linux-bcache, Jan Kara
Allocate holder object (cache or cached_dev) before offloading the
rest of the startup to async work. This will allow us to open the block
block device with proper holder.
Signed-off-by: Jan Kara <jack@suse.cz>
---
drivers/md/bcache/super.c | 66 +++++++++++++++------------------------
1 file changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e2a803683105..913dd94353b6 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2448,6 +2448,7 @@ struct async_reg_args {
struct cache_sb *sb;
struct cache_sb_disk *sb_disk;
struct block_device *bdev;
+ void *holder;
};
static void register_bdev_worker(struct work_struct *work)
@@ -2455,22 +2456,13 @@ static void register_bdev_worker(struct work_struct *work)
int fail = false;
struct async_reg_args *args =
container_of(work, struct async_reg_args, reg_work.work);
- struct cached_dev *dc;
-
- dc = kzalloc(sizeof(*dc), GFP_KERNEL);
- if (!dc) {
- fail = true;
- put_page(virt_to_page(args->sb_disk));
- blkdev_put(args->bdev, bcache_kobj);
- goto out;
- }
mutex_lock(&bch_register_lock);
- if (register_bdev(args->sb, args->sb_disk, args->bdev, dc) < 0)
+ if (register_bdev(args->sb, args->sb_disk, args->bdev, args->holder)
+ < 0)
fail = true;
mutex_unlock(&bch_register_lock);
-out:
if (fail)
pr_info("error %s: fail to register backing device\n",
args->path);
@@ -2485,21 +2477,11 @@ static void register_cache_worker(struct work_struct *work)
int fail = false;
struct async_reg_args *args =
container_of(work, struct async_reg_args, reg_work.work);
- struct cache *ca;
-
- ca = kzalloc(sizeof(*ca), GFP_KERNEL);
- if (!ca) {
- fail = true;
- put_page(virt_to_page(args->sb_disk));
- blkdev_put(args->bdev, bcache_kobj);
- goto out;
- }
/* blkdev_put() will be called in bch_cache_release() */
- if (register_cache(args->sb, args->sb_disk, args->bdev, ca) != 0)
+ if (register_cache(args->sb, args->sb_disk, args->bdev, args->holder))
fail = true;
-out:
if (fail)
pr_info("error %s: fail to register cache device\n",
args->path);
@@ -2520,6 +2502,13 @@ static void register_device_async(struct async_reg_args *args)
queue_delayed_work(system_wq, &args->reg_work, 10);
}
+static void *alloc_holder_object(struct cache_sb *sb)
+{
+ if (SB_IS_BDEV(sb))
+ return kzalloc(sizeof(struct cached_dev), GFP_KERNEL);
+ return kzalloc(sizeof(struct cache), GFP_KERNEL);
+}
+
static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
const char *buffer, size_t size)
{
@@ -2528,6 +2517,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
struct cache_sb *sb;
struct cache_sb_disk *sb_disk;
struct block_device *bdev;
+ void *holder;
ssize_t ret;
bool async_registration = false;
@@ -2585,6 +2575,13 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (err)
goto out_blkdev_put;
+ holder = alloc_holder_object(sb);
+ if (!holder) {
+ ret = -ENOMEM;
+ err = "cannot allocate memory";
+ goto out_put_sb_page;
+ }
+
err = "failed to register device";
if (async_registration) {
@@ -2595,44 +2592,29 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (!args) {
ret = -ENOMEM;
err = "cannot allocate memory";
- goto out_put_sb_page;
+ goto out_free_holder;
}
args->path = path;
args->sb = sb;
args->sb_disk = sb_disk;
args->bdev = bdev;
+ args->holder = holder;
register_device_async(args);
/* No wait and returns to user space */
goto async_done;
}
if (SB_IS_BDEV(sb)) {
- struct cached_dev *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
-
- if (!dc) {
- ret = -ENOMEM;
- err = "cannot allocate memory";
- goto out_put_sb_page;
- }
-
mutex_lock(&bch_register_lock);
- ret = register_bdev(sb, sb_disk, bdev, dc);
+ ret = register_bdev(sb, sb_disk, bdev, holder);
mutex_unlock(&bch_register_lock);
/* blkdev_put() will be called in cached_dev_free() */
if (ret < 0)
goto out_free_sb;
} else {
- struct cache *ca = kzalloc(sizeof(*ca), GFP_KERNEL);
-
- if (!ca) {
- ret = -ENOMEM;
- err = "cannot allocate memory";
- goto out_put_sb_page;
- }
-
/* blkdev_put() will be called in bch_cache_release() */
- ret = register_cache(sb, sb_disk, bdev, ca);
+ ret = register_cache(sb, sb_disk, bdev, holder);
if (ret)
goto out_free_sb;
}
@@ -2644,6 +2626,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
async_done:
return size;
+out_free_holder:
+ kfree(holder);
out_put_sb_page:
put_page(virt_to_page(sb_disk));
out_blkdev_put:
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-21 16:23 [PATCH 0/2] bcache: Fix block device claiming Jan Kara
2023-06-21 16:23 ` [PATCH 1/2] bcache: Alloc holder object before async registration Jan Kara
@ 2023-06-21 16:23 ` Jan Kara
2023-06-22 1:23 ` kernel test robot
` (3 more replies)
1 sibling, 4 replies; 12+ messages in thread
From: Jan Kara @ 2023-06-21 16:23 UTC (permalink / raw
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, Coly Li, linux-bcache, Jan Kara
Commit 2736e8eeb0cc ("block: use the holder as indication for exclusive
opens") introduced a change that blkdev_put() has to get exclusive
holder of the bdev as an argument. However it overlooked that
register_bdev() and register_cache() overwrite the bdev->bd_holder field
in the block device to point to the real owning object which was not
available at the time we called blkdev_get_by_path(). Messing with bdev
internals like this is a layering violation and it also causes
blkdev_put() to issue warning about mismatching holders.
Fix bcache to reopen the block device with appropriate holder once it is
available which also restores the behavior that multiple bcache caches
cannot claim the same device which was also broken by commit
2736e8eeb0cc.
Fixes: 2736e8eeb0cc ("block: use the holder as indication for exclusive opens")
Signed-off-by: Jan Kara <jack@suse.cz>
---
drivers/md/bcache/super.c | 63 ++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 913dd94353b6..9738e8c0cbfc 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1369,7 +1369,7 @@ static void cached_dev_free(struct closure *cl)
put_page(virt_to_page(dc->sb_disk));
if (!IS_ERR_OR_NULL(dc->bdev))
- blkdev_put(dc->bdev, bcache_kobj);
+ blkdev_put(dc->bdev, dc);
wake_up(&unregister_wait);
@@ -1453,7 +1453,6 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
memcpy(&dc->sb, sb, sizeof(struct cache_sb));
dc->bdev = bdev;
- dc->bdev->bd_holder = dc;
dc->sb_disk = sb_disk;
if (cached_dev_init(dc, sb->block_size << 9))
@@ -2218,7 +2217,7 @@ void bch_cache_release(struct kobject *kobj)
put_page(virt_to_page(ca->sb_disk));
if (!IS_ERR_OR_NULL(ca->bdev))
- blkdev_put(ca->bdev, bcache_kobj);
+ blkdev_put(ca->bdev, ca);
kfree(ca);
module_put(THIS_MODULE);
@@ -2345,7 +2344,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
memcpy(&ca->sb, sb, sizeof(struct cache_sb));
ca->bdev = bdev;
- ca->bdev->bd_holder = ca;
ca->sb_disk = sb_disk;
if (bdev_max_discard_sectors((bdev)))
@@ -2359,7 +2357,7 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
* call blkdev_put() to bdev in bch_cache_release(). So we
* explicitly call blkdev_put() here.
*/
- blkdev_put(bdev, bcache_kobj);
+ blkdev_put(bdev, ca);
if (ret == -ENOMEM)
err = "cache_alloc(): -ENOMEM";
else if (ret == -EPERM)
@@ -2516,10 +2514,11 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
char *path = NULL;
struct cache_sb *sb;
struct cache_sb_disk *sb_disk;
- struct block_device *bdev;
- void *holder;
+ struct block_device *bdev, *bdev2;
+ void *holder = NULL;
ssize_t ret;
bool async_registration = false;
+ bool quiet = false;
#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
async_registration = true;
@@ -2548,24 +2547,9 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
ret = -EINVAL;
err = "failed to open device";
- bdev = blkdev_get_by_path(strim(path), BLK_OPEN_READ | BLK_OPEN_WRITE,
- bcache_kobj, NULL);
- if (IS_ERR(bdev)) {
- if (bdev == ERR_PTR(-EBUSY)) {
- dev_t dev;
-
- mutex_lock(&bch_register_lock);
- if (lookup_bdev(strim(path), &dev) == 0 &&
- bch_is_open(dev))
- err = "device already registered";
- else
- err = "device busy";
- mutex_unlock(&bch_register_lock);
- if (attr == &ksysfs_register_quiet)
- goto done;
- }
+ bdev = blkdev_get_by_path(strim(path), BLK_OPEN_READ, NULL, NULL);
+ if (IS_ERR(bdev))
goto out_free_sb;
- }
err = "failed to set blocksize";
if (set_blocksize(bdev, 4096))
@@ -2582,6 +2566,31 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
goto out_put_sb_page;
}
+ /* Now reopen in exclusive mode with proper holder */
+ bdev2 = blkdev_get_by_dev(bdev->bd_dev, BLK_OPEN_READ | BLK_OPEN_WRITE,
+ holder, NULL);
+ blkdev_put(bdev, NULL);
+ bdev = bdev2;
+ if (IS_ERR(bdev))
+ ret = PTR_ERR(bdev);
+ if (bdev == ERR_PTR(-EBUSY)) {
+ dev_t dev;
+
+ mutex_lock(&bch_register_lock);
+ if (lookup_bdev(strim(path), &dev) == 0 &&
+ bch_is_open(dev))
+ err = "device already registered";
+ else
+ err = "device busy";
+ mutex_unlock(&bch_register_lock);
+ if (attr == &ksysfs_register_quiet) {
+ quiet = true;
+ ret = size;
+ }
+ }
+ goto out_free_holder;
+ }
+
err = "failed to register device";
if (async_registration) {
@@ -2619,7 +2628,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
goto out_free_sb;
}
-done:
kfree(sb);
kfree(path);
module_put(THIS_MODULE);
@@ -2631,7 +2639,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
out_put_sb_page:
put_page(virt_to_page(sb_disk));
out_blkdev_put:
- blkdev_put(bdev, register_bcache);
+ blkdev_put(bdev, holder);
out_free_sb:
kfree(sb);
out_free_path:
@@ -2640,7 +2648,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
out_module_put:
module_put(THIS_MODULE);
out:
- pr_info("error %s: %s\n", path?path:"", err);
+ if (!quiet)
+ pr_info("error %s: %s\n", path?path:"", err);
return ret;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bcache: Alloc holder object before async registration
2023-06-21 16:23 ` [PATCH 1/2] bcache: Alloc holder object before async registration Jan Kara
@ 2023-06-21 17:56 ` Kent Overstreet
2023-06-22 10:09 ` Jan Kara
0 siblings, 1 reply; 12+ messages in thread
From: Kent Overstreet @ 2023-06-21 17:56 UTC (permalink / raw
To: Jan Kara
Cc: Jens Axboe, Christoph Hellwig, linux-block, Coly Li, linux-bcache
On Wed, Jun 21, 2023 at 06:23:26PM +0200, Jan Kara wrote:
> Allocate holder object (cache or cached_dev) before offloading the
> rest of the startup to async work. This will allow us to open the block
> block device with proper holder.
This is a pretty big change for this fix, and we'd want to retest the
error paths - that's hard to do, because the fault injection framework I
was using for that never made it upstream.
What about just exposing a proper API for changing the holder? Wouldn't
that be simpler?
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> drivers/md/bcache/super.c | 66 +++++++++++++++------------------------
> 1 file changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e2a803683105..913dd94353b6 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2448,6 +2448,7 @@ struct async_reg_args {
> struct cache_sb *sb;
> struct cache_sb_disk *sb_disk;
> struct block_device *bdev;
> + void *holder;
> };
>
> static void register_bdev_worker(struct work_struct *work)
> @@ -2455,22 +2456,13 @@ static void register_bdev_worker(struct work_struct *work)
> int fail = false;
> struct async_reg_args *args =
> container_of(work, struct async_reg_args, reg_work.work);
> - struct cached_dev *dc;
> -
> - dc = kzalloc(sizeof(*dc), GFP_KERNEL);
> - if (!dc) {
> - fail = true;
> - put_page(virt_to_page(args->sb_disk));
> - blkdev_put(args->bdev, bcache_kobj);
> - goto out;
> - }
>
> mutex_lock(&bch_register_lock);
> - if (register_bdev(args->sb, args->sb_disk, args->bdev, dc) < 0)
> + if (register_bdev(args->sb, args->sb_disk, args->bdev, args->holder)
> + < 0)
> fail = true;
> mutex_unlock(&bch_register_lock);
>
> -out:
> if (fail)
> pr_info("error %s: fail to register backing device\n",
> args->path);
> @@ -2485,21 +2477,11 @@ static void register_cache_worker(struct work_struct *work)
> int fail = false;
> struct async_reg_args *args =
> container_of(work, struct async_reg_args, reg_work.work);
> - struct cache *ca;
> -
> - ca = kzalloc(sizeof(*ca), GFP_KERNEL);
> - if (!ca) {
> - fail = true;
> - put_page(virt_to_page(args->sb_disk));
> - blkdev_put(args->bdev, bcache_kobj);
> - goto out;
> - }
>
> /* blkdev_put() will be called in bch_cache_release() */
> - if (register_cache(args->sb, args->sb_disk, args->bdev, ca) != 0)
> + if (register_cache(args->sb, args->sb_disk, args->bdev, args->holder))
> fail = true;
>
> -out:
> if (fail)
> pr_info("error %s: fail to register cache device\n",
> args->path);
> @@ -2520,6 +2502,13 @@ static void register_device_async(struct async_reg_args *args)
> queue_delayed_work(system_wq, &args->reg_work, 10);
> }
>
> +static void *alloc_holder_object(struct cache_sb *sb)
> +{
> + if (SB_IS_BDEV(sb))
> + return kzalloc(sizeof(struct cached_dev), GFP_KERNEL);
> + return kzalloc(sizeof(struct cache), GFP_KERNEL);
> +}
> +
> static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> const char *buffer, size_t size)
> {
> @@ -2528,6 +2517,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> struct cache_sb *sb;
> struct cache_sb_disk *sb_disk;
> struct block_device *bdev;
> + void *holder;
> ssize_t ret;
> bool async_registration = false;
>
> @@ -2585,6 +2575,13 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> if (err)
> goto out_blkdev_put;
>
> + holder = alloc_holder_object(sb);
> + if (!holder) {
> + ret = -ENOMEM;
> + err = "cannot allocate memory";
> + goto out_put_sb_page;
> + }
> +
> err = "failed to register device";
>
> if (async_registration) {
> @@ -2595,44 +2592,29 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> if (!args) {
> ret = -ENOMEM;
> err = "cannot allocate memory";
> - goto out_put_sb_page;
> + goto out_free_holder;
> }
>
> args->path = path;
> args->sb = sb;
> args->sb_disk = sb_disk;
> args->bdev = bdev;
> + args->holder = holder;
> register_device_async(args);
> /* No wait and returns to user space */
> goto async_done;
> }
>
> if (SB_IS_BDEV(sb)) {
> - struct cached_dev *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
> -
> - if (!dc) {
> - ret = -ENOMEM;
> - err = "cannot allocate memory";
> - goto out_put_sb_page;
> - }
> -
> mutex_lock(&bch_register_lock);
> - ret = register_bdev(sb, sb_disk, bdev, dc);
> + ret = register_bdev(sb, sb_disk, bdev, holder);
> mutex_unlock(&bch_register_lock);
> /* blkdev_put() will be called in cached_dev_free() */
> if (ret < 0)
> goto out_free_sb;
> } else {
> - struct cache *ca = kzalloc(sizeof(*ca), GFP_KERNEL);
> -
> - if (!ca) {
> - ret = -ENOMEM;
> - err = "cannot allocate memory";
> - goto out_put_sb_page;
> - }
> -
> /* blkdev_put() will be called in bch_cache_release() */
> - ret = register_cache(sb, sb_disk, bdev, ca);
> + ret = register_cache(sb, sb_disk, bdev, holder);
> if (ret)
> goto out_free_sb;
> }
> @@ -2644,6 +2626,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> async_done:
> return size;
>
> +out_free_holder:
> + kfree(holder);
> out_put_sb_page:
> put_page(virt_to_page(sb_disk));
> out_blkdev_put:
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
@ 2023-06-22 1:23 ` kernel test robot
2023-06-22 10:26 ` Jan Kara
2023-06-22 1:44 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: kernel test robot @ 2023-06-22 1:23 UTC (permalink / raw
To: Jan Kara, Jens Axboe
Cc: oe-kbuild-all, Christoph Hellwig, linux-block, Coly Li,
linux-bcache, Jan Kara
Hi Jan,
kernel test robot noticed the following build errors:
[auto build test ERROR on axboe-block/for-6.5/block]
[also build test ERROR on next-20230621]
[cannot apply to linus/master v6.4-rc7]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/bcache-Alloc-holder-object-before-async-registration/20230622-002543
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-6.5/block
patch link: https://lore.kernel.org/r/20230621162333.30027-2-jack%40suse.cz
patch subject: [PATCH 2/2] bcache: Fix bcache device claiming
config: s390-defconfig (https://download.01.org/0day-ci/archive/20230622/202306220927.dRUNZbek-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230622/202306220927.dRUNZbek-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306220927.dRUNZbek-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/md/bcache/super.c: In function 'register_bcache':
>> drivers/md/bcache/super.c:2574:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
2574 | if (IS_ERR(bdev))
| ^~
drivers/md/bcache/super.c:2576:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
2576 | if (bdev == ERR_PTR(-EBUSY)) {
| ^~
>> drivers/md/bcache/super.c:2591:17: error: label 'out_free_holder' used but not defined
2591 | goto out_free_holder;
| ^~~~
>> drivers/md/bcache/super.c:2566:17: error: label 'out_put_sb_page' used but not defined
2566 | goto out_put_sb_page;
| ^~~~
>> drivers/md/bcache/super.c:2560:17: error: label 'out_blkdev_put' used but not defined
2560 | goto out_blkdev_put;
| ^~~~
>> drivers/md/bcache/super.c:2552:17: error: label 'out_free_sb' used but not defined
2552 | goto out_free_sb;
| ^~~~
>> drivers/md/bcache/super.c:2546:17: error: label 'out_free_path' used but not defined
2546 | goto out_free_path;
| ^~~~
>> drivers/md/bcache/super.c:2542:17: error: label 'out_module_put' used but not defined
2542 | goto out_module_put;
| ^~~~
>> drivers/md/bcache/super.c:2530:17: error: label 'out' used but not defined
2530 | goto out;
| ^~~~
>> drivers/md/bcache/super.c:2521:14: warning: variable 'quiet' set but not used [-Wunused-but-set-variable]
2521 | bool quiet = false;
| ^~~~~
drivers/md/bcache/super.c:2520:14: warning: unused variable 'async_registration' [-Wunused-variable]
2520 | bool async_registration = false;
| ^~~~~~~~~~~~~~~~~~
>> drivers/md/bcache/super.c:2519:17: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
2519 | ssize_t ret;
| ^~~
drivers/md/bcache/super.c:2592:9: error: no return statement in function returning non-void [-Werror=return-type]
2592 | }
| ^
drivers/md/bcache/super.c: At top level:
>> drivers/md/bcache/super.c:2594:9: warning: data definition has no type or storage class
2594 | err = "failed to register device";
| ^~~
>> drivers/md/bcache/super.c:2594:9: error: type defaults to 'int' in declaration of 'err' [-Werror=implicit-int]
>> drivers/md/bcache/super.c:2594:15: warning: initialization of 'int' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
2594 | err = "failed to register device";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/md/bcache/super.c:2594:15: error: initializer element is not computable at load time
>> drivers/md/bcache/super.c:2596:9: error: expected identifier or '(' before 'if'
2596 | if (async_registration) {
| ^~
drivers/md/bcache/super.c:2617:9: error: expected identifier or '(' before 'if'
2617 | if (SB_IS_BDEV(sb)) {
| ^~
>> drivers/md/bcache/super.c:2624:11: error: expected identifier or '(' before 'else'
2624 | } else {
| ^~~~
drivers/md/bcache/super.c:2631:9: warning: data definition has no type or storage class
2631 | kfree(sb);
| ^~~~~
>> drivers/md/bcache/super.c:2631:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
>> drivers/md/bcache/super.c:2631:9: warning: parameter names (without types) in function declaration
>> drivers/md/bcache/super.c:2631:9: error: conflicting types for 'kfree'; have 'int()'
In file included from include/linux/fs.h:45,
from include/linux/highmem.h:5,
from include/linux/bvec.h:10,
from include/linux/blk_types.h:10,
from include/linux/bio.h:10,
from drivers/md/bcache/bcache.h:181,
from drivers/md/bcache/super.c:10:
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
drivers/md/bcache/super.c:2632:9: warning: data definition has no type or storage class
2632 | kfree(path);
| ^~~~~
drivers/md/bcache/super.c:2632:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
drivers/md/bcache/super.c:2632:9: warning: parameter names (without types) in function declaration
drivers/md/bcache/super.c:2632:9: error: conflicting types for 'kfree'; have 'int()'
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
In file included from include/linux/linkage.h:7,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:56,
from include/linux/wait.h:9,
from include/linux/mempool.h:8,
from include/linux/bio.h:8:
>> include/linux/export.h:27:21: error: expected declaration specifiers or '...' before '(' token
27 | #define THIS_MODULE (&__this_module)
| ^
drivers/md/bcache/super.c:2633:20: note: in expansion of macro 'THIS_MODULE'
2633 | module_put(THIS_MODULE);
| ^~~~~~~~~~~
drivers/md/bcache/super.c:2634:11: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2634 | async_done:
| ^
drivers/md/bcache/super.c:2637:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2637 | out_free_holder:
| ^
drivers/md/bcache/super.c:2639:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2639 | out_put_sb_page:
| ^
drivers/md/bcache/super.c:2641:15: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2641 | out_blkdev_put:
| ^
drivers/md/bcache/super.c:2643:12: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2643 | out_free_sb:
| ^
drivers/md/bcache/super.c:2645:14: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2645 | out_free_path:
| ^
drivers/md/bcache/super.c:2647:9: warning: data definition has no type or storage class
2647 | path = NULL;
| ^~~~
drivers/md/bcache/super.c:2647:9: error: type defaults to 'int' in declaration of 'path' [-Werror=implicit-int]
In file included from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/kasan-checks.h:5,
from include/asm-generic/rwonce.h:26,
from arch/s390/include/asm/rwonce.h:29,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/wait.h:7:
include/linux/stddef.h:8:14: warning: initialization of 'int' from 'void *' makes integer from pointer without a cast [-Wint-conversion]
8 | #define NULL ((void *)0)
| ^
drivers/md/bcache/super.c:2647:16: note: in expansion of macro 'NULL'
2647 | path = NULL;
| ^~~~
drivers/md/bcache/super.c:2648:15: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2648 | out_module_put:
| ^
drivers/md/bcache/super.c:2650:4: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2650 | out:
| ^
In file included from include/asm-generic/bug.h:22,
from arch/s390/include/asm/bug.h:69,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from arch/s390/include/asm/preempt.h:6,
from include/linux/preempt.h:78:
include/linux/printk.h:428:10: error: expected identifier or '(' before ')' token
428 | })
| ^
include/linux/printk.h:455:26: note: in expansion of macro 'printk_index_wrap'
455 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~
include/linux/printk.h:528:9: note: in expansion of macro 'printk'
528 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
drivers/md/bcache/super.c:2652:17: note: in expansion of macro 'pr_info'
2652 | pr_info("error %s: %s\n", path?path:"", err);
| ^~~~~~~
drivers/md/bcache/super.c:2653:9: error: expected identifier or '(' before 'return'
2653 | return ret;
| ^~~~~~
drivers/md/bcache/super.c:2654:1: error: expected identifier or '(' before '}' token
2654 | }
| ^
drivers/md/bcache/super.c:2492:13: warning: 'register_device_async' defined but not used [-Wunused-function]
2492 | static void register_device_async(struct async_reg_args *args)
| ^~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/out_free_holder +2591 drivers/md/bcache/super.c
2509
2510 static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
2511 const char *buffer, size_t size)
2512 {
2513 const char *err;
2514 char *path = NULL;
2515 struct cache_sb *sb;
2516 struct cache_sb_disk *sb_disk;
2517 struct block_device *bdev, *bdev2;
2518 void *holder = NULL;
> 2519 ssize_t ret;
2520 bool async_registration = false;
> 2521 bool quiet = false;
2522
2523 #ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
2524 async_registration = true;
2525 #endif
2526
2527 ret = -EBUSY;
2528 err = "failed to reference bcache module";
2529 if (!try_module_get(THIS_MODULE))
> 2530 goto out;
2531
2532 /* For latest state of bcache_is_reboot */
2533 smp_mb();
2534 err = "bcache is in reboot";
2535 if (bcache_is_reboot)
2536 goto out_module_put;
2537
2538 ret = -ENOMEM;
2539 err = "cannot allocate memory";
2540 path = kstrndup(buffer, size, GFP_KERNEL);
2541 if (!path)
> 2542 goto out_module_put;
2543
2544 sb = kmalloc(sizeof(struct cache_sb), GFP_KERNEL);
2545 if (!sb)
> 2546 goto out_free_path;
2547
2548 ret = -EINVAL;
2549 err = "failed to open device";
2550 bdev = blkdev_get_by_path(strim(path), BLK_OPEN_READ, NULL, NULL);
2551 if (IS_ERR(bdev))
> 2552 goto out_free_sb;
2553
2554 err = "failed to set blocksize";
2555 if (set_blocksize(bdev, 4096))
2556 goto out_blkdev_put;
2557
2558 err = read_super(sb, bdev, &sb_disk);
2559 if (err)
> 2560 goto out_blkdev_put;
2561
2562 holder = alloc_holder_object(sb);
2563 if (!holder) {
2564 ret = -ENOMEM;
2565 err = "cannot allocate memory";
> 2566 goto out_put_sb_page;
2567 }
2568
2569 /* Now reopen in exclusive mode with proper holder */
2570 bdev2 = blkdev_get_by_dev(bdev->bd_dev, BLK_OPEN_READ | BLK_OPEN_WRITE,
2571 holder, NULL);
2572 blkdev_put(bdev, NULL);
2573 bdev = bdev2;
> 2574 if (IS_ERR(bdev))
2575 ret = PTR_ERR(bdev);
> 2576 if (bdev == ERR_PTR(-EBUSY)) {
2577 dev_t dev;
2578
2579 mutex_lock(&bch_register_lock);
2580 if (lookup_bdev(strim(path), &dev) == 0 &&
2581 bch_is_open(dev))
2582 err = "device already registered";
2583 else
2584 err = "device busy";
2585 mutex_unlock(&bch_register_lock);
2586 if (attr == &ksysfs_register_quiet) {
2587 quiet = true;
2588 ret = size;
2589 }
2590 }
> 2591 goto out_free_holder;
> 2592 }
2593
> 2594 err = "failed to register device";
2595
> 2596 if (async_registration) {
2597 /* register in asynchronous way */
2598 struct async_reg_args *args =
2599 kzalloc(sizeof(struct async_reg_args), GFP_KERNEL);
2600
2601 if (!args) {
2602 ret = -ENOMEM;
2603 err = "cannot allocate memory";
2604 goto out_free_holder;
2605 }
2606
2607 args->path = path;
2608 args->sb = sb;
2609 args->sb_disk = sb_disk;
2610 args->bdev = bdev;
2611 args->holder = holder;
2612 register_device_async(args);
2613 /* No wait and returns to user space */
2614 goto async_done;
2615 }
2616
> 2617 if (SB_IS_BDEV(sb)) {
2618 mutex_lock(&bch_register_lock);
2619 ret = register_bdev(sb, sb_disk, bdev, holder);
2620 mutex_unlock(&bch_register_lock);
2621 /* blkdev_put() will be called in cached_dev_free() */
2622 if (ret < 0)
2623 goto out_free_sb;
> 2624 } else {
2625 /* blkdev_put() will be called in bch_cache_release() */
2626 ret = register_cache(sb, sb_disk, bdev, holder);
2627 if (ret)
2628 goto out_free_sb;
2629 }
2630
> 2631 kfree(sb);
2632 kfree(path);
> 2633 module_put(THIS_MODULE);
> 2634 async_done:
2635 return size;
2636
2637 out_free_holder:
2638 kfree(holder);
2639 out_put_sb_page:
2640 put_page(virt_to_page(sb_disk));
2641 out_blkdev_put:
2642 blkdev_put(bdev, holder);
> 2643 out_free_sb:
2644 kfree(sb);
2645 out_free_path:
2646 kfree(path);
> 2647 path = NULL;
2648 out_module_put:
2649 module_put(THIS_MODULE);
2650 out:
2651 if (!quiet)
> 2652 pr_info("error %s: %s\n", path?path:"", err);
> 2653 return ret;
> 2654 }
2655
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 1:23 ` kernel test robot
@ 2023-06-22 1:44 ` kernel test robot
2023-06-22 3:29 ` kernel test robot
2023-06-22 15:12 ` Christoph Hellwig
3 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2023-06-22 1:44 UTC (permalink / raw
To: Jan Kara, Jens Axboe
Cc: oe-kbuild-all, Christoph Hellwig, linux-block, Coly Li,
linux-bcache, Jan Kara
Hi Jan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on axboe-block/for-6.5/block]
[also build test WARNING on next-20230621]
[cannot apply to linus/master hch-configfs/for-next v6.4-rc7]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/bcache-Alloc-holder-object-before-async-registration/20230622-002543
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-6.5/block
patch link: https://lore.kernel.org/r/20230621162333.30027-2-jack%40suse.cz
patch subject: [PATCH 2/2] bcache: Fix bcache device claiming
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20230622/202306220914.B0THiZdL-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230622/202306220914.B0THiZdL-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306220914.B0THiZdL-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/md/bcache/super.c: In function 'register_bcache':
drivers/md/bcache/super.c:2574:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
2574 | if (IS_ERR(bdev))
| ^~
drivers/md/bcache/super.c:2576:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
2576 | if (bdev == ERR_PTR(-EBUSY)) {
| ^~
drivers/md/bcache/super.c:2591:17: error: label 'out_free_holder' used but not defined
2591 | goto out_free_holder;
| ^~~~
drivers/md/bcache/super.c:2566:17: error: label 'out_put_sb_page' used but not defined
2566 | goto out_put_sb_page;
| ^~~~
drivers/md/bcache/super.c:2560:17: error: label 'out_blkdev_put' used but not defined
2560 | goto out_blkdev_put;
| ^~~~
drivers/md/bcache/super.c:2552:17: error: label 'out_free_sb' used but not defined
2552 | goto out_free_sb;
| ^~~~
drivers/md/bcache/super.c:2546:17: error: label 'out_free_path' used but not defined
2546 | goto out_free_path;
| ^~~~
drivers/md/bcache/super.c:2542:17: error: label 'out_module_put' used but not defined
2542 | goto out_module_put;
| ^~~~
drivers/md/bcache/super.c:2530:17: error: label 'out' used but not defined
2530 | goto out;
| ^~~~
drivers/md/bcache/super.c:2521:14: warning: variable 'quiet' set but not used [-Wunused-but-set-variable]
2521 | bool quiet = false;
| ^~~~~
>> drivers/md/bcache/super.c:2520:14: warning: variable 'async_registration' set but not used [-Wunused-but-set-variable]
2520 | bool async_registration = false;
| ^~~~~~~~~~~~~~~~~~
drivers/md/bcache/super.c:2519:17: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
2519 | ssize_t ret;
| ^~~
drivers/md/bcache/super.c:2592:9: error: no return statement in function returning non-void [-Werror=return-type]
2592 | }
| ^
drivers/md/bcache/super.c: At top level:
drivers/md/bcache/super.c:2594:9: warning: data definition has no type or storage class
2594 | err = "failed to register device";
| ^~~
drivers/md/bcache/super.c:2594:9: error: type defaults to 'int' in declaration of 'err' [-Werror=implicit-int]
drivers/md/bcache/super.c:2594:15: warning: initialization of 'int' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
2594 | err = "failed to register device";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/md/bcache/super.c:2594:15: error: initializer element is not computable at load time
drivers/md/bcache/super.c:2596:9: error: expected identifier or '(' before 'if'
2596 | if (async_registration) {
| ^~
drivers/md/bcache/super.c:2617:9: error: expected identifier or '(' before 'if'
2617 | if (SB_IS_BDEV(sb)) {
| ^~
drivers/md/bcache/super.c:2624:11: error: expected identifier or '(' before 'else'
2624 | } else {
| ^~~~
drivers/md/bcache/super.c:2631:9: warning: data definition has no type or storage class
2631 | kfree(sb);
| ^~~~~
drivers/md/bcache/super.c:2631:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
drivers/md/bcache/super.c:2631:9: warning: parameter names (without types) in function declaration
drivers/md/bcache/super.c:2631:9: error: conflicting types for 'kfree'; have 'int()'
In file included from include/linux/fs.h:45,
from include/linux/highmem.h:5,
from include/linux/bvec.h:10,
from include/linux/blk_types.h:10,
from include/linux/bio.h:10,
from drivers/md/bcache/bcache.h:181,
from drivers/md/bcache/super.c:10:
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
drivers/md/bcache/super.c:2632:9: warning: data definition has no type or storage class
2632 | kfree(path);
| ^~~~~
drivers/md/bcache/super.c:2632:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
drivers/md/bcache/super.c:2632:9: warning: parameter names (without types) in function declaration
drivers/md/bcache/super.c:2632:9: error: conflicting types for 'kfree'; have 'int()'
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
In file included from include/linux/linkage.h:7,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:56,
from include/linux/wait.h:9,
from include/linux/mempool.h:8,
from include/linux/bio.h:8:
include/linux/export.h:27:21: error: expected declaration specifiers or '...' before '(' token
27 | #define THIS_MODULE (&__this_module)
| ^
drivers/md/bcache/super.c:2633:20: note: in expansion of macro 'THIS_MODULE'
2633 | module_put(THIS_MODULE);
| ^~~~~~~~~~~
drivers/md/bcache/super.c:2634:11: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2634 | async_done:
| ^
drivers/md/bcache/super.c:2637:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2637 | out_free_holder:
| ^
drivers/md/bcache/super.c:2639:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2639 | out_put_sb_page:
| ^
drivers/md/bcache/super.c:2641:15: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2641 | out_blkdev_put:
| ^
drivers/md/bcache/super.c:2643:12: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2643 | out_free_sb:
| ^
drivers/md/bcache/super.c:2645:14: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2645 | out_free_path:
| ^
drivers/md/bcache/super.c:2647:9: warning: data definition has no type or storage class
2647 | path = NULL;
| ^~~~
drivers/md/bcache/super.c:2647:9: error: type defaults to 'int' in declaration of 'path' [-Werror=implicit-int]
In file included from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/kasan-checks.h:5,
from include/asm-generic/rwonce.h:26,
from arch/s390/include/asm/rwonce.h:29,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/wait.h:7:
include/linux/stddef.h:8:14: warning: initialization of 'int' from 'void *' makes integer from pointer without a cast [-Wint-conversion]
8 | #define NULL ((void *)0)
| ^
drivers/md/bcache/super.c:2647:16: note: in expansion of macro 'NULL'
vim +/async_registration +2520 drivers/md/bcache/super.c
867f981b0739bd Jan Kara 2023-06-21 2509
cafe563591446c Kent Overstreet 2013-03-23 2510 static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
cafe563591446c Kent Overstreet 2013-03-23 2511 const char *buffer, size_t size)
cafe563591446c Kent Overstreet 2013-03-23 2512 {
50246693f81fe8 Christoph Hellwig 2020-01-24 2513 const char *err;
29cda393bcaad1 Coly Li 2020-01-24 2514 char *path = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2515 struct cache_sb *sb;
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2516 struct cache_sb_disk *sb_disk;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2517 struct block_device *bdev, *bdev2;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2518 void *holder = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2519 ssize_t ret;
a58e88bfdc4d52 Coly Li 2020-10-01 @2520 bool async_registration = false;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2521 bool quiet = false;
a58e88bfdc4d52 Coly Li 2020-10-01 2522
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 1:23 ` kernel test robot
2023-06-22 1:44 ` kernel test robot
@ 2023-06-22 3:29 ` kernel test robot
2023-06-22 15:12 ` Christoph Hellwig
3 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2023-06-22 3:29 UTC (permalink / raw
To: Jan Kara, Jens Axboe
Cc: oe-kbuild-all, Christoph Hellwig, linux-block, Coly Li,
linux-bcache, Jan Kara
Hi Jan,
kernel test robot noticed the following build errors:
[auto build test ERROR on axboe-block/for-6.5/block]
[also build test ERROR on next-20230621]
[cannot apply to linus/master hch-configfs/for-next v6.4-rc7]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/bcache-Alloc-holder-object-before-async-registration/20230622-002543
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-6.5/block
patch link: https://lore.kernel.org/r/20230621162333.30027-2-jack%40suse.cz
patch subject: [PATCH 2/2] bcache: Fix bcache device claiming
config: i386-debian-10.3 (https://download.01.org/0day-ci/archive/20230622/202306221133.HFiLGynP-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230622/202306221133.HFiLGynP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306221133.HFiLGynP-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/md/bcache/super.c: In function 'register_bcache':
drivers/md/bcache/super.c:2574:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
2574 | if (IS_ERR(bdev))
| ^~
drivers/md/bcache/super.c:2576:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
2576 | if (bdev == ERR_PTR(-EBUSY)) {
| ^~
drivers/md/bcache/super.c:2591:17: error: label 'out_free_holder' used but not defined
2591 | goto out_free_holder;
| ^~~~
drivers/md/bcache/super.c:2566:17: error: label 'out_put_sb_page' used but not defined
2566 | goto out_put_sb_page;
| ^~~~
drivers/md/bcache/super.c:2560:17: error: label 'out_blkdev_put' used but not defined
2560 | goto out_blkdev_put;
| ^~~~
drivers/md/bcache/super.c:2552:17: error: label 'out_free_sb' used but not defined
2552 | goto out_free_sb;
| ^~~~
drivers/md/bcache/super.c:2546:17: error: label 'out_free_path' used but not defined
2546 | goto out_free_path;
| ^~~~
drivers/md/bcache/super.c:2542:17: error: label 'out_module_put' used but not defined
2542 | goto out_module_put;
| ^~~~
drivers/md/bcache/super.c:2530:17: error: label 'out' used but not defined
2530 | goto out;
| ^~~~
drivers/md/bcache/super.c:2521:14: warning: variable 'quiet' set but not used [-Wunused-but-set-variable]
2521 | bool quiet = false;
| ^~~~~
>> drivers/md/bcache/super.c:2520:14: warning: unused variable 'async_registration' [-Wunused-variable]
2520 | bool async_registration = false;
| ^~~~~~~~~~~~~~~~~~
drivers/md/bcache/super.c:2519:17: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
2519 | ssize_t ret;
| ^~~
drivers/md/bcache/super.c:2592:9: error: no return statement in function returning non-void [-Werror=return-type]
2592 | }
| ^
drivers/md/bcache/super.c: At top level:
drivers/md/bcache/super.c:2594:9: warning: data definition has no type or storage class
2594 | err = "failed to register device";
| ^~~
drivers/md/bcache/super.c:2594:9: error: type defaults to 'int' in declaration of 'err' [-Werror=implicit-int]
drivers/md/bcache/super.c:2594:15: warning: initialization of 'int' from 'char *' makes integer from pointer without a cast [-Wint-conversion]
2594 | err = "failed to register device";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/md/bcache/super.c:2596:9: error: expected identifier or '(' before 'if'
2596 | if (async_registration) {
| ^~
drivers/md/bcache/super.c:2617:9: error: expected identifier or '(' before 'if'
2617 | if (SB_IS_BDEV(sb)) {
| ^~
drivers/md/bcache/super.c:2624:11: error: expected identifier or '(' before 'else'
2624 | } else {
| ^~~~
drivers/md/bcache/super.c:2631:9: warning: data definition has no type or storage class
2631 | kfree(sb);
| ^~~~~
drivers/md/bcache/super.c:2631:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
drivers/md/bcache/super.c:2631:9: warning: parameter names (without types) in function declaration
drivers/md/bcache/super.c:2631:9: error: conflicting types for 'kfree'; have 'int()'
In file included from include/linux/fs.h:45,
from include/linux/highmem.h:5,
from include/linux/bvec.h:10,
from include/linux/blk_types.h:10,
from include/linux/bio.h:10,
from drivers/md/bcache/bcache.h:181,
from drivers/md/bcache/super.c:10:
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
drivers/md/bcache/super.c:2632:9: warning: data definition has no type or storage class
2632 | kfree(path);
| ^~~~~
drivers/md/bcache/super.c:2632:9: error: type defaults to 'int' in declaration of 'kfree' [-Werror=implicit-int]
drivers/md/bcache/super.c:2632:9: warning: parameter names (without types) in function declaration
drivers/md/bcache/super.c:2632:9: error: conflicting types for 'kfree'; have 'int()'
include/linux/slab.h:210:6: note: previous declaration of 'kfree' with type 'void(const void *)'
210 | void kfree(const void *objp);
| ^~~~~
In file included from include/linux/linkage.h:7,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:56,
from include/linux/wait.h:9,
from include/linux/mempool.h:8,
from include/linux/bio.h:8:
include/linux/export.h:27:21: error: expected declaration specifiers or '...' before '(' token
27 | #define THIS_MODULE (&__this_module)
| ^
drivers/md/bcache/super.c:2633:20: note: in expansion of macro 'THIS_MODULE'
2633 | module_put(THIS_MODULE);
| ^~~~~~~~~~~
drivers/md/bcache/super.c:2634:11: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2634 | async_done:
| ^
drivers/md/bcache/super.c:2637:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2637 | out_free_holder:
| ^
drivers/md/bcache/super.c:2639:16: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2639 | out_put_sb_page:
| ^
In file included from arch/x86/include/asm/page.h:85,
from arch/x86/include/asm/thread_info.h:12,
from include/linux/thread_info.h:60,
from arch/x86/include/asm/preempt.h:9,
from include/linux/preempt.h:78:
>> include/asm-generic/memory_model.h:55:2: error: expected identifier or '(' before ')' token
55 | })
| ^
include/asm-generic/memory_model.h:65:21: note: in expansion of macro '__pfn_to_page'
65 | #define pfn_to_page __pfn_to_page
| ^~~~~~~~~~~~~
arch/x86/include/asm/page.h:68:33: note: in expansion of macro 'pfn_to_page'
68 | #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
| ^~~~~~~~~~~
drivers/md/bcache/super.c:2640:18: note: in expansion of macro 'virt_to_page'
2640 | put_page(virt_to_page(sb_disk));
| ^~~~~~~~~~~~
drivers/md/bcache/super.c:2641:15: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2641 | out_blkdev_put:
| ^
drivers/md/bcache/super.c:2643:12: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2643 | out_free_sb:
| ^
drivers/md/bcache/super.c:2645:14: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2645 | out_free_path:
| ^
drivers/md/bcache/super.c:2647:9: warning: data definition has no type or storage class
2647 | path = NULL;
| ^~~~
drivers/md/bcache/super.c:2647:9: error: type defaults to 'int' in declaration of 'path' [-Werror=implicit-int]
In file included from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/kasan-checks.h:5,
from include/asm-generic/rwonce.h:26,
from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:247,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/wait.h:7:
include/linux/stddef.h:8:14: warning: initialization of 'int' from 'void *' makes integer from pointer without a cast [-Wint-conversion]
8 | #define NULL ((void *)0)
| ^
drivers/md/bcache/super.c:2647:16: note: in expansion of macro 'NULL'
2647 | path = NULL;
| ^~~~
drivers/md/bcache/super.c:2648:15: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2648 | out_module_put:
| ^
drivers/md/bcache/super.c:2650:4: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
2650 | out:
| ^
In file included from include/linux/kernel.h:30,
from arch/x86/include/asm/percpu.h:27,
from arch/x86/include/asm/preempt.h:6:
include/linux/printk.h:428:10: error: expected identifier or '(' before ')' token
428 | })
| ^
include/linux/printk.h:455:26: note: in expansion of macro 'printk_index_wrap'
455 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~
include/linux/printk.h:528:9: note: in expansion of macro 'printk'
528 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
drivers/md/bcache/super.c:2652:17: note: in expansion of macro 'pr_info'
2652 | pr_info("error %s: %s\n", path?path:"", err);
| ^~~~~~~
drivers/md/bcache/super.c:2653:9: error: expected identifier or '(' before 'return'
2653 | return ret;
| ^~~~~~
drivers/md/bcache/super.c:2654:1: error: expected identifier or '(' before '}' token
2654 | }
| ^
drivers/md/bcache/super.c:2492:13: warning: 'register_device_async' defined but not used [-Wunused-function]
2492 | static void register_device_async(struct async_reg_args *args)
| ^~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/async_registration +2520 drivers/md/bcache/super.c
867f981b0739bd Jan Kara 2023-06-21 2509
cafe563591446c Kent Overstreet 2013-03-23 2510 static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
cafe563591446c Kent Overstreet 2013-03-23 2511 const char *buffer, size_t size)
cafe563591446c Kent Overstreet 2013-03-23 2512 {
50246693f81fe8 Christoph Hellwig 2020-01-24 2513 const char *err;
29cda393bcaad1 Coly Li 2020-01-24 2514 char *path = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2515 struct cache_sb *sb;
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2516 struct cache_sb_disk *sb_disk;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2517 struct block_device *bdev, *bdev2;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2518 void *holder = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2519 ssize_t ret;
a58e88bfdc4d52 Coly Li 2020-10-01 @2520 bool async_registration = false;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2521 bool quiet = false;
a58e88bfdc4d52 Coly Li 2020-10-01 2522
a58e88bfdc4d52 Coly Li 2020-10-01 2523 #ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
a58e88bfdc4d52 Coly Li 2020-10-01 2524 async_registration = true;
a58e88bfdc4d52 Coly Li 2020-10-01 2525 #endif
cafe563591446c Kent Overstreet 2013-03-23 2526
50246693f81fe8 Christoph Hellwig 2020-01-24 2527 ret = -EBUSY;
29cda393bcaad1 Coly Li 2020-01-24 2528 err = "failed to reference bcache module";
cafe563591446c Kent Overstreet 2013-03-23 2529 if (!try_module_get(THIS_MODULE))
50246693f81fe8 Christoph Hellwig 2020-01-24 2530 goto out;
cafe563591446c Kent Overstreet 2013-03-23 2531
a59ff6ccc2bf2e Coly Li 2019-06-28 2532 /* For latest state of bcache_is_reboot */
a59ff6ccc2bf2e Coly Li 2019-06-28 2533 smp_mb();
29cda393bcaad1 Coly Li 2020-01-24 2534 err = "bcache is in reboot";
a59ff6ccc2bf2e Coly Li 2019-06-28 2535 if (bcache_is_reboot)
50246693f81fe8 Christoph Hellwig 2020-01-24 2536 goto out_module_put;
a59ff6ccc2bf2e Coly Li 2019-06-28 2537
50246693f81fe8 Christoph Hellwig 2020-01-24 2538 ret = -ENOMEM;
50246693f81fe8 Christoph Hellwig 2020-01-24 2539 err = "cannot allocate memory";
a56489d4b3c914 Florian Schmaus 2018-07-26 2540 path = kstrndup(buffer, size, GFP_KERNEL);
a56489d4b3c914 Florian Schmaus 2018-07-26 2541 if (!path)
50246693f81fe8 Christoph Hellwig 2020-01-24 @2542 goto out_module_put;
a56489d4b3c914 Florian Schmaus 2018-07-26 2543
a56489d4b3c914 Florian Schmaus 2018-07-26 2544 sb = kmalloc(sizeof(struct cache_sb), GFP_KERNEL);
a56489d4b3c914 Florian Schmaus 2018-07-26 2545 if (!sb)
50246693f81fe8 Christoph Hellwig 2020-01-24 2546 goto out_free_path;
cafe563591446c Kent Overstreet 2013-03-23 2547
50246693f81fe8 Christoph Hellwig 2020-01-24 2548 ret = -EINVAL;
cafe563591446c Kent Overstreet 2013-03-23 2549 err = "failed to open device";
ee9eea7d8b1a83 Jan Kara 2023-06-21 2550 bdev = blkdev_get_by_path(strim(path), BLK_OPEN_READ, NULL, NULL);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2551 if (IS_ERR(bdev))
50246693f81fe8 Christoph Hellwig 2020-01-24 2552 goto out_free_sb;
f59fce847fc848 Kent Overstreet 2013-05-15 2553
f59fce847fc848 Kent Overstreet 2013-05-15 2554 err = "failed to set blocksize";
f59fce847fc848 Kent Overstreet 2013-05-15 2555 if (set_blocksize(bdev, 4096))
50246693f81fe8 Christoph Hellwig 2020-01-24 2556 goto out_blkdev_put;
cafe563591446c Kent Overstreet 2013-03-23 2557
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2558 err = read_super(sb, bdev, &sb_disk);
cafe563591446c Kent Overstreet 2013-03-23 2559 if (err)
50246693f81fe8 Christoph Hellwig 2020-01-24 2560 goto out_blkdev_put;
cafe563591446c Kent Overstreet 2013-03-23 2561
867f981b0739bd Jan Kara 2023-06-21 2562 holder = alloc_holder_object(sb);
867f981b0739bd Jan Kara 2023-06-21 2563 if (!holder) {
867f981b0739bd Jan Kara 2023-06-21 2564 ret = -ENOMEM;
867f981b0739bd Jan Kara 2023-06-21 2565 err = "cannot allocate memory";
867f981b0739bd Jan Kara 2023-06-21 2566 goto out_put_sb_page;
867f981b0739bd Jan Kara 2023-06-21 2567 }
867f981b0739bd Jan Kara 2023-06-21 2568
ee9eea7d8b1a83 Jan Kara 2023-06-21 2569 /* Now reopen in exclusive mode with proper holder */
ee9eea7d8b1a83 Jan Kara 2023-06-21 2570 bdev2 = blkdev_get_by_dev(bdev->bd_dev, BLK_OPEN_READ | BLK_OPEN_WRITE,
ee9eea7d8b1a83 Jan Kara 2023-06-21 2571 holder, NULL);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2572 blkdev_put(bdev, NULL);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2573 bdev = bdev2;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2574 if (IS_ERR(bdev))
ee9eea7d8b1a83 Jan Kara 2023-06-21 2575 ret = PTR_ERR(bdev);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2576 if (bdev == ERR_PTR(-EBUSY)) {
ee9eea7d8b1a83 Jan Kara 2023-06-21 2577 dev_t dev;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2578
ee9eea7d8b1a83 Jan Kara 2023-06-21 2579 mutex_lock(&bch_register_lock);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2580 if (lookup_bdev(strim(path), &dev) == 0 &&
ee9eea7d8b1a83 Jan Kara 2023-06-21 2581 bch_is_open(dev))
ee9eea7d8b1a83 Jan Kara 2023-06-21 2582 err = "device already registered";
ee9eea7d8b1a83 Jan Kara 2023-06-21 2583 else
ee9eea7d8b1a83 Jan Kara 2023-06-21 2584 err = "device busy";
ee9eea7d8b1a83 Jan Kara 2023-06-21 2585 mutex_unlock(&bch_register_lock);
ee9eea7d8b1a83 Jan Kara 2023-06-21 2586 if (attr == &ksysfs_register_quiet) {
ee9eea7d8b1a83 Jan Kara 2023-06-21 2587 quiet = true;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2588 ret = size;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2589 }
ee9eea7d8b1a83 Jan Kara 2023-06-21 2590 }
ee9eea7d8b1a83 Jan Kara 2023-06-21 2591 goto out_free_holder;
ee9eea7d8b1a83 Jan Kara 2023-06-21 2592 }
ee9eea7d8b1a83 Jan Kara 2023-06-21 2593
cc40daf91bdddb Tang Junhui 2018-03-05 2594 err = "failed to register device";
a58e88bfdc4d52 Coly Li 2020-10-01 2595
a58e88bfdc4d52 Coly Li 2020-10-01 2596 if (async_registration) {
9e23ccf8f0a22e Coly Li 2020-05-27 2597 /* register in asynchronous way */
9e23ccf8f0a22e Coly Li 2020-05-27 2598 struct async_reg_args *args =
9e23ccf8f0a22e Coly Li 2020-05-27 2599 kzalloc(sizeof(struct async_reg_args), GFP_KERNEL);
9e23ccf8f0a22e Coly Li 2020-05-27 2600
9e23ccf8f0a22e Coly Li 2020-05-27 2601 if (!args) {
9e23ccf8f0a22e Coly Li 2020-05-27 2602 ret = -ENOMEM;
9e23ccf8f0a22e Coly Li 2020-05-27 2603 err = "cannot allocate memory";
867f981b0739bd Jan Kara 2023-06-21 2604 goto out_free_holder;
9e23ccf8f0a22e Coly Li 2020-05-27 2605 }
9e23ccf8f0a22e Coly Li 2020-05-27 2606
9e23ccf8f0a22e Coly Li 2020-05-27 2607 args->path = path;
9e23ccf8f0a22e Coly Li 2020-05-27 2608 args->sb = sb;
9e23ccf8f0a22e Coly Li 2020-05-27 2609 args->sb_disk = sb_disk;
9e23ccf8f0a22e Coly Li 2020-05-27 2610 args->bdev = bdev;
867f981b0739bd Jan Kara 2023-06-21 2611 args->holder = holder;
d7fae7b4fa1527 Kai Krakow 2021-02-10 2612 register_device_async(args);
9e23ccf8f0a22e Coly Li 2020-05-27 2613 /* No wait and returns to user space */
9e23ccf8f0a22e Coly Li 2020-05-27 2614 goto async_done;
9e23ccf8f0a22e Coly Li 2020-05-27 2615 }
9e23ccf8f0a22e Coly Li 2020-05-27 2616
2903381fce7100 Kent Overstreet 2013-04-11 2617 if (SB_IS_BDEV(sb)) {
4fa03402cda2fa Kent Overstreet 2014-03-17 2618 mutex_lock(&bch_register_lock);
867f981b0739bd Jan Kara 2023-06-21 2619 ret = register_bdev(sb, sb_disk, bdev, holder);
4fa03402cda2fa Kent Overstreet 2014-03-17 2620 mutex_unlock(&bch_register_lock);
bb6d355c2aff42 Coly Li 2019-04-25 2621 /* blkdev_put() will be called in cached_dev_free() */
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2622 if (ret < 0)
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2623 goto out_free_sb;
cafe563591446c Kent Overstreet 2013-03-23 2624 } else {
bb6d355c2aff42 Coly Li 2019-04-25 2625 /* blkdev_put() will be called in bch_cache_release() */
867f981b0739bd Jan Kara 2023-06-21 2626 ret = register_cache(sb, sb_disk, bdev, holder);
d55f7cb2e5c053 Chao Yu 2021-10-20 2627 if (ret)
fc8f19cc5dce18 Christoph Hellwig 2020-01-24 2628 goto out_free_sb;
50246693f81fe8 Christoph Hellwig 2020-01-24 2629 }
50246693f81fe8 Christoph Hellwig 2020-01-24 2630
f59fce847fc848 Kent Overstreet 2013-05-15 2631 kfree(sb);
f59fce847fc848 Kent Overstreet 2013-05-15 2632 kfree(path);
f59fce847fc848 Kent Overstreet 2013-05-15 2633 module_put(THIS_MODULE);
9e23ccf8f0a22e Coly Li 2020-05-27 2634 async_done:
50246693f81fe8 Christoph Hellwig 2020-01-24 2635 return size;
f59fce847fc848 Kent Overstreet 2013-05-15 2636
867f981b0739bd Jan Kara 2023-06-21 2637 out_free_holder:
867f981b0739bd Jan Kara 2023-06-21 2638 kfree(holder);
50246693f81fe8 Christoph Hellwig 2020-01-24 2639 out_put_sb_page:
cfa0c56db9c087 Christoph Hellwig 2020-01-24 2640 put_page(virt_to_page(sb_disk));
50246693f81fe8 Christoph Hellwig 2020-01-24 2641 out_blkdev_put:
ee9eea7d8b1a83 Jan Kara 2023-06-21 2642 blkdev_put(bdev, holder);
50246693f81fe8 Christoph Hellwig 2020-01-24 2643 out_free_sb:
50246693f81fe8 Christoph Hellwig 2020-01-24 2644 kfree(sb);
50246693f81fe8 Christoph Hellwig 2020-01-24 2645 out_free_path:
50246693f81fe8 Christoph Hellwig 2020-01-24 2646 kfree(path);
ae3cd299919af6 Coly Li 2020-01-24 2647 path = NULL;
50246693f81fe8 Christoph Hellwig 2020-01-24 2648 out_module_put:
50246693f81fe8 Christoph Hellwig 2020-01-24 2649 module_put(THIS_MODULE);
50246693f81fe8 Christoph Hellwig 2020-01-24 2650 out:
ee9eea7d8b1a83 Jan Kara 2023-06-21 2651 if (!quiet)
46f5aa8806e34f Joe Perches 2020-05-27 2652 pr_info("error %s: %s\n", path?path:"", err);
50246693f81fe8 Christoph Hellwig 2020-01-24 2653 return ret;
cafe563591446c Kent Overstreet 2013-03-23 2654 }
cafe563591446c Kent Overstreet 2013-03-23 2655
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bcache: Alloc holder object before async registration
2023-06-21 17:56 ` Kent Overstreet
@ 2023-06-22 10:09 ` Jan Kara
2023-06-22 12:05 ` Kent Overstreet
0 siblings, 1 reply; 12+ messages in thread
From: Jan Kara @ 2023-06-22 10:09 UTC (permalink / raw
To: Kent Overstreet
Cc: Jan Kara, Jens Axboe, Christoph Hellwig, linux-block, Coly Li,
linux-bcache
On Wed 21-06-23 13:56:59, Kent Overstreet wrote:
> On Wed, Jun 21, 2023 at 06:23:26PM +0200, Jan Kara wrote:
> > Allocate holder object (cache or cached_dev) before offloading the
> > rest of the startup to async work. This will allow us to open the block
> > block device with proper holder.
>
> This is a pretty big change for this fix, and we'd want to retest the
> error paths - that's hard to do, because the fault injection framework I
> was using for that never made it upstream.
I agree those are somewhat difficult to test. Although with memory
allocation error injection, we can easily simulate failures in
alloc_holder_object() or say later in bcache_device_init() if that's what
you're after to give at least some testing to the error paths. Admittedly,
I've just tested that registering and unregistering bcache devices works
without giving warnings. Or are you more worried about the "reopen the
block device" logic (and error handling) in the second patch?
> What about just exposing a proper API for changing the holder? Wouldn't
> that be simpler?
It would be doable but frankly I'd prefer to avoid implementing the API for
changing the holder just for bcache. For all I care bcache can also just
generate random cookie (or an id from IDA) and pass it as a holder to
blkdev_get_by_dev(). It would do the job as well and we then don't have to
play games with changing the holder. It would just need to be propagated to
the places doing blkdev_put(). Do you find that better?
I'm now working on a changes that will make blkdev_get_by_dev() return
bdev_handle (which contains bdev pointer but also other stuff we need to
propagate to blkdev_put()) so when that is done, the cookie propagation
will happen automatically.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-22 1:23 ` kernel test robot
@ 2023-06-22 10:26 ` Jan Kara
0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2023-06-22 10:26 UTC (permalink / raw
To: kernel test robot
Cc: Jan Kara, Jens Axboe, oe-kbuild-all, Christoph Hellwig,
linux-block, Coly Li, linux-bcache
Hi,
On Thu 22-06-23 09:23:21, kernel test robot wrote:
> Hi Jan,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on axboe-block/for-6.5/block]
> [also build test ERROR on next-20230621]
> [cannot apply to linus/master v6.4-rc7]
> [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#_base_tree_information]
Yeah, there's missing '{' there. The two fixes were part of a larger series
and I've mistakenly fixed this up only in the next patch in the series.
I'll resend once we agree with Kent how to proceed.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bcache: Alloc holder object before async registration
2023-06-22 10:09 ` Jan Kara
@ 2023-06-22 12:05 ` Kent Overstreet
2023-06-22 15:14 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Kent Overstreet @ 2023-06-22 12:05 UTC (permalink / raw
To: Jan Kara
Cc: Jens Axboe, Christoph Hellwig, linux-block, Coly Li, linux-bcache
On Thu, Jun 22, 2023 at 12:09:54PM +0200, Jan Kara wrote:
> On Wed 21-06-23 13:56:59, Kent Overstreet wrote:
> > On Wed, Jun 21, 2023 at 06:23:26PM +0200, Jan Kara wrote:
> > > Allocate holder object (cache or cached_dev) before offloading the
> > > rest of the startup to async work. This will allow us to open the block
> > > block device with proper holder.
> >
> > This is a pretty big change for this fix, and we'd want to retest the
> > error paths - that's hard to do, because the fault injection framework I
> > was using for that never made it upstream.
>
> I agree those are somewhat difficult to test. Although with memory
> allocation error injection, we can easily simulate failures in
> alloc_holder_object() or say later in bcache_device_init() if that's what
> you're after to give at least some testing to the error paths. Admittedly,
> I've just tested that registering and unregistering bcache devices works
> without giving warnings. Or are you more worried about the "reopen the
> block device" logic (and error handling) in the second patch?
All error paths need to be tested, yeah.
> > What about just exposing a proper API for changing the holder? Wouldn't
> > that be simpler?
>
> It would be doable but frankly I'd prefer to avoid implementing the API for
> changing the holder just for bcache. For all I care bcache can also just
> generate random cookie (or an id from IDA) and pass it as a holder to
> blkdev_get_by_dev(). It would do the job as well and we then don't have to
> play games with changing the holder. It would just need to be propagated to
> the places doing blkdev_put(). Do you find that better?
Well, looking over the code it doesn't seem like it would really
simplify the fix, unfortunately.
bcachefs has the same issue, but in bcachefs we've already got a handle
object where we can allocate and stash a holder - analagous to the
bdev_handle you're working on.
> I'm now working on a changes that will make blkdev_get_by_dev() return
> bdev_handle (which contains bdev pointer but also other stuff we need to
> propagate to blkdev_put()) so when that is done, the cookie propagation
> will happen automatically.
bdev_handle definitely sounds like the right direction.
Just changing the holder really seems like it should be easiest to me,
and it can get deleted after bdev_handle lands, but maybe there's a new
wrinkle I'm not aware of?
Anyways, as the error paths get tested I'm ok with this patchset - it's
fine if it's done completely by hand (you can just add a goto fail in
the appropriate place and make sure nothing explodes).
Coli - we should talk about testing at some point. I've been working on
test infrastructure; it would be great to get the bcache tests in ktest
updated, I've now got a CI we could point at those tests.
Christoph - you really gotta start CCing people properly, the patch that
broke this didn't even it linux-bcache. No bueno.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] bcache: Fix bcache device claiming
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
` (2 preceding siblings ...)
2023-06-22 3:29 ` kernel test robot
@ 2023-06-22 15:12 ` Christoph Hellwig
3 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2023-06-22 15:12 UTC (permalink / raw
To: Jan Kara
Cc: Jens Axboe, Christoph Hellwig, linux-block, Coly Li, linux-bcache
On Wed, Jun 21, 2023 at 06:23:27PM +0200, Jan Kara wrote:
> Commit 2736e8eeb0cc ("block: use the holder as indication for exclusive
> opens") introduced a change that blkdev_put() has to get exclusive
> holder of the bdev as an argument. However it overlooked that
> register_bdev() and register_cache() overwrite the bdev->bd_holder field
> in the block device to point to the real owning object which was not
> available at the time we called blkdev_get_by_path(). Messing with bdev
> internals like this is a layering violation and it also causes
> blkdev_put() to issue warning about mismatching holders.
Ugg, yes.
> Fix bcache to reopen the block device with appropriate holder once it is
> available which also restores the behavior that multiple bcache caches
> cannot claim the same device which was also broken by commit
> 2736e8eeb0cc.
That was actually an intentional and documented change in commit
29499ab060fe ("bcache: don't pass a stack address to
blkdev_get_by_path") because the old behavior was broken already in
addition to the changing of the block stuff underneath. Not that I'm
arguing against your changes here, but the commit log probably needs a
bit of tweaking.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] bcache: Alloc holder object before async registration
2023-06-22 12:05 ` Kent Overstreet
@ 2023-06-22 15:14 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2023-06-22 15:14 UTC (permalink / raw
To: Kent Overstreet
Cc: Jan Kara, Jens Axboe, Christoph Hellwig, linux-block, Coly Li,
linux-bcache
On Thu, Jun 22, 2023 at 08:05:48AM -0400, Kent Overstreet wrote:
> Christoph - you really gotta start CCing people properly, the patch that
> broke this didn't even it linux-bcache. No bueno.
The series did absolutely go out to linux-bcache. See here for v2:
https://lore.kernel.org/linux-bcache/20230608110258.189493-13-hch@lst.de/T/#u
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-06-22 15:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-21 16:23 [PATCH 0/2] bcache: Fix block device claiming Jan Kara
2023-06-21 16:23 ` [PATCH 1/2] bcache: Alloc holder object before async registration Jan Kara
2023-06-21 17:56 ` Kent Overstreet
2023-06-22 10:09 ` Jan Kara
2023-06-22 12:05 ` Kent Overstreet
2023-06-22 15:14 ` Christoph Hellwig
2023-06-21 16:23 ` [PATCH 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 1:23 ` kernel test robot
2023-06-22 10:26 ` Jan Kara
2023-06-22 1:44 ` kernel test robot
2023-06-22 3:29 ` kernel test robot
2023-06-22 15:12 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).