nvme-blkmq fixes

Sets the vector to an invalid value after it's freed so we don't free it twice.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

drivers/block/nvme-core.c +0 -0

index b1d5d87..9642c83 100644

This fixes some surprise hot plug and error handling cases.

There is still one deadlock when we run out of tags and get stuck in bt_get, but Jens sent me a test patch that I verfied fixes that when used with this set.

Quoted 5 lines · Keith Busch

This fixes some surprise hot plug and error handling cases.

There is still one deadlock when we run out of tags and get stuck in bt_get, but Jens sent me a test patch that I verfied fixes that when used with this set.

Thanks Keith, I'll apply these for the current series. Only change I'll make is turn the freeze/unfreeze exports into _GPL exports.

Here's the patch referenced. Keith, if you tested it, can I add your tested/reviewed-by to it?

Quoted 2 lines · Jens Axboe

Here's the patch referenced. Keith, if you tested it, can I add your tested/reviewed-by to it?

Oh, the perils of sending patches at the end of a Friday before holiday...

I re-tested on my dual-ported machine but without the linux-dm 3.20 bits, so we're not multipath capable here. DM rejects the device, clears its request_queue and hits a bug, like the wait queue's task_list has something invalid.

---
device-mapper: table: table load rejected: including non-request-stackable devices
device-mapper: table: unable to set table type

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff81065459>] __wake_up_common+0x1e/0x78
PGD 7bb0d067 PUD 36b24067 PMD 0
SMP
Modules linked in: nvme bnep rfcomm bluetooth rfkill nfsd auth_rpcgss

oid_registry nfs_acl nfs lockd grace fscache sunrpc dm_round_robin loop dm_multipath parport_pc evdev parport pcspkr psmouse serio_raw processor thermal_sys button ext4 crc16 jbd2 mbcache sg sr_mod cdrom sd_mod ata_generic ata_piix e1000 floppy libata scsi_mod [last unloaded: nvme]

CPU: 0 PID: 4597 Comm: multipath Tainted: G      D        3.18.0+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
task: ffff880036ac15d0 ti: ffff880076880000 task.ti: ffff880076880000
RIP: 0010:[<ffffffff81065459>]  [<ffffffff81065459>]
__wake_up_common+0x1e/0x78
RSP: 0018:ffff880076883bd8  EFLAGS: 00010096
RAX: 0000000000000296 RBX: 0000000000000001 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffff88007ab00878
RBP: ffff88007ab00880 R08: 0000000000000000 R09: 000000000000c201
R10: 000000000000c210 R11: 000000000000c1c1 R12: 0000000000000003
R13: 0000000000000000 R14: ffff880077ca5110 R15: ffff880076883d50
FS:  00007f53829d67a0(0000) GS:ffff88007f200000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 00000000776be000 CR4: 00000000000006f0
Stack:
  ffff88007c3df800 ffffffff810e75ae ffff88007f219430 ffff88007ab00878

  0000000000000296 ffff88007ab00e90 ffff880077ca5110 ffff880077ca5110
  ffff880076883d50 ffffffff8106579f ffff880077ca5110 0000000000000000
Call Trace:
  [<ffffffff810e75ae>] ? pcpu_free_area+0x79/0xf8
  [<ffffffff8106579f>] ? __wake_up+0x35/0x46
  [<ffffffff811bb67d>] ? blk_set_queue_dying+0x33/0x69
  [<ffffffff811bce39>] ? blk_cleanup_queue+0x25/0xfd
  [<ffffffff812b2ca5>] ? __dm_destroy+0x22c/0x254

I also couldn't remember if I wrote this next part. It looks like I did, and it's needed when we run out of requests. I think this still might lose a "wake" in the case we call blk_cleanup_queue() just before bt_get() calls prepare_to_wait(), so maybe need to check for dying before and after io_schedule().

---

block/blk-mq-tag.c +0 -0

index 32e8dbb..69628ef 100644

Finally as Willy pointed out, I messed the nvme_queue struct's natural alignment, so it doesn't pack.

Quoted 6 lines · Keith Busch

Call Trace:
  [<ffffffff810e75ae>] ? pcpu_free_area+0x79/0xf8
  [<ffffffff8106579f>] ? __wake_up+0x35/0x46
  [<ffffffff811bb67d>] ? blk_set_queue_dying+0x33/0x69
  [<ffffffff811bce39>] ? blk_cleanup_queue+0x25/0xfd
  [<ffffffff812b2ca5>] ? __dm_destroy+0x22c/0x254

