Re: [PATCH 7/8] nvme: use blk_mq_queue_tag_busy_iter

[Hannes Reinecke]

On 3/15/19 5:39 PM, James Smart wrote:
> On 3/15/2019 9:33 AM, James Smart wrote:
> > On 3/15/2019 1:57 AM, Jianchao Wang wrote:
> > > blk_mq_tagset_busy_iter is not safe that it could get stale request
> > > in tags->rqs[]. Use blk_mq_queue_tag_busy_iter here. A new helper
> > > interface nvme_iterate_inflight_rqs is introduced to iterate
> > > all of the ns under a ctrl.
> > >
> > > Signed-off-by: Jianchao Wang <jianchao.w.wang@xxxxxxxxxx>
> > > ---
> > > Â drivers/nvme/host/core.cÂÂ | 12 ++++++++++++
> > > Â drivers/nvme/host/fc.cÂÂÂÂ | 12 ++++++------
> > > Â drivers/nvme/host/nvme.hÂÂ |Â 2 ++
> > > Â drivers/nvme/host/pci.cÂÂÂ |Â 5 +++--
> > > Â drivers/nvme/host/rdma.cÂÂ |Â 6 +++---
> > > Â drivers/nvme/host/tcp.cÂÂÂ |Â 5 +++--
> > > Â drivers/nvme/target/loop.c |Â 6 +++---
> > > Â 7 files changed, 32 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index 6a9dd68..5760fad 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -3816,6 +3816,18 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
> > > Â }
> > > Â EXPORT_SYMBOL_GPL(nvme_start_queues);
> > > Â +void nvme_iterate_inflight_rqs(struct nvme_ctrl *ctrl,
> > > +ÂÂÂÂÂÂÂ busy_iter_fn *fn, void *data)
> > > +{
> > > +ÂÂÂ struct nvme_ns *ns;
> > > +
> > > +ÂÂÂ down_read(&ctrl->namespaces_rwsem);
> > > +ÂÂÂ list_for_each_entry(ns, &ctrl->namespaces, list)
> > > +ÂÂÂÂÂÂÂ blk_mq_queue_tag_busy_iter(ns->queue, fn, data, true);
> > > +ÂÂÂ up_read(&ctrl->namespaces_rwsem);
> > > +}
> > > +EXPORT_SYMBOL_GPL(nvme_iterate_inflight_rqs);
> >
> > Hmm... so this aborts ios for namespaces. How about ios that aren't 
> > for a namespace but rather for the controller itself. For example, 
> > anything on the admin queue ?ÂÂ Rather critical for those ios be 
> > terminated as well.
> NVM - didn't look at which tag set. but it does highlight - doesn't 
> cover anything issued by core/transport on the io queues.
Actually, I would consider that an error.
_All_ commands on io queues (internal or block-layer generated) whould 
be tracked sbitmap and hence the tag iterator.
That's what we have the 'private' tags for, and why the iter callback 
has this ominous 'bool' argument...

Cheers,

Hannes