[v3,1/7] block: rename __blk_mq_alloc_rq_map
diff mbox series

Message ID 9e542bceca1c467c99f114be7ab958066b8c7bf5.1586199103.git.zhangweiping@didiglobal.com
State New
Headers show
Series
  • Fix potential kernel panic when increase hardware queue
Related show

Commit Message

Weiping Zhang April 6, 2020, 7:36 p.m. UTC
rename __blk_mq_alloc_rq_map to __blk_mq_alloc_rq_map_and_request,
actually it alloc both map and request, make function name
align with function.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 block/blk-mq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Bart Van Assche April 20, 2020, 8:41 p.m. UTC | #1
On 4/6/20 12:36 PM, Weiping Zhang wrote:
> rename __blk_mq_alloc_rq_map to __blk_mq_alloc_rq_map_and_request,
> actually it alloc both map and request, make function name
> align with function.
> 
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
>   block/blk-mq.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index f6291ceedee4..3a482ce7ed28 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2468,7 +2468,7 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
>   	}
>   }
>   
> -static bool __blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, int hctx_idx)
> +static bool __blk_mq_alloc_rq_map_and_request(struct blk_mq_tag_set *set, int hctx_idx)
>   {
>   	int ret = 0;
>   
> @@ -2519,7 +2519,7 @@ static void blk_mq_map_swqueue(struct request_queue *q)
>   		hctx_idx = set->map[HCTX_TYPE_DEFAULT].mq_map[i];
>   		/* unmapped hw queue can be remapped after CPU topo changed */
>   		if (!set->tags[hctx_idx] &&
> -		    !__blk_mq_alloc_rq_map(set, hctx_idx)) {
> +		    !__blk_mq_alloc_rq_map_and_request(set, hctx_idx)) {
>   			/*
>   			 * If tags initialization fail for some hctx,
>   			 * that hctx won't be brought online.  In this
> @@ -2983,7 +2983,7 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
>   	int i;
>   
>   	for (i = 0; i < set->nr_hw_queues; i++)
> -		if (!__blk_mq_alloc_rq_map(set, i))
> +		if (!__blk_mq_alloc_rq_map_and_request(set, i))
>   			goto out_unwind;
>   
>   	return 0;

What the __blk_mq_alloc_rq_map() function allocates is a request map and 
requests. The new name is misleading because it suggests that only a 
single request is allocated instead of multiple. The name 
__blk_mq_alloc_rq_map_and_requests() is probably a better choice than 
__blk_mq_alloc_rq_map_and_request().

My opinion is that the old name is clear enough. I prefer the current name.

Thanks,

Bart.
Ming Lei April 21, 2020, 1:25 a.m. UTC | #2
On Tue, Apr 21, 2020 at 4:42 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 4/6/20 12:36 PM, Weiping Zhang wrote:
> > rename __blk_mq_alloc_rq_map to __blk_mq_alloc_rq_map_and_request,
> > actually it alloc both map and request, make function name
> > align with function.
> >
> > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > ---
> >   block/blk-mq.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index f6291ceedee4..3a482ce7ed28 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2468,7 +2468,7 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
> >       }
> >   }
> >
> > -static bool __blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, int hctx_idx)
> > +static bool __blk_mq_alloc_rq_map_and_request(struct blk_mq_tag_set *set, int hctx_idx)
> >   {
> >       int ret = 0;
> >
> > @@ -2519,7 +2519,7 @@ static void blk_mq_map_swqueue(struct request_queue *q)
> >               hctx_idx = set->map[HCTX_TYPE_DEFAULT].mq_map[i];
> >               /* unmapped hw queue can be remapped after CPU topo changed */
> >               if (!set->tags[hctx_idx] &&
> > -                 !__blk_mq_alloc_rq_map(set, hctx_idx)) {
> > +                 !__blk_mq_alloc_rq_map_and_request(set, hctx_idx)) {
> >                       /*
> >                        * If tags initialization fail for some hctx,
> >                        * that hctx won't be brought online.  In this
> > @@ -2983,7 +2983,7 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
> >       int i;
> >
> >       for (i = 0; i < set->nr_hw_queues; i++)
> > -             if (!__blk_mq_alloc_rq_map(set, i))
> > +             if (!__blk_mq_alloc_rq_map_and_request(set, i))
> >                       goto out_unwind;
> >
> >       return 0;
>
> What the __blk_mq_alloc_rq_map() function allocates is a request map and
> requests. The new name is misleading because it suggests that only a
> single request is allocated instead of multiple. The name
> __blk_mq_alloc_rq_map_and_requests() is probably a better choice than
> __blk_mq_alloc_rq_map_and_request().
>
> My opinion is that the old name is clear enough. I prefer the current name.

Also putting renaming patches before actual fix patches does make more trouble
for backporting the fix to stable tree.

So please re-organize patches by fixing issues first, then following rename
stuff.

Thanks,
Ming Lei
weiping zhang April 21, 2020, 1:17 p.m. UTC | #3
On Tue, Apr 21, 2020 at 09:25:49AM +0800, Ming Lei wrote:
> On Tue, Apr 21, 2020 at 4:42 AM Bart Van Assche <bvanassche@acm.org> wrote:
> >
> > On 4/6/20 12:36 PM, Weiping Zhang wrote:
> > > rename __blk_mq_alloc_rq_map to __blk_mq_alloc_rq_map_and_request,
> > > actually it alloc both map and request, make function name
> > > align with function.
> > >
> > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > ---
> > >   block/blk-mq.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > index f6291ceedee4..3a482ce7ed28 100644
> > > --- a/block/blk-mq.c
> > > +++ b/block/blk-mq.c
> > > @@ -2468,7 +2468,7 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
> > >       }
> > >   }
> > >
> > > -static bool __blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, int hctx_idx)
> > > +static bool __blk_mq_alloc_rq_map_and_request(struct blk_mq_tag_set *set, int hctx_idx)
> > >   {
> > >       int ret = 0;
> > >
> > > @@ -2519,7 +2519,7 @@ static void blk_mq_map_swqueue(struct request_queue *q)
> > >               hctx_idx = set->map[HCTX_TYPE_DEFAULT].mq_map[i];
> > >               /* unmapped hw queue can be remapped after CPU topo changed */
> > >               if (!set->tags[hctx_idx] &&
> > > -                 !__blk_mq_alloc_rq_map(set, hctx_idx)) {
> > > +                 !__blk_mq_alloc_rq_map_and_request(set, hctx_idx)) {
> > >                       /*
> > >                        * If tags initialization fail for some hctx,
> > >                        * that hctx won't be brought online.  In this
> > > @@ -2983,7 +2983,7 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
> > >       int i;
> > >
> > >       for (i = 0; i < set->nr_hw_queues; i++)
> > > -             if (!__blk_mq_alloc_rq_map(set, i))
> > > +             if (!__blk_mq_alloc_rq_map_and_request(set, i))
> > >                       goto out_unwind;
> > >
> > >       return 0;
> >
> > What the __blk_mq_alloc_rq_map() function allocates is a request map and
> > requests. The new name is misleading because it suggests that only a
> > single request is allocated instead of multiple. The name
> > __blk_mq_alloc_rq_map_and_requests() is probably a better choice than
> > __blk_mq_alloc_rq_map_and_request().
> >
> > My opinion is that the old name is clear enough. I prefer the current name.
> 
> Also putting renaming patches before actual fix patches does make more trouble
> for backporting the fix to stable tree.
> 
> So please re-organize patches by fixing issues first, then following rename
> stuff.
OK, I reorder them.

Thanks
> 
> Thanks,
> Ming Lei

Patch
diff mbox series

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f6291ceedee4..3a482ce7ed28 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2468,7 +2468,7 @@  static void blk_mq_init_cpu_queues(struct request_queue *q,
 	}
 }
 
-static bool __blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, int hctx_idx)
+static bool __blk_mq_alloc_rq_map_and_request(struct blk_mq_tag_set *set, int hctx_idx)
 {
 	int ret = 0;
 
@@ -2519,7 +2519,7 @@  static void blk_mq_map_swqueue(struct request_queue *q)
 		hctx_idx = set->map[HCTX_TYPE_DEFAULT].mq_map[i];
 		/* unmapped hw queue can be remapped after CPU topo changed */
 		if (!set->tags[hctx_idx] &&
-		    !__blk_mq_alloc_rq_map(set, hctx_idx)) {
+		    !__blk_mq_alloc_rq_map_and_request(set, hctx_idx)) {
 			/*
 			 * If tags initialization fail for some hctx,
 			 * that hctx won't be brought online.  In this
@@ -2983,7 +2983,7 @@  static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
 	int i;
 
 	for (i = 0; i < set->nr_hw_queues; i++)
-		if (!__blk_mq_alloc_rq_map(set, i))
+		if (!__blk_mq_alloc_rq_map_and_request(set, i))
 			goto out_unwind;
 
 	return 0;