* [PATCH v2 0/2] bcache: Fix block device claiming
@ 2023-06-22 16:46 Jan Kara
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jan Kara @ 2023-06-22 16:46 UTC (permalink / raw
To: Coly Li
Cc: linux-bcache, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet, 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.
This time I've actually tested various modified error handling paths (which was
good because one of them was indeed wrong!).
Jens, please consider merging these fixes before sending Linus a pull request
with blkdev changes.
Changes since v1:
* Fix compile breakage spotted by 0-day
* Fix error handling path when the second blkdev_get_by_dev() fails
* Fix commit message of patch 2/2
Honza
Previous versions:
Link: http://lore.kernel.org/r/20230621162024.29310-1-jack@suse.cz # v1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] bcache: Alloc holder object before async registration
2023-06-22 16:46 [PATCH v2 0/2] bcache: Fix block device claiming Jan Kara
@ 2023-06-22 16:46 ` Jan Kara
2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:56 ` Coly Li
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-23 22:10 ` [PATCH v2 0/2] bcache: Fix block " Jens Axboe
2 siblings, 2 replies; 9+ messages in thread
From: Jan Kara @ 2023-06-22 16:46 UTC (permalink / raw
To: Coly Li
Cc: linux-bcache, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet, 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] 9+ messages in thread
* [PATCH v2 2/2] bcache: Fix bcache device claiming
2023-06-22 16:46 [PATCH v2 0/2] bcache: Fix block device claiming Jan Kara
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
@ 2023-06-22 16:46 ` Jan Kara
2023-06-22 16:52 ` Kent Overstreet
` (2 more replies)
2023-06-23 22:10 ` [PATCH v2 0/2] bcache: Fix block " Jens Axboe
2 siblings, 3 replies; 9+ messages in thread
From: Jan Kara @ 2023-06-22 16:46 UTC (permalink / raw
To: Coly Li
Cc: linux-bcache, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet, 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 broken by commit 29499ab060fe
("bcache: don't pass a stack address to blkdev_get_by_path").
Fixes: 2736e8eeb0cc ("block: use the holder as indication for exclusive opens")
Signed-off-by: Jan Kara <jack@suse.cz>
---
drivers/md/bcache/super.c | 65 +++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 913dd94353b6..0ae2b3676293 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,32 @@ 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);
+ bdev = NULL;
+ if (ret == -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 +2629,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 +2640,8 @@ 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);
+ if (bdev)
+ blkdev_put(bdev, holder);
out_free_sb:
kfree(sb);
out_free_path:
@@ -2640,7 +2650,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] 9+ messages in thread
* Re: [PATCH v2 1/2] bcache: Alloc holder object before async registration
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
@ 2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:56 ` Coly Li
1 sibling, 0 replies; 9+ messages in thread
From: Kent Overstreet @ 2023-06-22 16:52 UTC (permalink / raw
To: Jan Kara
Cc: Coly Li, linux-bcache, Jens Axboe, Christoph Hellwig, linux-block
On Thu, Jun 22, 2023 at 06:46:54PM +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.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
> 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] 9+ messages in thread
* Re: [PATCH v2 2/2] bcache: Fix bcache device claiming
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
@ 2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:55 ` Coly Li
2023-06-22 16:59 ` Coly Li
2 siblings, 0 replies; 9+ messages in thread
From: Kent Overstreet @ 2023-06-22 16:52 UTC (permalink / raw
To: Jan Kara
Cc: Coly Li, linux-bcache, Jens Axboe, Christoph Hellwig, linux-block
On Thu, Jun 22, 2023 at 06:46:55PM +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.
>
> 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 broken by commit 29499ab060fe
> ("bcache: don't pass a stack address to blkdev_get_by_path").
>
> Fixes: 2736e8eeb0cc ("block: use the holder as indication for exclusive opens")
> Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
> drivers/md/bcache/super.c | 65 +++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 913dd94353b6..0ae2b3676293 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,32 @@ 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);
> + bdev = NULL;
> + if (ret == -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 +2629,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 +2640,8 @@ 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);
> + if (bdev)
> + blkdev_put(bdev, holder);
> out_free_sb:
> kfree(sb);
> out_free_path:
> @@ -2640,7 +2650,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 [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] bcache: Fix bcache device claiming
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 16:52 ` Kent Overstreet
@ 2023-06-22 16:55 ` Coly Li
2023-06-22 16:59 ` Coly Li
2 siblings, 0 replies; 9+ messages in thread
From: Coly Li @ 2023-06-22 16:55 UTC (permalink / raw
To: Jan Kara
Cc: Bcache Linux, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet
On Thu, Jun 22, 2023 at 06:46:55PM +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.
>
> 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 broken by commit 29499ab060fe
> ("bcache: don't pass a stack address to blkdev_get_by_path").
>
> Fixes: 2736e8eeb0cc ("block: use the holder as indication for exclusive opens")
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Coly Li <colyli@suse.de>
Thanks.
Coly Li
> ---
> drivers/md/bcache/super.c | 65 +++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 913dd94353b6..0ae2b3676293 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,32 @@ 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);
> + bdev = NULL;
> + if (ret == -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 +2629,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 +2640,8 @@ 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);
> + if (bdev)
> + blkdev_put(bdev, holder);
> out_free_sb:
> kfree(sb);
> out_free_path:
> @@ -2640,7 +2650,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
>
--
Coly Li
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] bcache: Alloc holder object before async registration
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
2023-06-22 16:52 ` Kent Overstreet
@ 2023-06-22 16:56 ` Coly Li
1 sibling, 0 replies; 9+ messages in thread
From: Coly Li @ 2023-06-22 16:56 UTC (permalink / raw
To: Jan Kara
Cc: Bcache Linux, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet
On Thu, Jun 22, 2023 at 06:46:54PM +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.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Coly Li <colyli@suse.de>
Thanks.
Coly Li
> ---
> 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
>
--
Coly Li
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] bcache: Fix bcache device claiming
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:55 ` Coly Li
@ 2023-06-22 16:59 ` Coly Li
2 siblings, 0 replies; 9+ messages in thread
From: Coly Li @ 2023-06-22 16:59 UTC (permalink / raw
To: Jan Kara
Cc: linux-bcache, Jens Axboe, Christoph Hellwig, linux-block,
Kent Overstreet
On Thu, Jun 22, 2023 at 06:46:55PM +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.
>
> 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 broken by commit 29499ab060fe
> ("bcache: don't pass a stack address to blkdev_get_by_path").
>
> Fixes: 2736e8eeb0cc ("block: use the holder as indication for exclusive opens")
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Coly Li <colyli@suse.de>
Thanks.
Coly Li
> ---
> drivers/md/bcache/super.c | 65 +++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 913dd94353b6..0ae2b3676293 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,32 @@ 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);
> + bdev = NULL;
> + if (ret == -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 +2629,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 +2640,8 @@ 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);
> + if (bdev)
> + blkdev_put(bdev, holder);
> out_free_sb:
> kfree(sb);
> out_free_path:
> @@ -2640,7 +2650,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
>
--
Coly Li
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/2] bcache: Fix block device claiming
2023-06-22 16:46 [PATCH v2 0/2] bcache: Fix block device claiming Jan Kara
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
@ 2023-06-23 22:10 ` Jens Axboe
2 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2023-06-23 22:10 UTC (permalink / raw
To: Coly Li, Jan Kara
Cc: linux-bcache, Christoph Hellwig, linux-block, Kent Overstreet
On Thu, 22 Jun 2023 18:46:53 +0200, Jan Kara wrote:
> 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.
>
> This time I've actually tested various modified error handling paths (which was
> good because one of them was indeed wrong!).
>
> [...]
Applied, thanks!
[1/2] bcache: Alloc holder object before async registration
commit: abcc0cbd49283fccd20420e86416b2475b00819c
[2/2] bcache: Fix bcache device claiming
commit: 2c5555983bd27d24162534b682b10654639a5576
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-23 22:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-22 16:46 [PATCH v2 0/2] bcache: Fix block device claiming Jan Kara
2023-06-22 16:46 ` [PATCH v2 1/2] bcache: Alloc holder object before async registration Jan Kara
2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:56 ` Coly Li
2023-06-22 16:46 ` [PATCH v2 2/2] bcache: Fix bcache device claiming Jan Kara
2023-06-22 16:52 ` Kent Overstreet
2023-06-22 16:55 ` Coly Li
2023-06-22 16:59 ` Coly Li
2023-06-23 22:10 ` [PATCH v2 0/2] bcache: Fix block " Jens Axboe
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).