I wonder if it cleaned up the requests lists upfront, otherwise I don't see where that would crash. I'll look into that. This particular patch isn't pushed out yet.

Quoted 11 lines · Jens Axboe

Quoted 6 lines · Keith Busch

Call Trace:
  [<ffffffff810e75ae>] ? pcpu_free_area+0x79/0xf8
  [<ffffffff8106579f>] ? __wake_up+0x35/0x46
  [<ffffffff811bb67d>] ? blk_set_queue_dying+0x33/0x69
  [<ffffffff811bce39>] ? blk_cleanup_queue+0x25/0xfd
  [<ffffffff812b2ca5>] ? __dm_destroy+0x22c/0x254

I wonder if it cleaned up the requests lists upfront, otherwise I don't see where that would crash. I'll look into that. This particular patch isn't pushed out yet.

The above failure happens because dm called blk_alloc_queue(), but never made it far enough to call blk_init_allocated_queue(). One way to fix is call blk_init_rl() from blk_alloc_queue_node() instead of blk_init_allocated_queue(). I'm not sure if that's the right way to fix it or if blk_cleanup_queue() should somehow be aware if the request_list was initialized in the first place.

Also I found out I set nvmeq->cq_vector in the wrong place, messing up h/w completion queue interrupt setup (I was wondering why things got so slow!), so I'll fix that along with the struct alignment.

Quoted 13 lines · Keith Busch

Quoted 11 lines · Jens Axboe

Quoted 6 lines · Keith Busch

Call Trace:
  [<ffffffff810e75ae>] ? pcpu_free_area+0x79/0xf8
  [<ffffffff8106579f>] ? __wake_up+0x35/0x46
  [<ffffffff811bb67d>] ? blk_set_queue_dying+0x33/0x69
  [<ffffffff811bce39>] ? blk_cleanup_queue+0x25/0xfd
  [<ffffffff812b2ca5>] ? __dm_destroy+0x22c/0x254

I wonder if it cleaned up the requests lists upfront, otherwise I don't see where that would crash. I'll look into that. This particular patch isn't pushed out yet.

Quoted 6 lines

The above failure happens because dm called blk_alloc_queue(), but never made it far enough to call blk_init_allocated_queue(). One way to fix is call blk_init_rl() from blk_alloc_queue_node() instead of blk_init_allocated_queue(). I'm not sure if that's the right way to fix it or if blk_cleanup_queue() should somehow be aware if the request_list was initialized in the first place.

OK, I'll take care of this one.

Should be enough to just check for ->rq_pool being initialized or not - if it is, we could have waiters and we know the waitqueues have been setup, etc.

V2 attached.

Quoted 4 lines · Jens Axboe

Should be enough to just check for ->rq_pool being initialized or not - if it is, we could have waiters and we know the waitqueues have been setup, etc.

Quoted 1 line

V2 attached.

Yep, that fixes the bug.

I'm not sure I follow your suggestion for forcing bt_get() to abandon allocating a request tag when the queue is dying. If hctx_may_queue() fails, it returns a generic error and bt_get() reschedules itself. Should a different error than -1 be returned if the queue is dying?

Quoted 5 lines · Keith Busch

Quoted 4 lines · Jens Axboe

Should be enough to just check for ->rq_pool being initialized or not
- if it is, we could have waiters and we know the waitqueues have been setup, etc.

Quoted 8 lines

Quoted 1 line

V2 attached.

Yep, that fixes the bug.

I'm not sure I follow your suggestion for forcing bt_get() to abandon allocating a request tag when the queue is dying. If hctx_may_queue() fails, it returns a generic error and bt_get() reschedules itself. Should a different error than -1 be returned if the queue is dying?

We're making good incremental improvements, but finding oddities the more I test this. This one's a doozy.

Requeued IO's are automatically dispatched, and I don't see an immediately available way stop them. It causes a bug because the queue doorbells are unmapped during reset, so you can't touch them when the queue should be quiesced. I could fix that by having the driver not kick the requeue_list when it knows a reset is in progress, but there's no immediate way to drain the list if the reset fails and the device requires removal, and blk_cleanup_queue() will be stuck.

Is there something available to call that I'm missing or do I need to add more removal handling?

Quoted 28 lines · Keith Busch

Quoted 7 lines · Keith Busch

Quoted 5 lines · Jens Axboe

