[PATCH 4/5] nvme: paring quiesce/unquiesce

[Ming Lei]

On Wed, Sep 29, 2021 at 02:49:39PM +0300, Sagi Grimberg wrote:
> 
> 
> On 9/29/21 7:15 AM, Ming Lei wrote:
> > The current blk_mq_quiesce_queue() and blk_mq_unquiesce_queue() always
> > stops and starts the queue unconditionally. And there can be concurrent
> > quiesce/unquiesce coming from different unrelated code paths, so
> > unquiesce may come unexpectedly and start queue too early.
> > 
> > Prepare for supporting nested / concurrent quiesce/unquiesce, so that we
> > can address the above issue.
> > 
> > NVMe has very complicated quiesce/unquiesce use pattern, add one mutex
> > and queue stopped state in nvme_ctrl, so that we can make sure that
> > quiece/unquiesce is called in pair.
> > 
> > Signed-off-by: Ming Lei <ming.lei at redhat.com>
> > ---
> >   drivers/nvme/host/core.c | 51 ++++++++++++++++++++++++++++++++++++----
> >   drivers/nvme/host/nvme.h |  4 ++++
> >   2 files changed, 50 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 23fb746a8970..5d0b2eb38e43 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -4375,6 +4375,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> >   	clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
> >   	spin_lock_init(&ctrl->lock);
> >   	mutex_init(&ctrl->scan_lock);
> > +	mutex_init(&ctrl->queues_stop_lock);
> >   	INIT_LIST_HEAD(&ctrl->namespaces);
> >   	xa_init(&ctrl->cels);
> >   	init_rwsem(&ctrl->namespaces_rwsem);
> > @@ -4450,14 +4451,44 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> >   }
> >   EXPORT_SYMBOL_GPL(nvme_init_ctrl);
> > +static void __nvme_stop_admin_queue(struct nvme_ctrl *ctrl)
> > +{
> > +	lockdep_assert_held(&ctrl->queues_stop_lock);
> > +
> > +	if (!ctrl->admin_queue_stopped) {
> > +		blk_mq_quiesce_queue(ctrl->admin_q);
> > +		ctrl->admin_queue_stopped = true;
> > +	}
> > +}
> > +
> > +static void __nvme_start_admin_queue(struct nvme_ctrl *ctrl)
> > +{
> > +	lockdep_assert_held(&ctrl->queues_stop_lock);
> > +
> > +	if (ctrl->admin_queue_stopped) {
> > +		blk_mq_unquiesce_queue(ctrl->admin_q);
> > +		ctrl->admin_queue_stopped = false;
> > +	}
> > +}
> 
> I'd make this a bit we can flip atomically.
> 
> > +
> >   static void nvme_start_ns_queue(struct nvme_ns *ns)
> >   {
> > -	blk_mq_unquiesce_queue(ns->queue);
> > +	lockdep_assert_held(&ns->ctrl->queues_stop_lock);
> > +
> > +	if (test_bit(NVME_NS_STOPPED, &ns->flags)) {
> > +		blk_mq_unquiesce_queue(ns->queue);
> > +		clear_bit(NVME_NS_STOPPED, &ns->flags);
> > +	}
> >   }
> >   static void nvme_stop_ns_queue(struct nvme_ns *ns)
> >   {
> > -	blk_mq_quiesce_queue(ns->queue);
> > +	lockdep_assert_held(&ns->ctrl->queues_stop_lock);
> > +
> > +	if (!test_bit(NVME_NS_STOPPED, &ns->flags)) {
> > +		blk_mq_quiesce_queue(ns->queue);
> > +		set_bit(NVME_NS_STOPPED, &ns->flags);
> > +	}
> >   }
> 
> Why not use test_and_set_bit/test_and_clear_bit for serialization?
> 
> >   /*
> > @@ -4490,16 +4521,18 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
> >   {
> >   	struct nvme_ns *ns;
> > +	mutex_lock(&ctrl->queues_stop_lock);
> >   	down_read(&ctrl->namespaces_rwsem);
> >   	/* Forcibly unquiesce queues to avoid blocking dispatch */
> >   	if (ctrl->admin_q && !blk_queue_dying(ctrl->admin_q))
> > -		nvme_start_admin_queue(ctrl);
> > +		__nvme_start_admin_queue(ctrl);
> >   	list_for_each_entry(ns, &ctrl->namespaces, list)
> >   		nvme_set_queue_dying(ns);
> >   	up_read(&ctrl->namespaces_rwsem);
> > +	mutex_unlock(&ctrl->queues_stop_lock);
> 
> This extra lock wrapping the namespaces_rwsem is scary. The
> ordering rules are not clear to me.

The rule is clear: queues_stop_lock has to be acquired before locking
->namespaces_rwsem.

Using test_and_set_bit/test_and_clear_bit could be enough for pairing
quiesce and unquiesce, I will try to remove the lock of
->queues_stop_lock in next version.


Thanks,
Ming