Re: [PATCH V5 3/4] blk-mq: clear stale request in tags->rq[] before freeing one request pool

[Ming Lei <ming.lei@redhat.com>]

On Fri, May 07, 2021 at 07:57:23AM +0100, Christoph Hellwig wrote:
> On Thu, May 06, 2021 at 09:38:41PM +0800, Ming Lei wrote:
> > > So.  Even a different LUN shares the same tagset.  So I can see the
> > > need for the cmpxchg (please document it!), but I don't see the need
> > > for the complex iteration.  All the rqs are freed in one single loop,
> > > so we can just iterate through them sequentially.
> > 
> > That is exactly what the patch is doing, requests are stored in page
> > list, so check if one request(covered in page list) reference in
> > drv_tags->rq[i] exists, if yes, we clear the request reference.
> > 
> > The code is actually sort of self-document: before we free requests,
> > clear the reference in hostwide drv->rqs[].
> 
> What the patch does it to do a completely pointless nested loop.
> Instead of just looping through all requests which is simple and fast
> it loops through each page, and then does another loop inside that,
> just increasing complexity and runtime.  We should at least do something
> like the incremental patch below instead which is simpler, faster and
> easier to understand:

The pages to be freed may be from scheduler tags(set->sched_tags), which
belongs to one request queue being shutdown, but set->tags->rqs[] is
shared by all request queues in the host, and it can be actively assigned
from other LUN/request queue.

> 
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index c1b28e09a27e..598fe82cfbcf 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2311,29 +2311,20 @@ static size_t order_to_size(unsigned int order)
>  	return (size_t)PAGE_SIZE << order;
>  }
>  
> -/* called before freeing request pool in @tags */
> +/* ensure that blk_mq_find_and_get_req can't find the tags any more */
>  static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set,
>  		struct blk_mq_tags *tags, unsigned int hctx_idx)
>  {
>  	struct blk_mq_tags *drv_tags = set->tags[hctx_idx];
> -	struct page *page;
>  	unsigned long flags;
> +	int i;
>  
>  	spin_lock_irqsave(&drv_tags->lock, flags);
> -	list_for_each_entry(page, &tags->page_list, lru) {
> -		unsigned long start = (unsigned long)page_address(page);
> -		unsigned long end = start + order_to_size(page->private);
> -		int i;
> +	for (i = 0; i < set->queue_depth; i++) {
> +		struct request *rq = drv_tags->rqs[i];
>  
> -		for (i = 0; i < set->queue_depth; i++) {
> -			struct request *rq = drv_tags->rqs[i];
> -			unsigned long rq_addr = (unsigned long)rq;
> -
> -			if (rq_addr >= start && rq_addr < end) {
> -				WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
> -				cmpxchg(&drv_tags->rqs[i], rq, NULL);
> -			}
> -		}
> +		WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
> +		cmpxchg(&drv_tags->rqs[i], rq, NULL);

set->tags->rqs[] is just one dynamic mapping between host-wide driver tag and
request which may be allocated from sched tags which is per-request-queue,
and set->tags->rqs[] is host wide.

What if the request pointed by 'rq' is just assigned from another active LUN's
sched tags?

What we need to do is to make sure every reference to being freed request is
cleared, that is all.

Thanks,
Ming