Should be enough to just check for ->rq_pool being initialized or not
- if it is, we could have waiters and we know the waitqueues have been setup, etc.

V2 attached.

Quoted 7 lines

Yep, that fixes the bug.

I'm not sure I follow your suggestion for forcing bt_get() to abandon allocating a request tag when the queue is dying. If hctx_may_queue() fails, it returns a generic error and bt_get() reschedules itself. Should
a different error than -1 be returned if the queue is dying?

We're making good incremental improvements, but finding oddities the more I test this. This one's a doozy.

Requeued IO's are automatically dispatched, and I don't see an immediately
available way stop them. It causes a bug because the queue doorbells are unmapped during reset, so you can't touch them when the queue should be quiesced. I could fix that by having the driver not kick the requeue_list when it knows a reset is in progress, but there's no immediate way to drain the list if the reset fails and the device requires removal, and blk_cleanup_queue() will be stuck.

Quoted 2 lines

Is there something available to call that I'm missing or do I need to add more removal handling?

So that's actually a case where having the queues auto-started on requeue run is harmful, since we should be able to handle this situation by stopping queues, requeueing, and then having a helper to eventually abort pending requeued work, if we have to. But if you simply requeue them and defer kicking the requeue list it might work. At that point you'd either kick the requeues (and hence start processing them) if things went well on the reset, or we could have some blk_mq_abort_requeues() helper that'd kill them with -EIO instead. Would that work for you?

Quoted 4 lines · Jens Axboe

Quoted 2 lines · Keith Busch

Is there something available to call that I'm missing or do I need to add more removal handling?

Quoted 9 lines

So that's actually a case where having the queues auto-started on requeue run is harmful, since we should be able to handle this situation by stopping queues, requeueing, and then having a helper to eventually abort pending requeued work, if we have to. But if you simply requeue them and defer kicking the requeue list it might work. At that point you'd either kick the requeues (and hence start processing them) if things went well on the reset, or we could have some blk_mq_abort_requeues() helper that'd kill them with -EIO instead. Would that work for you?

Something like this.

Quoted 9 lines · Jens Axboe

So that's actually a case where having the queues auto-started on requeue run is harmful, since we should be able to handle this situation by stopping queues, requeueing, and then having a helper to eventually abort pending requeued work, if we have to. But if you simply requeue them and defer kicking the requeue list it might work. At that point you'd either kick the requeues (and hence start processing them) if things went well on the reset, or we could have some blk_mq_abort_requeues() helper that'd kill them with -EIO instead. Would that work for you?

Something like this.

