* [PATCH 1/2] blk-mq: fix leak of q->stats
@ 2017-03-27 17:43 Omar Sandoval
2017-03-27 17:43 ` [PATCH 2/2] block: fix leak of q->rq_wb Omar Sandoval
2017-03-28 2:43 ` [PATCH 1/2] blk-mq: fix leak of q->stats Ming Lei
0 siblings, 2 replies; 6+ messages in thread
From: Omar Sandoval @ 2017-03-27 17:43 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
From: Omar Sandoval <osandov@fb.com>
blk_alloc_queue_node() already allocates q->stats, so
blk_mq_init_allocated_queue() is overwriting it with a new allocation.
Fixes: a83b576c9c25 ("block: fix stacked driver stats init and free")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index c212b9644a9f..9c6b39bd8358 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2212,10 +2212,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
/* mark the queue as mq asap */
q->mq_ops = set->ops;
- q->stats = blk_alloc_queue_stats();
- if (!q->stats)
- goto err_exit;
-
q->poll_cb = blk_stat_alloc_callback(blk_mq_poll_stats_fn,
blk_stat_rq_ddir, 2, q);
if (!q->poll_cb)
--
2.12.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] block: fix leak of q->rq_wb
2017-03-27 17:43 [PATCH 1/2] blk-mq: fix leak of q->stats Omar Sandoval
@ 2017-03-27 17:43 ` Omar Sandoval
2017-03-28 2:52 ` Omar Sandoval
2017-03-28 3:43 ` Ming Lei
2017-03-28 2:43 ` [PATCH 1/2] blk-mq: fix leak of q->stats Ming Lei
1 sibling, 2 replies; 6+ messages in thread
From: Omar Sandoval @ 2017-03-27 17:43 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
From: Omar Sandoval <osandov@fb.com>
CONFIG_DEBUG_TEST_DRIVER_REMOVE found a possible leak of q->rq_wb in a
couple of cases: when a request queue is reregistered and when gendisks
share a request_queue. This has been a problem since wbt was introduced,
but the WARN_ON(!list_empty(&stats->callbacks)) in the blk-stat rework
exposed it. The fix is unfortunately a hack until we fix all of the
drivers sharing a request_queue.
Fixes: 87760e5eef35 ("block: hook up writeback throttling")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-sysfs.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index fa831cb2fc30..a187e3f70028 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -893,7 +893,21 @@ int blk_register_queue(struct gendisk *disk)
kobject_uevent(&q->kobj, KOBJ_ADD);
- blk_wb_init(q);
+ /*
+ * There are two cases where wbt may have already been initialized:
+ * 1. A call sequence of blk_register_queue(); blk_unregister_queue();
+ * blk_register_queue().
+ * 2. Multiple gendisks sharing a request_queue.
+ *
+ * To fix case 1, we'd like to call wbt_exit() in
+ * blk_unregister_queue(). However, that's unsafe for case 2. So, we're
+ * forced to do this and call wbt_exit() in blk_release_queue() instead.
+ *
+ * Note that in case 2, wbt will account across disks until those legacy
+ * drivers are fixed.
+ */
+ if (!q->rq_wb)
+ blk_wb_init(q);
if (q->request_fn || (q->mq_ops && q->elevator)) {
ret = elv_register_queue(q);
--
2.12.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] block: fix leak of q->rq_wb
2017-03-27 17:43 ` [PATCH 2/2] block: fix leak of q->rq_wb Omar Sandoval
@ 2017-03-28 2:52 ` Omar Sandoval
2017-03-28 3:43 ` Ming Lei
1 sibling, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2017-03-28 2:52 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
On Mon, Mar 27, 2017 at 10:43:59AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> CONFIG_DEBUG_TEST_DRIVER_REMOVE found a possible leak of q->rq_wb in a
> couple of cases: when a request queue is reregistered and when gendisks
> share a request_queue. This has been a problem since wbt was introduced,
> but the WARN_ON(!list_empty(&stats->callbacks)) in the blk-stat rework
> exposed it. The fix is unfortunately a hack until we fix all of the
> drivers sharing a request_queue.
FYI, I went through the remaining cases and fixed most of them up,
working on the remaining few. Maybe we can finally put this crap to
rest...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: fix leak of q->rq_wb
2017-03-27 17:43 ` [PATCH 2/2] block: fix leak of q->rq_wb Omar Sandoval
2017-03-28 2:52 ` Omar Sandoval
@ 2017-03-28 3:43 ` Ming Lei
2017-03-28 3:45 ` Omar Sandoval
1 sibling, 1 reply; 6+ messages in thread
From: Ming Lei @ 2017-03-28 3:43 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-block, kernel-team
On Mon, Mar 27, 2017 at 10:43:59AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> CONFIG_DEBUG_TEST_DRIVER_REMOVE found a possible leak of q->rq_wb in a
> couple of cases: when a request queue is reregistered and when gendisks
> share a request_queue. This has been a problem since wbt was introduced,
> but the WARN_ON(!list_empty(&stats->callbacks)) in the blk-stat rework
> exposed it. The fix is unfortunately a hack until we fix all of the
> drivers sharing a request_queue.
>
> Fixes: 87760e5eef35 ("block: hook up writeback throttling")
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> block/blk-sysfs.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index fa831cb2fc30..a187e3f70028 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -893,7 +893,21 @@ int blk_register_queue(struct gendisk *disk)
>
> kobject_uevent(&q->kobj, KOBJ_ADD);
>
> - blk_wb_init(q);
> + /*
> + * There are two cases where wbt may have already been initialized:
> + * 1. A call sequence of blk_register_queue(); blk_unregister_queue();
> + * blk_register_queue().
> + * 2. Multiple gendisks sharing a request_queue.
> + *
> + * To fix case 1, we'd like to call wbt_exit() in
> + * blk_unregister_queue(). However, that's unsafe for case 2. So, we're
> + * forced to do this and call wbt_exit() in blk_release_queue() instead.
> + *
> + * Note that in case 2, wbt will account across disks until those legacy
> + * drivers are fixed.
> + */
> + if (!q->rq_wb)
> + blk_wb_init(q);
Since 'rq_wb' is per-queue and its life time is same with queue's, I
am wondering why blk_wb_init() isn't put into blk_alloc_queue_node() or
queue's initialization api(blk_init_allocated_queue(), or
blk_mq_init_allocated_queue())?
Thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] block: fix leak of q->rq_wb
2017-03-28 3:43 ` Ming Lei
@ 2017-03-28 3:45 ` Omar Sandoval
0 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2017-03-28 3:45 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, kernel-team
On Tue, Mar 28, 2017 at 11:43:04AM +0800, Ming Lei wrote:
> On Mon, Mar 27, 2017 at 10:43:59AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > CONFIG_DEBUG_TEST_DRIVER_REMOVE found a possible leak of q->rq_wb in a
> > couple of cases: when a request queue is reregistered and when gendisks
> > share a request_queue. This has been a problem since wbt was introduced,
> > but the WARN_ON(!list_empty(&stats->callbacks)) in the blk-stat rework
> > exposed it. The fix is unfortunately a hack until we fix all of the
> > drivers sharing a request_queue.
> >
> > Fixes: 87760e5eef35 ("block: hook up writeback throttling")
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> > block/blk-sysfs.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index fa831cb2fc30..a187e3f70028 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -893,7 +893,21 @@ int blk_register_queue(struct gendisk *disk)
> >
> > kobject_uevent(&q->kobj, KOBJ_ADD);
> >
> > - blk_wb_init(q);
> > + /*
> > + * There are two cases where wbt may have already been initialized:
> > + * 1. A call sequence of blk_register_queue(); blk_unregister_queue();
> > + * blk_register_queue().
> > + * 2. Multiple gendisks sharing a request_queue.
> > + *
> > + * To fix case 1, we'd like to call wbt_exit() in
> > + * blk_unregister_queue(). However, that's unsafe for case 2. So, we're
> > + * forced to do this and call wbt_exit() in blk_release_queue() instead.
> > + *
> > + * Note that in case 2, wbt will account across disks until those legacy
> > + * drivers are fixed.
> > + */
> > + if (!q->rq_wb)
> > + blk_wb_init(q);
>
> Since 'rq_wb' is per-queue and its life time is same with queue's, I
> am wondering why blk_wb_init() isn't put into blk_alloc_queue_node() or
> queue's initialization api(blk_init_allocated_queue(), or
> blk_mq_init_allocated_queue())?
Doing it at queue init time might be cleaner, I'll try that.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] blk-mq: fix leak of q->stats
2017-03-27 17:43 [PATCH 1/2] blk-mq: fix leak of q->stats Omar Sandoval
2017-03-27 17:43 ` [PATCH 2/2] block: fix leak of q->rq_wb Omar Sandoval
@ 2017-03-28 2:43 ` Ming Lei
1 sibling, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-03-28 2:43 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-block, kernel-team
On Mon, Mar 27, 2017 at 10:43:58AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> blk_alloc_queue_node() already allocates q->stats, so
> blk_mq_init_allocated_queue() is overwriting it with a new allocation.
>
> Fixes: a83b576c9c25 ("block: fix stacked driver stats init and free")
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> block/blk-mq.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index c212b9644a9f..9c6b39bd8358 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2212,10 +2212,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
> /* mark the queue as mq asap */
> q->mq_ops = set->ops;
>
> - q->stats = blk_alloc_queue_stats();
> - if (!q->stats)
> - goto err_exit;
> -
> q->poll_cb = blk_stat_alloc_callback(blk_mq_poll_stats_fn,
> blk_stat_rq_ddir, 2, q);
> if (!q->poll_cb)
> --
> 2.12.1
>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
--
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-28 3:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-27 17:43 [PATCH 1/2] blk-mq: fix leak of q->stats Omar Sandoval
2017-03-27 17:43 ` [PATCH 2/2] block: fix leak of q->rq_wb Omar Sandoval
2017-03-28 2:52 ` Omar Sandoval
2017-03-28 3:43 ` Ming Lei
2017-03-28 3:45 ` Omar Sandoval
2017-03-28 2:43 ` [PATCH 1/2] blk-mq: fix leak of q->stats Ming Lei
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 NNTP newsgroup(s).