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