drivers/block/nvme-core.c static void req_completion(struct nvme_queue · line 439

Quoted 10 lines · Keith Busch

drivers/block/nvme-core.c +2 -1

@@ -432,7 +432,8 @@ static void req_completion(struct nvme_queue
 *nvmeq, void *ctx,
        if (!(status & NVME_SC_DNR || blk_noretry_request(req))
            && (jiffies - req->start_time) < req->timeout) {
            blk_mq_requeue_request(req);
-            blk_mq_kick_requeue_list(req->q);
+            if (!blk_queue_stopped(req->q))
+                blk_mq_kick_requeue_list(req->q);
            return;
        }

Oops, experimenting with different things, took the wrong snapshot of the patch. Should be:

+ if (nvmeq->cq_vector != -1)

rather than:

+ if (!blk_queue_stopped(req->q))

Anyway, I'm going to keep messing with this until I can either hit the other failures I mentioned or convince myself it's safe before sending something for official consideration.

Quoted 17 lines · Keith Busch

drivers/block/nvme-core.c static void req_completion(struct nvme_queue · line 439

Quoted 10 lines · Keith Busch

drivers/block/nvme-core.c +2 -1

@@ -432,7 +432,8 @@ static void req_completion(struct nvme_queue
 *nvmeq, void *ctx,
        if (!(status & NVME_SC_DNR || blk_noretry_request(req))
            && (jiffies - req->start_time) < req->timeout) {
            blk_mq_requeue_request(req);
-            blk_mq_kick_requeue_list(req->q);
+            if (!blk_queue_stopped(req->q))
+                blk_mq_kick_requeue_list(req->q);
            return;
        }

Oops, experimenting with different things, took the wrong snapshot of the patch. Should be:

+ if (nvmeq->cq_vector != -1)

Quoted 7 lines

rather than:

+ if (!blk_queue_stopped(req->q))

Anyway, I'm going to keep messing with this until I can either hit the other failures I mentioned or convince myself it's safe before sending something for official consideration.

I was puzzled by the signed change for cq_vector by itself. I'll wait on more results from you before doing anything.

Abandon the whole series... Too many corner cases where this falls to pieces. I'm running high queue-depth IO with random error injection that causes requests to get on lists from ctx->rq_list, hctx->dispatch, and q->requeue_list. No matter what I do from the driver, there is always a case in either reset or removal where a requests get lost and blk_cleanup_queue never completes.

Quoted 6 lines · Keith Busch

Abandon the whole series... Too many corner cases where this falls to pieces. I'm running high queue-depth IO with random error injection that causes requests to get on lists from ctx->rq_list, hctx->dispatch, and q->requeue_list. No matter what I do from the driver, there is always a case in either reset or removal where a requests get lost and blk_cleanup_queue never completes.

Back to the drawing board, I'll drop the series. Still off from work here, I'll take a look when I get back soon. I'm surprised it's this difficult, we already went through most of this design/test hash out with scsi-mq.

I'll drop this one:

NVMe: Freeze queues on shutdown

but keep this one:

NVMe: Fix double free irq

Agree?

Quoted 15 lines · Jens Axboe

Quoted 6 lines · Keith Busch

Abandon the whole series... Too many corner cases where this falls to pieces. I'm running high queue-depth IO with random error injection that causes requests to get on lists from ctx->rq_list, hctx->dispatch, and q->requeue_list. No matter what I do from the driver, there is always a case in either reset or removal where a requests get lost and blk_cleanup_queue never completes.

Back to the drawing board, I'll drop the series. Still off from work here, I'll take a look when I get back soon. I'm surprised it's this difficult, we already went through most of this design/test hash out with scsi-mq.

I'll drop this one:

Quoted 7 lines

NVMe: Freeze queues on shutdown

but keep this one:

NVMe: Fix double free irq

Agree?

Yep, that sounds good. Sorry I rushed out that last email without much explanation.

We need the driver to temporarily block tasks allocating new requests but let existing requests requeue. Freeze looked good, but unfreeze expects the usage count to have been 0, which it's not guaranteed with when we let failed requests requeue.

The only reason I need the freeze/unfreeze exports is for the IOCTL path that submits commands outside the block layer. If I can change all those usages to be "blk_execute_rq" or something like that, we don't need the new exports, and can block requests at a different level, but that brings me to the next issue.

We also need the driver to temporarily prevent the block layer from submitting requests to the driver's hw queues. 'blk_mq_stop_hw_queues' looked right, but anyone can restart them at the wrong time by kicking the requeue list.

Quoted 5 lines · Keith Busch

On Wed, 31 Dec 2014, Jens Axboe wrote:
We need the driver to temporarily block tasks allocating new requests but let existing requests requeue. Freeze looked good, but unfreeze expects the usage count to have been 0, which it's not guaranteed with when we let failed requests requeue.

OK, I think that is a concern we can fix. And yes, that was the intended use case for it originally.

Quoted 4 lines

We also need the driver to temporarily prevent the block layer from submitting requests to the driver's hw queues. 'blk_mq_stop_hw_queues' looked right, but anyone can restart them at the wrong time by kicking the requeue list.

The driver is the only one that should kick the requeue action into gear, which would start those queues up again. So that should be under your control already.

Quoted 11 lines · Jens Axboe

Quoted 6 lines · Keith Busch

On Wed, 31 Dec 2014, Jens Axboe wrote:
We need the driver to temporarily block tasks allocating new requests but
let existing requests requeue. Freeze looked good, but unfreeze expects the usage count to have been 0, which it's not guaranteed with when we let failed requests requeue.

OK, I think that is a concern we can fix. And yes, that was the intended use case for it originally.

Okay cool, that would help a lot.

Quoted 8 lines

Quoted 4 lines

We also need the driver to temporarily prevent the block layer from submitting requests to the driver's hw queues. 'blk_mq_stop_hw_queues' looked right, but anyone can restart them at the wrong time by kicking the requeue list.

The driver is the only one that should kick the requeue action into gear, which would start those queues up again. So that should be under your control already.

Right, we can stop the driver from kicking if it knows a device reset is occuring, but there can't be any requeue work prior to stopping all h/w queues to prevent a race condition. We could have the driver's reset handler call 'cancel_work_sync(q->requeue_work)' to address that. There's no existing driver using q->reset_work, but it looks safe to treat as public.