* [PATCH] ublk_drv: fix request queue leak
@ 2022-07-14 10:32 Ming Lei
2022-07-14 13:00 ` Jens Axboe
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Ming Lei @ 2022-07-14 10:32 UTC (permalink / raw
To: Jens Axboe; +Cc: linux-block, Ming Lei
Call blk_cleanup_queue() in release code path for fixing request
queue leak.
Also for-5.20/block has cleaned up blk_cleanup_queue(), which is
basically merged to del_gendisk() if blk_mq_alloc_disk() is used
for allocating disk and queue.
However, ublk may not add disk in case of starting device failure, then
del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
will not be callsed, and it can be bit hard to deal with this kind of
merge conflict.
Turns out ublk's queue/disk use model is very similar with scsi, so switch
to scsi's model by allocating disk and queue independently, then it can be
quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
with blk_mq_destroy_queue.
Reported-by: Jens Axboe <axboe@kernel.dk>
Fixes: 3fee8d7599e1 ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
drivers/block/ublk_drv.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 35fa06ee70ff..eeeac43e1dc1 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -155,6 +155,8 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
static struct miscdevice ublk_misc;
+static struct lock_class_key ublk_bio_compl_lkclass;
+
static inline bool ublk_can_use_task_work(const struct ublk_queue *ubq)
{
if (IS_BUILTIN(CONFIG_BLK_DEV_UBLK) &&
@@ -634,7 +636,7 @@ static void ublk_commit_rqs(struct blk_mq_hw_ctx *hctx)
static int ublk_init_hctx(struct blk_mq_hw_ctx *hctx, void *driver_data,
unsigned int hctx_idx)
{
- struct ublk_device *ub = hctx->queue->queuedata;
+ struct ublk_device *ub = driver_data;
struct ublk_queue *ubq = ublk_get_queue(ub, hctx->queue_num);
hctx->driver_data = ubq;
@@ -1076,6 +1078,8 @@ static void ublk_cdev_rel(struct device *dev)
{
struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
+ blk_cleanup_queue(ub->ub_queue);
+
put_disk(ub->ub_disk);
blk_mq_free_tag_set(&ub->tag_set);
@@ -1165,14 +1169,17 @@ static int ublk_add_dev(struct ublk_device *ub)
if (err)
goto out_deinit_queues;
- disk = ub->ub_disk = blk_mq_alloc_disk(&ub->tag_set, ub);
+ ub->ub_queue = blk_mq_init_queue(&ub->tag_set);
+ if (IS_ERR(ub->ub_queue))
+ goto out_cleanup_tags;
+ ub->ub_queue->queuedata = ub;
+
+ disk = ub->ub_disk = __alloc_disk_node(ub->ub_queue, NUMA_NO_NODE,
+ &ublk_bio_compl_lkclass);
if (IS_ERR(disk)) {
err = PTR_ERR(disk);
- goto out_cleanup_tags;
+ goto out_free_request_queue;
}
- ub->ub_queue = ub->ub_disk->queue;
-
- ub->ub_queue->queuedata = ub;
blk_queue_logical_block_size(ub->ub_queue, bsize);
blk_queue_physical_block_size(ub->ub_queue, bsize);
@@ -1204,6 +1211,8 @@ static int ublk_add_dev(struct ublk_device *ub)
return 0;
+out_free_request_queue:
+ blk_cleanup_queue(ub->ub_queue);
out_cleanup_tags:
blk_mq_free_tag_set(&ub->tag_set);
out_deinit_queues:
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 10:32 [PATCH] ublk_drv: fix request queue leak Ming Lei
@ 2022-07-14 13:00 ` Jens Axboe
2022-07-14 13:10 ` Ming Lei
2022-07-14 13:13 ` Christoph Hellwig
2022-07-17 22:21 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2022-07-14 13:00 UTC (permalink / raw
To: Ming Lei; +Cc: linux-block
On 7/14/22 4:32 AM, Ming Lei wrote:
> Call blk_cleanup_queue() in release code path for fixing request
> queue leak.
>
> Also for-5.20/block has cleaned up blk_cleanup_queue(), which is
> basically merged to del_gendisk() if blk_mq_alloc_disk() is used
> for allocating disk and queue.
>
> However, ublk may not add disk in case of starting device failure, then
> del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
> will not be callsed, and it can be bit hard to deal with this kind of
> merge conflict.
>
> Turns out ublk's queue/disk use model is very similar with scsi, so switch
> to scsi's model by allocating disk and queue independently, then it can be
> quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
> with blk_mq_destroy_queue.
Tried this with the below incremental added to make it compile with
the core block changes too, and it still fails for me:
[ 22.488660] WARNING: CPU: 0 PID: 11 at block/blk-mq.c:3880 blk_mq_release+0xa4/0xf0
[ 22.490797] Modules linked in:
[ 22.491762] CPU: 0 PID: 11 Comm: kworker/0:1 Not tainted 5.19.0-rc6-00322-g42ed61fe42f3-dirty #1609
[ 22.494659] Hardware name: linux,dummy-virt (DT)
[ 22.496171] Workqueue: events blkg_free_workfn
[ 22.497652] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 22.499965] pc : blk_mq_release+0xa4/0xf0
[ 22.501386] lr : blk_mq_release+0x44/0xf0
[ 22.502748] sp : ffff80000af73cb0
[ 22.503880] x29: ffff80000af73cb0 x28: 0000000000000000 x27: 0000000000000000
[ 22.506263] x26: 0000000000000000 x25: ffff00001fe47b05 x24: 0000000000000000
[ 22.508655] x23: ffff0000052b6cb8 x22: ffff0000031e1c38 x21: 0000000000000000
[ 22.511035] x20: ffff0000031e1cf0 x19: ffff0000031e1bf0 x18: 0000000000000000
[ 22.513427] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa8000b80
[ 22.515814] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000001
[ 22.518209] x11: ffff80000945b7e8 x10: 0000000000006cb9 x9 : 00000000ffffffff
[ 22.520600] x8 : ffff800008fb5000 x7 : ffff80000860cf28 x6 : 0000000000000000
[ 22.522987] x5 : 0000000000000000 x4 : 0000000000000028 x3 : ffff80000af73c14
[ 22.525363] x2 : ffff0000071ccaa8 x1 : ffff0000071ccaa8 x0 : ffff0000071cc800
[ 22.527624] Call trace:
[ 22.528473] blk_mq_release+0xa4/0xf0
[ 22.529724] blk_release_queue+0x58/0xa0
[ 22.530946] kobject_put+0x84/0xe0
[ 22.531821] blk_put_queue+0x10/0x18
[ 22.532716] blkg_free_workfn+0x58/0x84
[ 22.533681] process_one_work+0x2ac/0x438
[ 22.534872] worker_thread+0x1cc/0x264
[ 22.535829] kthread+0xd0/0xe0
[ 22.536598] ret_from_fork+0x10/0x20
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index eeeac43e1dc1..d818da818c00 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1078,7 +1078,7 @@ static void ublk_cdev_rel(struct device *dev)
{
struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
- blk_cleanup_queue(ub->ub_queue);
+ blk_put_queue(ub->ub_queue);
put_disk(ub->ub_disk);
@@ -1174,8 +1174,8 @@ static int ublk_add_dev(struct ublk_device *ub)
goto out_cleanup_tags;
ub->ub_queue->queuedata = ub;
- disk = ub->ub_disk = __alloc_disk_node(ub->ub_queue, NUMA_NO_NODE,
- &ublk_bio_compl_lkclass);
+ disk = ub->ub_disk = blk_mq_alloc_disk_for_queue(ub->ub_queue,
+ &ublk_bio_compl_lkclass);
if (IS_ERR(disk)) {
err = PTR_ERR(disk);
goto out_free_request_queue;
@@ -1212,7 +1212,7 @@ static int ublk_add_dev(struct ublk_device *ub)
return 0;
out_free_request_queue:
- blk_cleanup_queue(ub->ub_queue);
+ blk_put_queue(ub->ub_queue);
out_cleanup_tags:
blk_mq_free_tag_set(&ub->tag_set);
out_deinit_queues:
--
Jens Axboe
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:00 ` Jens Axboe
@ 2022-07-14 13:10 ` Ming Lei
2022-07-14 13:14 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2022-07-14 13:10 UTC (permalink / raw
To: Jens Axboe; +Cc: linux-block, ming.lei
On Thu, Jul 14, 2022 at 07:00:59AM -0600, Jens Axboe wrote:
> On 7/14/22 4:32 AM, Ming Lei wrote:
> > Call blk_cleanup_queue() in release code path for fixing request
> > queue leak.
> >
> > Also for-5.20/block has cleaned up blk_cleanup_queue(), which is
> > basically merged to del_gendisk() if blk_mq_alloc_disk() is used
> > for allocating disk and queue.
> >
> > However, ublk may not add disk in case of starting device failure, then
> > del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
> > will not be callsed, and it can be bit hard to deal with this kind of
> > merge conflict.
> >
> > Turns out ublk's queue/disk use model is very similar with scsi, so switch
> > to scsi's model by allocating disk and queue independently, then it can be
> > quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
> > with blk_mq_destroy_queue.
>
> Tried this with the below incremental added to make it compile with
> the core block changes too, and it still fails for me:
>
> [ 22.488660] WARNING: CPU: 0 PID: 11 at block/blk-mq.c:3880 blk_mq_release+0xa4/0xf0
> [ 22.490797] Modules linked in:
> [ 22.491762] CPU: 0 PID: 11 Comm: kworker/0:1 Not tainted 5.19.0-rc6-00322-g42ed61fe42f3-dirty #1609
> [ 22.494659] Hardware name: linux,dummy-virt (DT)
> [ 22.496171] Workqueue: events blkg_free_workfn
> [ 22.497652] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 22.499965] pc : blk_mq_release+0xa4/0xf0
> [ 22.501386] lr : blk_mq_release+0x44/0xf0
> [ 22.502748] sp : ffff80000af73cb0
> [ 22.503880] x29: ffff80000af73cb0 x28: 0000000000000000 x27: 0000000000000000
> [ 22.506263] x26: 0000000000000000 x25: ffff00001fe47b05 x24: 0000000000000000
> [ 22.508655] x23: ffff0000052b6cb8 x22: ffff0000031e1c38 x21: 0000000000000000
> [ 22.511035] x20: ffff0000031e1cf0 x19: ffff0000031e1bf0 x18: 0000000000000000
> [ 22.513427] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa8000b80
> [ 22.515814] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000001
> [ 22.518209] x11: ffff80000945b7e8 x10: 0000000000006cb9 x9 : 00000000ffffffff
> [ 22.520600] x8 : ffff800008fb5000 x7 : ffff80000860cf28 x6 : 0000000000000000
> [ 22.522987] x5 : 0000000000000000 x4 : 0000000000000028 x3 : ffff80000af73c14
> [ 22.525363] x2 : ffff0000071ccaa8 x1 : ffff0000071ccaa8 x0 : ffff0000071cc800
> [ 22.527624] Call trace:
> [ 22.528473] blk_mq_release+0xa4/0xf0
> [ 22.529724] blk_release_queue+0x58/0xa0
> [ 22.530946] kobject_put+0x84/0xe0
> [ 22.531821] blk_put_queue+0x10/0x18
> [ 22.532716] blkg_free_workfn+0x58/0x84
> [ 22.533681] process_one_work+0x2ac/0x438
> [ 22.534872] worker_thread+0x1cc/0x264
> [ 22.535829] kthread+0xd0/0xe0
> [ 22.536598] ret_from_fork+0x10/0x20
>
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index eeeac43e1dc1..d818da818c00 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1078,7 +1078,7 @@ static void ublk_cdev_rel(struct device *dev)
> {
> struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
>
> - blk_cleanup_queue(ub->ub_queue);
> + blk_put_queue(ub->ub_queue);
I guess you run test on for-next, and it should work by just replacing
two blk_cleanup_queue with blk_mq_destroy_queue().
Thanks,
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 10:32 [PATCH] ublk_drv: fix request queue leak Ming Lei
2022-07-14 13:00 ` Jens Axboe
@ 2022-07-14 13:13 ` Christoph Hellwig
2022-07-14 13:20 ` Ming Lei
2022-07-17 22:21 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2022-07-14 13:13 UTC (permalink / raw
To: Ming Lei; +Cc: Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 06:32:01PM +0800, Ming Lei wrote:
> However, ublk may not add disk in case of starting device failure, then
> del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
> will not be callsed, and it can be bit hard to deal with this kind of
> merge conflict.
So base it on a tree that has everything you need.
> Turns out ublk's queue/disk use model is very similar with scsi, so switch
> to scsi's model by allocating disk and queue independently, then it can be
> quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
> with blk_mq_destroy_queue.
Don't do that. That thing really is a workaround for the lack of admin
queues in scsi. Nothing newly designed should use it. It will not
allow to optimize things and cause maintainaince burden down the road.
Please fix the lifetime problems properly.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:10 ` Ming Lei
@ 2022-07-14 13:14 ` Jens Axboe
2022-07-14 13:24 ` Christoph Hellwig
0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2022-07-14 13:14 UTC (permalink / raw
To: Ming Lei; +Cc: linux-block
On 7/14/22 7:10 AM, Ming Lei wrote:
> On Thu, Jul 14, 2022 at 07:00:59AM -0600, Jens Axboe wrote:
>> On 7/14/22 4:32 AM, Ming Lei wrote:
>>> Call blk_cleanup_queue() in release code path for fixing request
>>> queue leak.
>>>
>>> Also for-5.20/block has cleaned up blk_cleanup_queue(), which is
>>> basically merged to del_gendisk() if blk_mq_alloc_disk() is used
>>> for allocating disk and queue.
>>>
>>> However, ublk may not add disk in case of starting device failure, then
>>> del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
>>> will not be callsed, and it can be bit hard to deal with this kind of
>>> merge conflict.
>>>
>>> Turns out ublk's queue/disk use model is very similar with scsi, so switch
>>> to scsi's model by allocating disk and queue independently, then it can be
>>> quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
>>> with blk_mq_destroy_queue.
>>
>> Tried this with the below incremental added to make it compile with
>> the core block changes too, and it still fails for me:
>>
>> [ 22.488660] WARNING: CPU: 0 PID: 11 at block/blk-mq.c:3880 blk_mq_release+0xa4/0xf0
>> [ 22.490797] Modules linked in:
>> [ 22.491762] CPU: 0 PID: 11 Comm: kworker/0:1 Not tainted 5.19.0-rc6-00322-g42ed61fe42f3-dirty #1609
>> [ 22.494659] Hardware name: linux,dummy-virt (DT)
>> [ 22.496171] Workqueue: events blkg_free_workfn
>> [ 22.497652] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [ 22.499965] pc : blk_mq_release+0xa4/0xf0
>> [ 22.501386] lr : blk_mq_release+0x44/0xf0
>> [ 22.502748] sp : ffff80000af73cb0
>> [ 22.503880] x29: ffff80000af73cb0 x28: 0000000000000000 x27: 0000000000000000
>> [ 22.506263] x26: 0000000000000000 x25: ffff00001fe47b05 x24: 0000000000000000
>> [ 22.508655] x23: ffff0000052b6cb8 x22: ffff0000031e1c38 x21: 0000000000000000
>> [ 22.511035] x20: ffff0000031e1cf0 x19: ffff0000031e1bf0 x18: 0000000000000000
>> [ 22.513427] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa8000b80
>> [ 22.515814] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000001
>> [ 22.518209] x11: ffff80000945b7e8 x10: 0000000000006cb9 x9 : 00000000ffffffff
>> [ 22.520600] x8 : ffff800008fb5000 x7 : ffff80000860cf28 x6 : 0000000000000000
>> [ 22.522987] x5 : 0000000000000000 x4 : 0000000000000028 x3 : ffff80000af73c14
>> [ 22.525363] x2 : ffff0000071ccaa8 x1 : ffff0000071ccaa8 x0 : ffff0000071cc800
>> [ 22.527624] Call trace:
>> [ 22.528473] blk_mq_release+0xa4/0xf0
>> [ 22.529724] blk_release_queue+0x58/0xa0
>> [ 22.530946] kobject_put+0x84/0xe0
>> [ 22.531821] blk_put_queue+0x10/0x18
>> [ 22.532716] blkg_free_workfn+0x58/0x84
>> [ 22.533681] process_one_work+0x2ac/0x438
>> [ 22.534872] worker_thread+0x1cc/0x264
>> [ 22.535829] kthread+0xd0/0xe0
>> [ 22.536598] ret_from_fork+0x10/0x20
>>
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index eeeac43e1dc1..d818da818c00 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -1078,7 +1078,7 @@ static void ublk_cdev_rel(struct device *dev)
>> {
>> struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
>>
>> - blk_cleanup_queue(ub->ub_queue);
>> + blk_put_queue(ub->ub_queue);
>
> I guess you run test on for-next, and it should work by just replacing
> two blk_cleanup_queue with blk_mq_destroy_queue().
Ah yes, that does the trick. I think I'll migrate the driver to the core
branch instead to avoid these issues.
--
Jens Axboe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:13 ` Christoph Hellwig
@ 2022-07-14 13:20 ` Ming Lei
2022-07-14 13:23 ` Christoph Hellwig
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2022-07-14 13:20 UTC (permalink / raw
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 06:13:41AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 14, 2022 at 06:32:01PM +0800, Ming Lei wrote:
> > However, ublk may not add disk in case of starting device failure, then
> > del_gendisk() won't be called when removing ublk device, so blk_mq_exit_queue
> > will not be callsed, and it can be bit hard to deal with this kind of
> > merge conflict.
>
> So base it on a tree that has everything you need.
>
> > Turns out ublk's queue/disk use model is very similar with scsi, so switch
> > to scsi's model by allocating disk and queue independently, then it can be
> > quite easy to handle v5.20 merge conflict by replacing blk_cleanup_queue
> > with blk_mq_destroy_queue.
>
> Don't do that. That thing really is a workaround for the lack of admin
> queues in scsi. Nothing newly designed should use it. It will not
> allow to optimize things and cause maintainaince burden down the road.
The problem is that you moved part of blk_cleanup_queue() into
del_gendisk().
Here, the issue Jens reproduced is that we don't add disk yet, so won't
call del_gendisk(). The queue & disk is allocated & initialized correctly.
Then how to do the part done by original blk_cleanup_queue() without calling
blk_mq_destroy_queue()?
Thanks,
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:20 ` Ming Lei
@ 2022-07-14 13:23 ` Christoph Hellwig
2022-07-14 13:26 ` Ming Lei
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2022-07-14 13:23 UTC (permalink / raw
To: Ming Lei; +Cc: Christoph Hellwig, Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 09:20:24PM +0800, Ming Lei wrote:
> The problem is that you moved part of blk_cleanup_queue() into
> del_gendisk().
>
> Here, the issue Jens reproduced is that we don't add disk yet, so won't
> call del_gendisk(). The queue & disk is allocated & initialized correctly.
>
> Then how to do the part done by original blk_cleanup_queue() without calling
> blk_mq_destroy_queue()?
What do you need to clean up? put_disk is supposed to eventually
clean up everything allocated by blk_alloc_disk through disk_release.
If it fails to cleanup anything that is a bug we need to fix in the core
as it will affect all drivers.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:14 ` Jens Axboe
@ 2022-07-14 13:24 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2022-07-14 13:24 UTC (permalink / raw
To: Jens Axboe; +Cc: Ming Lei, linux-block
On Thu, Jul 14, 2022 at 07:14:52AM -0600, Jens Axboe wrote:
> >> - blk_cleanup_queue(ub->ub_queue);
> >> + blk_put_queue(ub->ub_queue);
> >
> > I guess you run test on for-next, and it should work by just replacing
> > two blk_cleanup_queue with blk_mq_destroy_queue().
>
> Ah yes, that does the trick. I think I'll migrate the driver to the core
> branch instead to avoid these issues.
Please drop it for now. It's pretty clear it did not have enough
review yet. I'll try to allocate some time today or tomorrow to
go through it.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:23 ` Christoph Hellwig
@ 2022-07-14 13:26 ` Ming Lei
2022-07-14 13:37 ` Ming Lei
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2022-07-14 13:26 UTC (permalink / raw
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 06:23:12AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 14, 2022 at 09:20:24PM +0800, Ming Lei wrote:
> > The problem is that you moved part of blk_cleanup_queue() into
> > del_gendisk().
> >
> > Here, the issue Jens reproduced is that we don't add disk yet, so won't
> > call del_gendisk(). The queue & disk is allocated & initialized correctly.
> >
> > Then how to do the part done by original blk_cleanup_queue() without calling
> > blk_mq_destroy_queue()?
>
> What do you need to clean up? put_disk is supposed to eventually
> clean up everything allocated by blk_alloc_disk through disk_release.
> If it fails to cleanup anything that is a bug we need to fix in the core
> as it will affect all drivers.
The part to be cleaned up is nothing to do with disk:
if (queue_is_mq(q))
blk_mq_exit_queue(q);
->exit_hctx() is called in blk_mq_exit_queue().
Without calling blk_mq_destroy_queue, I don't see other way to address
this issue, or suggestions?
Thanks,
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:26 ` Ming Lei
@ 2022-07-14 13:37 ` Ming Lei
2022-07-14 13:55 ` Christoph Hellwig
0 siblings, 1 reply; 13+ messages in thread
From: Ming Lei @ 2022-07-14 13:37 UTC (permalink / raw
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 09:26:25PM +0800, Ming Lei wrote:
> On Thu, Jul 14, 2022 at 06:23:12AM -0700, Christoph Hellwig wrote:
> > On Thu, Jul 14, 2022 at 09:20:24PM +0800, Ming Lei wrote:
> > > The problem is that you moved part of blk_cleanup_queue() into
> > > del_gendisk().
> > >
> > > Here, the issue Jens reproduced is that we don't add disk yet, so won't
> > > call del_gendisk(). The queue & disk is allocated & initialized correctly.
> > >
> > > Then how to do the part done by original blk_cleanup_queue() without calling
> > > blk_mq_destroy_queue()?
> >
> > What do you need to clean up? put_disk is supposed to eventually
> > clean up everything allocated by blk_alloc_disk through disk_release.
> > If it fails to cleanup anything that is a bug we need to fix in the core
> > as it will affect all drivers.
>
> The part to be cleaned up is nothing to do with disk:
>
> if (queue_is_mq(q))
> blk_mq_exit_queue(q);
>
> ->exit_hctx() is called in blk_mq_exit_queue().
>
> Without calling blk_mq_destroy_queue, I don't see other way to address
> this issue, or suggestions?
It is actually one big problem of 6f8191fdf41d ("block: simplify disk shutdown")
since blk_put_queue() can't do what blk_cleanup_queue() did.
Anywhere using blk_put_queue() to release blk-mq queue before adding
disk has the same issue.
Thanks,
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:37 ` Ming Lei
@ 2022-07-14 13:55 ` Christoph Hellwig
2022-07-14 14:02 ` Ming Lei
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2022-07-14 13:55 UTC (permalink / raw
To: Ming Lei; +Cc: Christoph Hellwig, Jens Axboe, linux-block
On Thu, Jul 14, 2022 at 09:37:10PM +0800, Ming Lei wrote:
> It is actually one big problem of 6f8191fdf41d ("block: simplify disk shutdown")
> since blk_put_queue() can't do what blk_cleanup_queue() did.
>
> Anywhere using blk_put_queue() to release blk-mq queue before adding
> disk has the same issue.
And the reason why blk_put_queue can't do is seems to be mostly because
queues don't hold a reference on the tag set (and tag_sets don't have
a reference at all). Which has caused us a bunch of issues before, so
let me see if I can fix that properly.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 13:55 ` Christoph Hellwig
@ 2022-07-14 14:02 ` Ming Lei
0 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2022-07-14 14:02 UTC (permalink / raw
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, ming.lei
On Thu, Jul 14, 2022 at 06:55:28AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 14, 2022 at 09:37:10PM +0800, Ming Lei wrote:
> > It is actually one big problem of 6f8191fdf41d ("block: simplify disk shutdown")
> > since blk_put_queue() can't do what blk_cleanup_queue() did.
> >
> > Anywhere using blk_put_queue() to release blk-mq queue before adding
> > disk has the same issue.
>
> And the reason why blk_put_queue can't do is seems to be mostly because
> queues don't hold a reference on the tag set (and tag_sets don't have
Exactly.
> a reference at all). Which has caused us a bunch of issues before, so
> let me see if I can fix that properly.
I guess it is hard to fix, not get any idea yet. Originally we stop to
referring tagset after blk_cleanup_queue(), but now you are trying to
kill blk_cleanup_queue()...
thanks,
Ming
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ublk_drv: fix request queue leak
2022-07-14 10:32 [PATCH] ublk_drv: fix request queue leak Ming Lei
2022-07-14 13:00 ` Jens Axboe
2022-07-14 13:13 ` Christoph Hellwig
@ 2022-07-17 22:21 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2022-07-17 22:21 UTC (permalink / raw
To: Ming Lei; +Cc: llvm, kbuild-all
Hi Ming,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on axboe-block/for-next]
[cannot apply to linus/master v5.19-rc7 next-20220715]
[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/Ming-Lei/ublk_drv-fix-request-queue-leak/20220714-183546
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20220718/202207180650.FHQQqTxN-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/98d681f8eed174b29ee9e72ed6a7d04e1a77835b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ming-Lei/ublk_drv-fix-request-queue-leak/20220714-183546
git checkout 98d681f8eed174b29ee9e72ed6a7d04e1a77835b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/block/ drivers/media/i2c/ drivers/net/pcs/ drivers/pinctrl/nuvoton/ kernel/trace/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/block/ublk_drv.c:890:6: warning: variable 'io' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (tag >= ubq->q_depth)
^~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:890:2: note: remove the 'if' if its condition is always false
if (tag >= ubq->q_depth)
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:887:6: warning: variable 'io' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (ubq->ubq_daemon && ubq->ubq_daemon != current)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:887:2: note: remove the 'if' if its condition is always false
if (ubq->ubq_daemon && ubq->ubq_daemon != current)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:884:6: warning: variable 'io' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!ubq || ub_cmd->q_id != ubq->q_id)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:884:2: note: remove the 'if' if its condition is always false
if (!ubq || ub_cmd->q_id != ubq->q_id)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:884:6: warning: variable 'io' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!ubq || ub_cmd->q_id != ubq->q_id)
^~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:884:6: note: remove the '||' if its condition is always false
if (!ubq || ub_cmd->q_id != ubq->q_id)
^~~~~~~
drivers/block/ublk_drv.c:880:6: warning: variable 'io' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:880:2: note: remove the 'if' if its condition is always false
if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:877:6: warning: variable 'io' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!(issue_flags & IO_URING_F_SQE128))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:940:2: note: uninitialized use occurs here
io->flags &= ~UBLK_IO_FLAG_ACTIVE;
^~
drivers/block/ublk_drv.c:877:2: note: remove the 'if' if its condition is always false
if (!(issue_flags & IO_URING_F_SQE128))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:868:20: note: initialize the variable 'io' to silence this warning
struct ublk_io *io;
^
= NULL
>> drivers/block/ublk_drv.c:1081:2: error: call to undeclared function 'blk_cleanup_queue'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
blk_cleanup_queue(ub->ub_queue);
^
>> drivers/block/ublk_drv.c:1177:23: error: call to undeclared function '__alloc_disk_node'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
disk = ub->ub_disk = __alloc_disk_node(ub->ub_queue, NUMA_NO_NODE,
^
drivers/block/ublk_drv.c:1177:23: note: did you mean '__alloc_pages_node'?
include/linux/gfp.h:582:1: note: '__alloc_pages_node' declared here
__alloc_pages_node(int nid, gfp_t gfp_mask, unsigned int order)
^
>> drivers/block/ublk_drv.c:1177:21: warning: incompatible integer to pointer conversion assigning to 'struct gendisk *' from 'int' [-Wint-conversion]
disk = ub->ub_disk = __alloc_disk_node(ub->ub_queue, NUMA_NO_NODE,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/block/ublk_drv.c:1215:2: error: call to undeclared function 'blk_cleanup_queue'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
blk_cleanup_queue(ub->ub_queue);
^
7 warnings and 3 errors generated.
vim +/blk_cleanup_queue +1081 drivers/block/ublk_drv.c
1076
1077 static void ublk_cdev_rel(struct device *dev)
1078 {
1079 struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
1080
> 1081 blk_cleanup_queue(ub->ub_queue);
1082
1083 put_disk(ub->ub_disk);
1084
1085 blk_mq_free_tag_set(&ub->tag_set);
1086
1087 ublk_deinit_queues(ub);
1088
1089 __ublk_destroy_dev(ub);
1090 }
1091
1092 static int ublk_add_chdev(struct ublk_device *ub)
1093 {
1094 struct device *dev = &ub->cdev_dev;
1095 int minor = ub->ub_number;
1096 int ret;
1097
1098 dev->parent = ublk_misc.this_device;
1099 dev->devt = MKDEV(MAJOR(ublk_chr_devt), minor);
1100 dev->class = ublk_chr_class;
1101 dev->release = ublk_cdev_rel;
1102 device_initialize(dev);
1103
1104 ret = dev_set_name(dev, "ublkc%d", minor);
1105 if (ret)
1106 goto fail;
1107
1108 cdev_init(&ub->cdev, &ublk_ch_fops);
1109 ret = cdev_device_add(&ub->cdev, dev);
1110 if (ret)
1111 goto fail;
1112 return 0;
1113 fail:
1114 put_device(dev);
1115 return ret;
1116 }
1117
1118 static void ublk_stop_work_fn(struct work_struct *work)
1119 {
1120 struct ublk_device *ub =
1121 container_of(work, struct ublk_device, stop_work);
1122
1123 ublk_stop_dev(ub);
1124 }
1125
1126 static void ublk_update_capacity(struct ublk_device *ub)
1127 {
1128 unsigned int max_rq_bytes;
1129
1130 /* make max request buffer size aligned with PAGE_SIZE */
1131 max_rq_bytes = round_down(ub->dev_info.rq_max_blocks <<
1132 ub->bs_shift, PAGE_SIZE);
1133 ub->dev_info.rq_max_blocks = max_rq_bytes >> ub->bs_shift;
1134
1135 set_capacity(ub->ub_disk, ub->dev_info.dev_blocks << (ub->bs_shift - 9));
1136 }
1137
1138 /* add disk & cdev, cleanup everything in case of failure */
1139 static int ublk_add_dev(struct ublk_device *ub)
1140 {
1141 struct gendisk *disk;
1142 int err = -ENOMEM;
1143 int bsize;
1144
1145 /* We are not ready to support zero copy */
1146 ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
1147
1148 bsize = ub->dev_info.block_size;
1149 ub->bs_shift = ilog2(bsize);
1150
1151 ub->dev_info.nr_hw_queues = min_t(unsigned int,
1152 ub->dev_info.nr_hw_queues, nr_cpu_ids);
1153
1154 INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
1155 INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
1156
1157 if (ublk_init_queues(ub))
1158 goto out_destroy_dev;
1159
1160 ub->tag_set.ops = &ublk_mq_ops;
1161 ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
1162 ub->tag_set.queue_depth = ub->dev_info.queue_depth;
1163 ub->tag_set.numa_node = NUMA_NO_NODE;
1164 ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
1165 ub->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
1166 ub->tag_set.driver_data = ub;
1167
1168 err = blk_mq_alloc_tag_set(&ub->tag_set);
1169 if (err)
1170 goto out_deinit_queues;
1171
1172 ub->ub_queue = blk_mq_init_queue(&ub->tag_set);
1173 if (IS_ERR(ub->ub_queue))
1174 goto out_cleanup_tags;
1175 ub->ub_queue->queuedata = ub;
1176
> 1177 disk = ub->ub_disk = __alloc_disk_node(ub->ub_queue, NUMA_NO_NODE,
1178 &ublk_bio_compl_lkclass);
1179 if (IS_ERR(disk)) {
1180 err = PTR_ERR(disk);
1181 goto out_free_request_queue;
1182 }
1183
1184 blk_queue_logical_block_size(ub->ub_queue, bsize);
1185 blk_queue_physical_block_size(ub->ub_queue, bsize);
1186 blk_queue_io_min(ub->ub_queue, bsize);
1187
1188 blk_queue_max_hw_sectors(ub->ub_queue, ub->dev_info.rq_max_blocks <<
1189 (ub->bs_shift - 9));
1190
1191 ub->ub_queue->limits.discard_granularity = PAGE_SIZE;
1192
1193 blk_queue_max_discard_sectors(ub->ub_queue, UINT_MAX >> 9);
1194 blk_queue_max_write_zeroes_sectors(ub->ub_queue, UINT_MAX >> 9);
1195
1196 ublk_update_capacity(ub);
1197
1198 disk->fops = &ub_fops;
1199 disk->private_data = ub;
1200 disk->queue = ub->ub_queue;
1201 sprintf(disk->disk_name, "ublkb%d", ub->ub_number);
1202
1203 mutex_init(&ub->mutex);
1204
1205 /* add char dev so that ublksrv daemon can be setup */
1206 err = ublk_add_chdev(ub);
1207 if (err)
1208 return err;
1209
1210 /* don't expose disk now until we got start command from cdev */
1211
1212 return 0;
1213
1214 out_free_request_queue:
1215 blk_cleanup_queue(ub->ub_queue);
1216 out_cleanup_tags:
1217 blk_mq_free_tag_set(&ub->tag_set);
1218 out_deinit_queues:
1219 ublk_deinit_queues(ub);
1220 out_destroy_dev:
1221 __ublk_destroy_dev(ub);
1222 return err;
1223 }
1224
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-07-17 22:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-14 10:32 [PATCH] ublk_drv: fix request queue leak Ming Lei
2022-07-14 13:00 ` Jens Axboe
2022-07-14 13:10 ` Ming Lei
2022-07-14 13:14 ` Jens Axboe
2022-07-14 13:24 ` Christoph Hellwig
2022-07-14 13:13 ` Christoph Hellwig
2022-07-14 13:20 ` Ming Lei
2022-07-14 13:23 ` Christoph Hellwig
2022-07-14 13:26 ` Ming Lei
2022-07-14 13:37 ` Ming Lei
2022-07-14 13:55 ` Christoph Hellwig
2022-07-14 14:02 ` Ming Lei
2022-07-17 22:21 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.