Re: [PATCH v3 4/7] block: free both map and request

[weiping zhang <zhangweiping@xxxxxxxxxxxxxxx>]

On Mon, Apr 20, 2020 at 01:58:54PM -0700, Bart Van Assche wrote:
> On 4/6/20 12:36 PM, Weiping Zhang wrote:
> >For this error handle, it should free both map and request,
> >otherwise memleak occur.
>             ^^^^^^^
> Please expand this into "a memory leak".
> 
> >diff --git a/block/blk-mq.c b/block/blk-mq.c
> >index 4692e8232699..406df9ce9b55 100644
> >--- a/block/blk-mq.c
> >+++ b/block/blk-mq.c
> >@@ -2990,7 +2990,7 @@ static int __blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set)
> >  out_unwind:
> >  	while (--i >= 0)
> >-		blk_mq_free_rq_map(set->tags[i]);
> >+		blk_mq_free_map_and_requests(set, i);
> >  	return -ENOMEM;
> >  }
> 
> The current upstream implementation is as follows:
> 
> 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))
> 			goto out_unwind;
> 
> 	return 0;
> 
> out_unwind:
> 	while (--i >= 0)
> 		blk_mq_free_rq_map(set->tags[i]);
> 
> 	return -ENOMEM;
> }
> 
> The pointers to the memory allocated by __blk_mq_alloc_rq_map() are
> stored in set->tags[i] and set->tags[i]->static_rqs.
> blk_mq_free_rq_map() frees set->tags[i]->static_rqs and
> set->tags[i]. In other words, I don't see which memory is leaked.
> What did I miss?

Hi Bart,

__blk_mq_alloc_rq_map
	blk_mq_alloc_rq_map
		blk_mq_alloc_rq_map
			tags = blk_mq_init_tags : kzalloc_node:
			tags->rqs = kcalloc_node
			tags->static_rqs = kcalloc_node
	blk_mq_alloc_rqs
		p = alloc_pages_node
		tags->static_rqs[i] = p + offset;

but

blk_mq_free_rq_map
	kfree(tags->rqs);
	kfree(tags->static_rqs);
	blk_mq_free_tags
		kfree(tags);

The page allocated in blk_mq_alloc_rqs cannot be released,
so we should use blk_mq_free_map_and_requests here,

blk_mq_free_map_and_requests
	blk_mq_free_rqs
		__free_pages : cleanup for blk_mq_alloc_rqs
	blk_mq_free_rq_map : cleanup for blk_mq_alloc_rq_map
		...

The reason why I rename some functions in previous three patches is
that, these function not only allocate rq_map but also requests.

How about rename them like this:
__blk_mq_alloc_rq_map	=> __blk_mq_alloc_map_and_request
__blk_mq_alloc_rq_maps	=> __blk_mq_alloc_map_and_requests,
blk_mq_alloc_rq_maps	=> blk_mq_alloc_map_and_requests

Thanks

> 
> Thanks,
> 
> Bart.