On Thu, May 06, 2021 at 01:18:49PM +0100, 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.
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[].
>
> Btw, I think ->lock might better be named ->clear_lock to document its
> purpose better.
OK.
thanks,
Ming
