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 Thu, May 06, 2021 at 07:51:53AM -0700, Bart Van Assche wrote:
> On 5/6/21 5:18 AM, Christoph Hellwig wrote:
> > On Thu, May 06, 2021 at 03:34:17PM +0800, Ming Lei wrote:
> >>> {
> >>> 	struct blk_mq_tags *drv_tags = set->tags[hctx_idx];
> >>> 	unsigned int i = 0;
> >>> 	unsigned long flags;
> >>>
> >>> 	spin_lock_irqsave(&drv_tags->lock, flags);
> >>> 	for (i = 0; i < set->queue_depth; i++)
> >>> 		WARN_ON_ONCE(refcount_read(&drv_tags->rqs[i]->ref) != 0);
> >>> 		drv_tags->rqs[i] = NULL;
> >>
> >> drv_tags->rqs[] may be assigned from another LUN at the same time, then
> >> the simple clearing will overwrite the active assignment. We just need
> >> to clear mapping iff the stored rq will be freed.
> > 
> > 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.
> > 
> > Btw, I think ->lock might better be named ->clear_lock to document its
> > purpose better.
> 
> I'm not sure that would be a better name since I don't think that it is
> necessary to hold that lock around the cmpxchg() calls. How about using
> something like the following code in blk_mq_clear_rq_mapping() instead
> of the code in v5 of patch 3/4?
> 
> 	spin_lock_irqsave(&drv_tags->lock, flags);
> 	spin_unlock_irqrestore(&drv_tags->lock, flags);
> 
> 	list_for_each_entry(page, &tags->page_list, lru) {
> 		/* use cmpxchg() to clear request pointer selectively */
> 	}

This way won't work as expected because iterating may happen between
releasing drv_tags->lock and cmpxchg(->rqs[]), then freed requests
may still be touched during iteration after they are freed in blk_mq_free_rqs().


Thanks,
Ming