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 */
}
Thanks,
Bart.
