diff mbox

[1/7] blk-mq: fix use of incorrect goto label in blk_mq_init_queue error path

Message ID 1426132602-34331-2-git-send-email-snitzer@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Snitzer March 12, 2015, 3:56 a.m. UTC
If percpu_ref_init() fails the 'err_hctxs' label should be used instead
of 'err_map'.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ming Lei March 12, 2015, 7:48 a.m. UTC | #1
On Thu, Mar 12, 2015 at 11:56 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> If percpu_ref_init() fails the 'err_hctxs' label should be used instead
> of 'err_map'.
>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4f4bea2..459840c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1938,7 +1938,7 @@ struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
>          */
>         if (percpu_ref_init(&q->mq_usage_counter, blk_mq_usage_counter_release,
>                             PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
> -               goto err_map;
> +               goto err_hctxs;

If it is changed to 'goto err_hctxs', percpu_ref_init() need to
move before blk_alloc_queue_node(), otherwise just 'goto err_hw'
is enough, but the former is better.

Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Snitzer March 12, 2015, 1:51 p.m. UTC | #2
On Thu, Mar 12 2015 at  3:48am -0400,
Ming Lei <tom.leiming@gmail.com> wrote:

> On Thu, Mar 12, 2015 at 11:56 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> > If percpu_ref_init() fails the 'err_hctxs' label should be used instead
> > of 'err_map'.
> >
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > ---
> >  block/blk-mq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 4f4bea2..459840c 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -1938,7 +1938,7 @@ struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
> >          */
> >         if (percpu_ref_init(&q->mq_usage_counter, blk_mq_usage_counter_release,
> >                             PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
> > -               goto err_map;
> > +               goto err_hctxs;
> 
> If it is changed to 'goto err_hctxs', percpu_ref_init() need to
> move before blk_alloc_queue_node(), otherwise just 'goto err_hw'
> is enough, but the former is better.

Yes, you're correct.  Patch 2 happened to eliminate this problem.  But
I'll get it fixed up and post v2.

The reason this thinko happened is I noticed the problem after I moved
the blk_alloc_queue_node() out in patch 2 and when I rebased to reorder
the fix before that patch 2 context got lost in the shuffle.

Thanks for your review!
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Snitzer March 13, 2015, 3:29 a.m. UTC | #3
On Thu, Mar 12 2015 at  9:51am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Mar 12 2015 at  3:48am -0400,
> Ming Lei <tom.leiming@gmail.com> wrote:
> 
> > On Thu, Mar 12, 2015 at 11:56 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> > > If percpu_ref_init() fails the 'err_hctxs' label should be used instead
> > > of 'err_map'.
> > >
> > > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > > ---
> > >  block/blk-mq.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > index 4f4bea2..459840c 100644
> > > --- a/block/blk-mq.c
> > > +++ b/block/blk-mq.c
> > > @@ -1938,7 +1938,7 @@ struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
> > >          */
> > >         if (percpu_ref_init(&q->mq_usage_counter, blk_mq_usage_counter_release,
> > >                             PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
> > > -               goto err_map;
> > > +               goto err_hctxs;
> > 
> > If it is changed to 'goto err_hctxs', percpu_ref_init() need to
> > move before blk_alloc_queue_node(), otherwise just 'goto err_hw'
> > is enough, but the former is better.
> 
> Yes, you're correct.  Patch 2 happened to eliminate this problem.  But
> I'll get it fixed up and post v2.

Actually, percpu_ref_init clearly cannot go before blk_alloc_queue_node
since percpu_ref_init deferences q.  (buildbot just emailed complaining
about crashes with my v2 patch).  I didn't test this 1/7 patch in
isolation but should have -- when 2/7 is applied the problem goes away.

v3 of these first 2 patches will be inbound shortly.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4f4bea2..459840c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1938,7 +1938,7 @@  struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
 	 */
 	if (percpu_ref_init(&q->mq_usage_counter, blk_mq_usage_counter_release,
 			    PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
-		goto err_map;
+		goto err_hctxs;
 
 	setup_timer(&q->timeout, blk_mq_rq_timer, (unsigned long) q);
 	blk_queue_rq_timeout(q, 30000);