[PATCH] NVMe: Fix possible scheduling while atomic error

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

Stopping a h/w queue could have scheduled the task to wait on cancelling
work. This patch removes stopping the the delayed work so we can safely
hold rcu_read_lock() while stopping h/w queues.

Since blk-mq requeue work may restart the h/w queues during a reset,
the nvme driver's queue_rq will stop them again if the driver receives
a command during the reset window.

Reported-by: Ming Lin <mlin at kernel.org>
[fixes 0bf77e9 nvme: switch to RCU freeing the namespace]
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 6 ++++--
 drivers/nvme/host/pci.c  | 6 ++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a51584..1e05abf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1823,7 +1823,6 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 		queue_flag_set(QUEUE_FLAG_STOPPED, ns->queue);
 		spin_unlock_irq(ns->queue->queue_lock);
 
-		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
 	}
 	rcu_read_unlock();
@@ -1836,7 +1835,10 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
-		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
+		spin_lock_irq(ns->queue->queue_lock);
+		queue_flag_clear(QUEUE_FLAG_STOPPED, ns->queue);
+		spin_unlock_irq(ns->queue->queue_lock);
+
 		blk_mq_start_stopped_hw_queues(ns->queue, true);
 		blk_mq_kick_requeue_list(ns->queue);
 	}
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8356813..9693629 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	spin_unlock_irq(&nvmeq->q_lock);
 	return BLK_MQ_RQ_QUEUE_OK;
 out:
+	if (ret == BLK_MQ_RQ_QUEUE_BUSY) {
+		spin_lock_irq(ns->queue->queue_lock);
+		if (blk_queue_stopped(req->q))
+			blk_mq_stop_hw_queues(ns->queue);
+		spin_unlock_irq(ns->queue->queue_lock);
+	}
 	nvme_free_iod(dev, req);
 	return ret;
 }
-- 
2.7.2

[PATCH] NVMe: Fix possible scheduling while atomic error

[Christoph Hellwig]

On Tue, May 17, 2016@03:37:42PM -0600, Keith Busch wrote:
>  		spin_unlock_irq(ns->queue->queue_lock);
>  
> -		blk_mq_cancel_requeue_work(ns->queue);
>  		blk_mq_stop_hw_queues(ns->queue);
>  	}
>  	rcu_read_unlock();
> @@ -1836,7 +1835,10 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
> -		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
> +		spin_lock_irq(ns->queue->queue_lock);
> +		queue_flag_clear(QUEUE_FLAG_STOPPED, ns->queue);
> +		spin_unlock_irq(ns->queue->queue_lock);

What's the rationale for this change? 

> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	spin_unlock_irq(&nvmeq->q_lock);
>  	return BLK_MQ_RQ_QUEUE_OK;
>  out:
> +	if (ret == BLK_MQ_RQ_QUEUE_BUSY) {
> +		spin_lock_irq(ns->queue->queue_lock);
> +		if (blk_queue_stopped(req->q))
> +			blk_mq_stop_hw_queues(ns->queue);
> +		spin_unlock_irq(ns->queue->queue_lock);

Shouldn't we do this where set the stopped flag on the queue instead?

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

On Mon, May 23, 2016@03:58:07AM -0700, Christoph Hellwig wrote:
> On Tue, May 17, 2016@03:37:42PM -0600, Keith Busch wrote:
> >  	rcu_read_lock();
> >  	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
> > -		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
> > +		spin_lock_irq(ns->queue->queue_lock);
> > +		queue_flag_clear(QUEUE_FLAG_STOPPED, ns->queue);
> > +		spin_unlock_irq(ns->queue->queue_lock);
> 
> What's the rationale for this change? 

That was to make it so nvme_queue_rq wouldn't see a stopped flag just
before it was starting.
 
> > @@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> >  	spin_unlock_irq(&nvmeq->q_lock);
> >  	return BLK_MQ_RQ_QUEUE_OK;
> >  out:
> > +	if (ret == BLK_MQ_RQ_QUEUE_BUSY) {
> > +		spin_lock_irq(ns->queue->queue_lock);
> > +		if (blk_queue_stopped(req->q))
> > +			blk_mq_stop_hw_queues(ns->queue);
> > +		spin_unlock_irq(ns->queue->queue_lock);
> 
> Shouldn't we do this where set the stopped flag on the queue instead?

It's moved out of that function holding the rcu read lock since
blk_mq_stop_hw_queues potentially sleeps.

But the above is also broken because it's holding a spinlock, so this
patch is no good.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Christoph Hellwig]

On Mon, May 23, 2016@09:41:16AM -0400, Keith Busch wrote:
> On Mon, May 23, 2016@03:58:07AM -0700, Christoph Hellwig wrote:
> > On Tue, May 17, 2016@03:37:42PM -0600, Keith Busch wrote:
> > >  	rcu_read_lock();
> > >  	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
> > > -		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
> > > +		spin_lock_irq(ns->queue->queue_lock);
> > > +		queue_flag_clear(QUEUE_FLAG_STOPPED, ns->queue);
> > > +		spin_unlock_irq(ns->queue->queue_lock);
> > 
> > What's the rationale for this change? 
> 
> That was to make it so nvme_queue_rq wouldn't see a stopped flag just
> before it was starting.

I don't really see how that helps us with any race..

> > > @@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> > >  	spin_unlock_irq(&nvmeq->q_lock);
> > >  	return BLK_MQ_RQ_QUEUE_OK;
> > >  out:
> > > +	if (ret == BLK_MQ_RQ_QUEUE_BUSY) {
> > > +		spin_lock_irq(ns->queue->queue_lock);
> > > +		if (blk_queue_stopped(req->q))
> > > +			blk_mq_stop_hw_queues(ns->queue);
> > > +		spin_unlock_irq(ns->queue->queue_lock);
> > 
> > Shouldn't we do this where set the stopped flag on the queue instead?
> 
> It's moved out of that function holding the rcu read lock since
> blk_mq_stop_hw_queues potentially sleeps.

blk_mq_stop_hw_queues is a loop around blk_mq_stop_hw_queue,
which itself does two calls to cancel_delayed_work and a set_bit.
None of them can block.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

On Mon, May 23, 2016@07:36:31AM -0700, Christoph Hellwig wrote:
> On Mon, May 23, 2016@09:41:16AM -0400, Keith Busch wrote:
> > On Mon, May 23, 2016@03:58:07AM -0700, Christoph Hellwig wrote:
> > > On Tue, May 17, 2016@03:37:42PM -0600, Keith Busch wrote:
> > > >  	rcu_read_lock();
> > > >  	list_for_each_entry_rcu(ns, &ctrl->namespaces, list) {
> > > > -		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
> > > > +		spin_lock_irq(ns->queue->queue_lock);
> > > > +		queue_flag_clear(QUEUE_FLAG_STOPPED, ns->queue);
> > > > +		spin_unlock_irq(ns->queue->queue_lock);
> > > 
> > > What's the rationale for this change? 
> > 
> > That was to make it so nvme_queue_rq wouldn't see a stopped flag just
> > before it was starting.
> 
> I don't really see how that helps us with any race..

Without the additional spin lock protecting QUEUE_FLAG_STOPPED:

  CPU A                                      CPU B
  -----                                      -----
  nvme_start_queues                          nvme_queue_rq
                                              if (nvmeq->cq_vector < 0) <--  queue is disabled during reset, returning BLK_MQ_RQ_QUEUE_BUSY
                                                 goto out;
                                              if (blk_queue_stopped())  <--  returns true
    queue_flag_clear
    blk_mq_start_stopped_hw_queues
                                                blk_mq_stop_hw_queues() <--  incorrectly stops h/w queues

Locking the queue in nvme_start_queues before clearing the flag fixes
that up.


> > It's moved out of that function holding the rcu read lock since
> > blk_mq_stop_hw_queues potentially sleeps.
> 
> blk_mq_stop_hw_queues is a loop around blk_mq_stop_hw_queue,
> which itself does two calls to cancel_delayed_work and a set_bit.
> None of them can block.

Oops, I'm crossing wires and mixing up what API's do what ... need to
wake up before starting work.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Ming Lin]

On Mon, May 23, 2016@7:55 AM, Keith Busch <keith.busch@intel.com> wrote:
>
>> > It's moved out of that function holding the rcu read lock since
>> > blk_mq_stop_hw_queues potentially sleeps.
>>
>> blk_mq_stop_hw_queues is a loop around blk_mq_stop_hw_queue,
>> which itself does two calls to cancel_delayed_work and a set_bit.
>> None of them can block.

It's blk_mq_cancel_requeue_work() that potentially sleeps.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

On Tue, May 24, 2016@11:43:21AM -0700, Ming Lin wrote:
> 
> It's blk_mq_cancel_requeue_work() that potentially sleeps.

Right, I don't know what I was thinking... 

AFAICT, the patch is fine as-is if Christoph is okay with it.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Christoph Hellwig]

On Tue, May 24, 2016@03:59:21PM -0400, Keith Busch wrote:
> On Tue, May 24, 2016@11:43:21AM -0700, Ming Lin wrote:
> > 
> > It's blk_mq_cancel_requeue_work() that potentially sleeps.
> 
> Right, I don't know what I was thinking... 
> 
> AFAICT, the patch is fine as-is if Christoph is okay with it.

I still don't why we need to delay the call to blk_mq_stop_hw_queues
to ->queue_rq.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

On Wed, May 25, 2016@01:18:42AM -0700, Christoph Hellwig wrote:
> On Tue, May 24, 2016@03:59:21PM -0400, Keith Busch wrote:
> > On Tue, May 24, 2016@11:43:21AM -0700, Ming Lin wrote:
> > > 
> > > It's blk_mq_cancel_requeue_work() that potentially sleeps.
> > 
> > Right, I don't know what I was thinking... 
> > 
> > AFAICT, the patch is fine as-is if Christoph is okay with it.
> 
> I still don't why we need to delay the call to blk_mq_stop_hw_queues
> to ->queue_rq.

Eh? It's still stopped in the same place as always, so it's not
delayed. We just call it again from queue_rq because requeue_work can
start h/w queues on its own. This is very similar to scsi's queue_rq
in scsi_lib.c.

[PATCH] NVMe: Fix possible scheduling while atomic error

[Christoph Hellwig]

On Wed, May 25, 2016@01:57:37PM -0400, Keith Busch wrote:
> > > On Tue, May 24, 2016@11:43:21AM -0700, Ming Lin wrote:
> > > > 
> > > > It's blk_mq_cancel_requeue_work() that potentially sleeps.
> > > 
> > > Right, I don't know what I was thinking... 
> > > 
> > > AFAICT, the patch is fine as-is if Christoph is okay with it.
> > 
> > I still don't why we need to delay the call to blk_mq_stop_hw_queues
> > to ->queue_rq.
> 
> Eh? It's still stopped in the same place as always, so it's not
> delayed. We just call it again from queue_rq because requeue_work can
> start h/w queues on its own. This is very similar to scsi's queue_rq
> in scsi_lib.c.

The rationale in SCSI is that we stop the queue when we are above
the hosts or targets queuing limit, and only I/O ocmpletions will
wake it up again.  Kicking of the sotpped queues in blk_mq_requeue_work
is SCSI specific behavior that leaked into the core, and it's my fault..

Well' let's get the fix in as-is for now, but I really need to sort
this out properly, so maybe add some comments.

> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---

[PATCH] NVMe: Fix possible scheduling while atomic error

[Sagi Grimberg]

>>>> AFAICT, the patch is fine as-is if Christoph is okay with it.
>>>
>>> I still don't why we need to delay the call to blk_mq_stop_hw_queues
>>> to ->queue_rq.
>>
>> Eh? It's still stopped in the same place as always, so it's not
>> delayed. We just call it again from queue_rq because requeue_work can
>> start h/w queues on its own. This is very similar to scsi's queue_rq
>> in scsi_lib.c.
>
> The rationale in SCSI is that we stop the queue when we are above
> the hosts or targets queuing limit, and only I/O ocmpletions will
> wake it up again.  Kicking of the sotpped queues in blk_mq_requeue_work
> is SCSI specific behavior that leaked into the core, and it's my fault..
>
> Well' let's get the fix in as-is for now, but I really need to sort
> this out properly, so maybe add some comments.

I really don't like this patch (sorry), having queue_rq being aware
of what requeue_work *might* do looks backwards to me...
now I'm thinking what I need to do in fabrics rdma and loop. This is
why I didn't like the NVME_NS_DEAD check in queue_rq as well.

I'd really prefer to not propagate this backwards logic into
other transports...

But, if this is mandatory for now, does this patch makes sense for
rdma?
-- 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 938e7d55c4a8..60aaa1b4d021 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1450,6 +1450,12 @@ static int nvme_rdma_queue_rq(struct 
blk_mq_hw_ctx *hctx,
                 goto err;
         }

+       /* XXX: This is really not the correct layer to check this */
+       if (ns && !test_bit(NVME_NS_DEAD, &ns->flags)) {
+               ret = -EAGAIN;
+               goto err;
+       }
+
         ib_dma_sync_single_for_device(dev, sqe->dma,
                         sizeof(struct nvme_command), DMA_TO_DEVICE);

@@ -1464,8 +1470,14 @@ static int nvme_rdma_queue_rq(struct 
blk_mq_hw_ctx *hctx,

         return BLK_MQ_RQ_QUEUE_OK;
  err:
-       return (ret == -ENOMEM || ret == -EAGAIN) ?
-               BLK_MQ_RQ_QUEUE_BUSY : BLK_MQ_RQ_QUEUE_ERROR;
+       if (ret == -ENOMEM || ret == -EAGAIN) {
+               spin_lock_irq(ns->queue->queue_lock);
+               if (blk_queue_stopped(rq->q))
+                       blk_mq_stop_hw_queues(ns->queue);
+               spin_unlock_irq(ns->queue->queue_lock);
+               return BLK_MQ_RQ_QUEUE_BUSY;
+       }
+       return BLK_MQ_RQ_QUEUE_ERROR;
  }

  static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
-- 

[PATCH] NVMe: Fix possible scheduling while atomic error

[Keith Busch]

On Wed, Jun 08, 2016@02:17:03PM +0300, Sagi Grimberg wrote:
> I really don't like this patch (sorry), having queue_rq being aware
> of what requeue_work *might* do looks backwards to me...
> now I'm thinking what I need to do in fabrics rdma and loop. This is
> why I didn't like the NVME_NS_DEAD check in queue_rq as well.

I agree, but I didn't find another card we can play to get the right
sequence.
 
> I'd really prefer to not propagate this backwards logic into
> other transports...
> 
> But, if this is mandatory for now, does this patch makes sense for
> rdma?

Looks right to me.


> -- 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 938e7d55c4a8..60aaa1b4d021 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1450,6 +1450,12 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx
> *hctx,
>                 goto err;
>         }
> 
> +       /* XXX: This is really not the correct layer to check this */
> +       if (ns && !test_bit(NVME_NS_DEAD, &ns->flags)) {
> +               ret = -EAGAIN;
> +               goto err;
> +       }
> +
>         ib_dma_sync_single_for_device(dev, sqe->dma,
>                         sizeof(struct nvme_command), DMA_TO_DEVICE);
> 
> @@ -1464,8 +1470,14 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx
> *hctx,
> 
>         return BLK_MQ_RQ_QUEUE_OK;
>  err:
> -       return (ret == -ENOMEM || ret == -EAGAIN) ?
> -               BLK_MQ_RQ_QUEUE_BUSY : BLK_MQ_RQ_QUEUE_ERROR;
> +       if (ret == -ENOMEM || ret == -EAGAIN) {
> +               spin_lock_irq(ns->queue->queue_lock);
> +               if (blk_queue_stopped(rq->q))
> +                       blk_mq_stop_hw_queues(ns->queue);
> +               spin_unlock_irq(ns->queue->queue_lock);
> +               return BLK_MQ_RQ_QUEUE_BUSY;
> +       }
> +       return BLK_MQ_RQ_QUEUE_ERROR;
>  }
> 
>  static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
> --