NVMe fixups for 4.5
Subsystems (4)
cover NVMe fixups for 4.5
Sorry for the patch bomb in the middle of rc, though this is fixing legit issues.
Patches 1 and 2 are already reviewed/acked, and added those to this reviewers (thanks!).
Patches 3 and 4 are performance related and pretty straight forward.
The remaining are specifically for surprise removal and other controller failure scenarios. I had all these in a single patch, but there was too much going on, so I split at what appears to be reasonable and self contained features.
To test what was previously broken: run buffered writes to an nvme drive, remove the drive via sysfs, then pull the drive out.
Keith Busch (13):
blk-mq: End unstarted requests on dying queue
NVMe: Fix io incapable return values
NVMe: Allow request merges
NVMe: Set queue limits max_dev_sectors
NVMe: Fix namespace removal deadlock
NVMe: Remove WQ_MEM_RECLAIM from nvme work queue
NVMe: Requeue requests on suspended queues
NVMe: Poll device while still active during remove
NVMe: Simplify device reset failure
NVMe: Move error handling to failed reset handler
NVMe: Dead namespace handling
NVMe: Mark queues as dead on degraded controller
NVMe: Rate limit nvme IO warnings
block/blk-mq.c | 6 ++-- drivers/nvme/host/core.c | 74 ++++++++++++++++++++++----------------- drivers/nvme/host/nvme.h | 12 +++++-- drivers/nvme/host/pci.c | 91 ++++++++++++++++++++++++++++++------------------ 4 files changed, 113 insertions(+), 70 deletions(-)
Signature · 1 line
2.6.2.307.g37023ba
PATCH 1/13 blk-mq: End unstarted requests on dying queue
Go directly to ending a request if it wasn't started. Previously, completing a request may invoke a driver callback for a request it didn't initialize.
--- block/blk-mq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
block/blk-mq.c +4 -2
@@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,* If a request wasn't started before the queue was * marked dying, kill it here or it'll go unnoticed. */ -if (unlikely (blk_queue_dying (rq->q))) -blk_mq_complete_request (rq, -EIO ); +if (unlikely (blk_queue_dying (rq->q))) { + rq->errors = -EIO ; +blk_mq_end_request (rq, rq->errors); + }return ; }
Signature · 1 line
2.6.2.307.g37023ba
PATCH 1/13 blk-mq: End unstarted requests on dying queue
Quoted 5 lines · Keith Busch
Go directly to ending a request if it wasn't started. Previously, completing a request may invoke a driver callback for a request it didn't initialize.
Signed-off-by Keith Busch <keith.busch at intel.com>Acked-by Christoph Hellwig <hch at lst.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 1/13 blk-mq: End unstarted requests on dying queue
In reply to Keith BuschAgain,
PATCH 2/13 NVMe: Fix io incapable return values
In reply to Keith BuschThe function returns true when the controller can't handle IO.
--- drivers/nvme/host/nvme.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/nvme/host/nvme.h +2 -2
@@ -139,9 +139,9 @@ static inline bool nvme_io_incapable(struct nvme_ctrl *ctrl)u32 val =0 ;if (ctrl->ops->io_incapable (ctrl)) -return false; +return true;if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &val)) -return false; +return true;return val &NVME_CSTS_CFS ; }
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/13 NVMe: Fix io incapable return values
Quoted 5 lines · Keith Busch
The function returns true when the controller can't handle IO.
Signed-off-by Keith Busch <keith.busch at intel.com>Reviewed-by Sagi Grimberg <sagig at mellanox.com>Acked-by Christoph Hellwig <hch at lst.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 3/13 NVMe: Allow request merges
In reply to Keith BuschIt is generally more efficient to submit larger IO.
--- drivers/nvme/host/core.c | 1 - 1 file changed, 1 deletion(-)
drivers/nvme/host/core.c +0 -1
@@ -1121,7 +1121,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) ns->queue =blk_mq_init_queue (ctrl->tagset);if (IS_ERR (ns->queue)) goto out_free_ns; -queue_flag_set_unlocked (QUEUE_FLAG_NOMERGES , ns->queue);queue_flag_set_unlocked (QUEUE_FLAG_NONROT , ns->queue); ns->queue->queuedata = ns; ns->ctrl = ctrl;
Signature · 1 line
2.6.2.307.g37023ba
PATCH 3/13 NVMe: Allow request merges
Quoted 1 line · Keith Busch
It is generally more efficient to submit larger IO.
In general it is, but it might be good to try to understand why this flag was set in the first place, and why the argument for that isn't true anymore, or never was.
PATCH 3/13 NVMe: Allow request merges
Quoted 6 lines · Christoph Hellwig
Quoted 1 line · Keith Busch
It is generally more efficient to submit larger IO.
In general it is, but it might be good to try to understand why this flag was set in the first place, and why the argument for that isn't true anymore, or never was.
IIRC, it was set when the conversion to blk-mq happened, since the original driver could not do merges being bio based.
Signature · 1 line
Jens Axboe
PATCH 3/13 NVMe: Allow request merges
Quoted 7 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
In general it is, but it might be good to try to understand why this flag was set in the first place, and why the argument for that isn't true anymore, or never was.
IIRC, it was set when the conversion to blk-mq happened, since the original driver could not do merges being bio based.
This actually carried over from when it was still bio-based. The flag was set in the driver's initial commit, but should not have been.
PATCH 3/13 NVMe: Allow request merges
Quoted 11 lines · Keith Busch
Quoted 7 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
In general it is, but it might be good to try to understand why this flag was set in the first place, and why the argument for that isn't true anymore, or never was.
IIRC, it was set when the conversion to blk-mq happened, since the original driver could not do merges being bio based.
This actually carried over from when it was still bio-based. The flag was set in the driver's initial commit, but should not have been.
Yeah, but the flag was a no-op back then, since no merging is done for bio based drivers. So flag or no flag back then, no merging would have been done.
Signature · 1 line
Jens Axboe
PATCH 3/13 NVMe: Allow request merges
In reply to Keith BuschQuoted 4 lines · Keith Busch
It is generally more efficient to submit larger IO.
Signed-off-by Keith Busch <keith.busch at intel.com>---
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 3/13 NVMe: Allow request merges
In reply to Keith BuschQuoted 4 lines
It is generally more efficient to submit larger IO.
Signed-off-by Keith Busch <keith.busch at intel.com>
Oh yes, I wanted that for some time now, ever since Jens and I addressed some issues with front merges and gaps which detected by iser (I had this patch in my pipe but didn't have time to test...)
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
In reply to Keith Busch--- drivers/nvme/host/core.c | 1 + 1 file changed, 1 insertion(+)
drivers/nvme/host/core.c +1 -0
@@ -1136,6 +1136,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)blk_queue_logical_block_size (ns->queue,1 << ns->lba_shift);if (ctrl->max_hw_sectors) { + ns->queue->limits.max_dev_sectors = ctrl->max_hw_sectors;blk_queue_max_hw_sectors (ns->queue, ctrl->max_hw_sectors);blk_queue_max_segments (ns->queue, (ctrl->max_hw_sectors / (ctrl->page_size >>9 )) +1 );
Signature · 1 line
2.6.2.307.g37023ba
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
Quoted 1 line · Keith Busch
Signed-off-by Keith Busch <keith.busch at intel.com>
why?
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Keith Busch
Signed-off-by Keith Busch <keith.busch at intel.com>why?
Ah, missing change log here.
It is so the queue limits will be correct. The max_hw_sectors will be wrong without this.
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
Quoted 10 lines · Keith Busch
Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Keith Busch
Signed-off-by Keith Busch <keith.busch at intel.com>why?
Ah, missing change log here.
It is so the queue limits will be correct. The
max_hw_sectorswill be wrong without this.
How so? We only ever look at max_dev_sectors in blk_queue_max_hw_sectors and blk_stack_limits, and both do a min_not_zero. A zero max_dev_sectors should thus be perfectly fine, and in fact no one but the SCSI disk driver even bothers to set one.
Ohh, I see now - blk_set_default_limits initializes max_dev_sectors to BLK_SAFE_MAX_SECTORS instead of leaving it as zero. I think that's the issue which needs to be fixed.
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
Quoted 3 lines · Christoph Hellwig
Ohh, I see now -
blk_set_default_limitsinitializesmax_dev_sectorstoBLK_SAFE_MAX_SECTORSinstead of leaving it as zero. I think that's the issue which needs to be fixed.
Heh, that's pretty funny. I mentioned something along those lines after I broke the the block layer in rc1, but thought it was done that way on purpose. :)
So instead of this nvme specific patch, we should do this?
---
block/blk-settings.c +2 -2
@@ -91,8 +91,8 @@ void blk_set_default_limits(struct queue_limits *lim) lim->seg_boundary_mask =BLK_SEG_BOUNDARY_MASK ; lim->virt_boundary_mask =0 ; lim->max_segment_size =BLK_MAX_SEGMENT_SIZE ; - lim->max_sectors = lim->max_dev_sectors = lim->max_hw_sectors = -BLK_SAFE_MAX_SECTORS ; + lim->max_sectors = lim->max_hw_sectors =BLK_SAFE_MAX_SECTORS ; + lim->max_dev_sectors =0 ; lim->chunk_sectors =0 ; lim->max_write_same_sectors =0 ; lim->max_discard_sectors =0 ;
--
PATCH 4/13 NVMe: Set queue limits max_dev_sectors
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Keith" == Keith Busch <keith.busch at intel.com> writes:
Keith> Heh, that's pretty funny. I mentioned something along those lines Keith> after I broke the the block layer in rc1, but thought it was done Keith> that way on purpose. :)
Keith> So instead of this nvme specific patch, we should do this?
Yes. Thinko on my part, sorry about that.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
PATCH 5/13 NVMe: Fix namespace removal deadlock
In reply to Keith BuschNamespace removal holds the namespace list mutex. If the controller fails during namespace removal, the reset work will deadlock trying to acquire the mutex, and no progress will be made.
This patch completes the potentially blocking parts of namespace removal in a controller work queue so that reset work can cleanup a failed controller.
--- drivers/nvme/host/core.c | 81 ++++++++++++++++++++++++++++++------------------ drivers/nvme/host/nvme.h | 6 ++++ 2 files changed, 57 insertions(+), 30 deletions(-)
drivers/nvme/host/core.c +51 -30
@@ -48,6 +48,10 @@ static void nvme_free_ns(struct kref *kref) { struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref); +mutex_lock (&ns->ctrl->namespaces_mutex); +list_del_init (&ns->list); +mutex_unlock (&ns->ctrl->namespaces_mutex); +if (ns->type ==NVME_NS_LIGHTNVM )nvme_nvm_unregister (ns->queue, ns->disk->disk_name);
@@ -1106,6 +1110,37 @@ static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)return NULL ; } +static void nvme_ns_remove_work (struct work_struct *work) +{ + struct nvme_ns *ns = container_of(work, struct nvme_ns, remove_work); +bool kill =nvme_io_incapable (ns->ctrl) && + !blk_queue_dying (ns->queue); + +if (kill) { +blk_set_queue_dying (ns->queue); + +/* +* The controller was shutdown first if we got here through +* device removal. The shutdown may requeue outstanding +* requests. These need to be aborted immediately so +* del_gendisk doesn't block indefinitely for their completion. +*/ +blk_mq_abort_requeue_list (ns->queue); + } +if (ns->disk->flags &GENHD_FL_UP ) { +if (blk_get_integrity (ns->disk)) +blk_integrity_unregister (ns->disk); +sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, + &nvme_ns_attr_group); +del_gendisk (ns->disk); + } +if (kill || !blk_queue_dying (ns->queue)) { +blk_mq_abort_requeue_list (ns->queue); +blk_cleanup_queue (ns->queue); + } +nvme_put_ns (ns); +} +static void nvme_alloc_ns (struct nvme_ctrl *ctrl,unsigned nsid) {struct nvme_ns *ns;
@@ -1161,6 +1196,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)list_add_tail (&ns->list, &ctrl->namespaces);kref_get (&ctrl->kref); +INIT_WORK (&ns->remove_work, nvme_ns_remove_work);if (ns->type ==NVME_NS_LIGHTNVM )return ;
@@ -1180,35 +1216,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)static void nvme_ns_remove (struct nvme_ns *ns) { -bool kill =nvme_io_incapable (ns->ctrl) && - !blk_queue_dying (ns->queue); - -lockdep_assert_held (&ns->ctrl->namespaces_mutex); - -if (kill) { -blk_set_queue_dying (ns->queue); - -/* -* The controller was shutdown first if we got here through -* device removal. The shutdown may requeue outstanding -* requests. These need to be aborted immediately so -* del_gendisk doesn't block indefinitely for their completion. -*/ -blk_mq_abort_requeue_list (ns->queue); - } -if (ns->disk->flags &GENHD_FL_UP ) { -if (blk_get_integrity (ns->disk)) -blk_integrity_unregister (ns->disk); -sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, - &nvme_ns_attr_group); -del_gendisk (ns->disk); - } -if (kill || !blk_queue_dying (ns->queue)) { -blk_mq_abort_requeue_list (ns->queue); -blk_cleanup_queue (ns->queue); - } -list_del_init (&ns->list); -nvme_put_ns (ns); +if (!test_and_set_bit (NVME_NS_REMOVING , &ns->flags)) +queue_work (ns->ctrl->ns_workq, &ns->remove_work); }static void nvme_validate_ns (struct nvme_ctrl *ctrl,unsigned nsid)
@@ -1305,6 +1314,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)list_for_each_entry_safe (ns, next, &ctrl->namespaces, list)nvme_ns_remove (ns);mutex_unlock (&ctrl->namespaces_mutex); + +flush_workqueue (ctrl->ns_workq); }static DEFINE_IDA (nvme_instance_ida);
@@ -1337,12 +1348,14 @@ static void nvme_release_instance(struct nvme_ctrl *ctrl) }void nvme_uninit_ctrl (struct nvme_ctrl *ctrl) - { +{device_destroy (nvme_class,MKDEV (nvme_char_major, ctrl->instance));spin_lock (&dev_list_lock);list_del (&ctrl->node);spin_unlock (&dev_list_lock); + +destroy_workqueue (ctrl->ns_workq); }static void nvme_free_ctrl (struct kref *kref)
@@ -1392,11 +1405,19 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,get_device (ctrl->device);dev_set_drvdata (ctrl->device, ctrl); + ctrl->ns_workq =alloc_workqueue ("nvme-ns" ,WQ_UNBOUND ,0 ); +if (!ctrl->ns_workq) { + ret = -ENOMEM ; + goto out_destroy_device; + } +spin_lock (&dev_list_lock);list_add_tail (&ctrl->node, &nvme_ctrl_list);spin_unlock (&dev_list_lock);return 0 ; +out_destroy_device: +device_destroy (nvme_class,MKDEV (nvme_char_major, ctrl->instance)); out_release_instance:nvme_release_instance (ctrl); out:
drivers/nvme/host/nvme.h +6 -0
@@ -72,6 +72,7 @@ struct nvme_ctrl {struct mutex namespaces_mutex;struct device *device;/* char device */ struct list_head node; +struct workqueue_struct *ns_workq;char name[12 ];char serial[20 ];
@@ -112,6 +113,11 @@ struct nvme_ns {bool ext;u8 pi_type;int type; +struct work_struct remove_work; +unsigned long flags; + +#define NVME_NS_REMOVING 0 +u64 mode_select_num_blocks;u32 mode_select_block_len; };
Signature · 1 line
2.6.2.307.g37023ba
PATCH 5/13 NVMe: Fix namespace removal deadlock
Quoted 10 lines · Keith Busch
Namespace removal holds the namespace list mutex. If the controller fails during namespace removal, the reset work will deadlock trying to acquire the mutex, and no progress will be made.
This patch completes the potentially blocking parts of namespace removal in a controller work queue so that reset work can cleanup a failed controller.
Signed-off-by Keith Busch <keith.busch at intel.com>---
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 5/13 NVMe: Fix namespace removal deadlock
In reply to Keith BuschNot sure if async namespace removal has any potential risk.
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, February 11, 2016 2:17 AM
To: linux-nvme at lists.infradead.org; Jens Axboe
Cc: Christoph Hellwig; Sagi Grimberg; Wenbo Wang; Keith Busch
Subject: [PATCH for-4.5 05/13] NVMe: Fix namespace removal deadlock
Namespace removal holds the namespace list mutex. If the controller fails during namespace removal, the reset work will deadlock trying to acquire the mutex, and no progress will be made.
This patch completes the potentially blocking parts of namespace removal in a controller work queue so that reset work can cleanup a failed controller.
--- drivers/nvme/host/core.c | 81 ++++++++++++++++++++++++++++++------------------ drivers/nvme/host/nvme.h | 6 ++++ 2 files changed, 57 insertions(+), 30 deletions(-)
drivers/nvme/host/core.c +37 -29
@@ -48,6 +48,10 @@ static void nvme_free_ns(struct kref *kref) { struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref); +mutex_lock (&ns->ctrl->namespaces_mutex); +list_del_init (&ns->list); +mutex_unlock (&ns->ctrl->namespaces_mutex); +if (ns->type ==NVME_NS_LIGHTNVM )nvme_nvm_unregister (ns->queue, ns->disk->disk_name);
@@ -1106,6 +1110,37 @@ static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)return NULL ; } +static void nvme_ns_remove_work (struct work_struct *work) { + struct nvme_ns *ns = container_of(work, struct nvme_ns, remove_work); +bool kill =nvme_io_incapable (ns->ctrl) && + !blk_queue_dying (ns->queue); + +if (kill) { +blk_set_queue_dying (ns->queue); + +/* +* The controller was shutdown first if we got here through +* device removal. The shutdown may requeue outstanding +* requests. These need to be aborted immediately so +* del_gendisk doesn't block indefinitely for their completion. +*/ +blk_mq_abort_requeue_list (ns->queue); + } +if (ns->disk->flags &GENHD_FL_UP ) { +if (blk_get_integrity (ns->disk)) +blk_integrity_unregister (ns->disk); +sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, + &nvme_ns_attr_group); +del_gendisk (ns->disk); + } +if (kill || !blk_queue_dying (ns->queue)) { +blk_mq_abort_requeue_list (ns->queue); +blk_cleanup_queue (ns->queue); + } +nvme_put_ns (ns); +} +static void nvme_alloc_ns (struct nvme_ctrl *ctrl,unsigned nsid) {struct nvme_ns *ns;
@@ -1161,6 +1196,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)list_add_tail (&ns->list, &ctrl->namespaces);kref_get (&ctrl->kref); +INIT_WORK (&ns->remove_work, nvme_ns_remove_work);if (ns->type ==NVME_NS_LIGHTNVM )return ;
@@ -1180,35 +1216,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)static void nvme_ns_remove (struct nvme_ns *ns) { -bool kill =nvme_io_incapable (ns->ctrl) && - !blk_queue_dying (ns->queue); - -lockdep_assert_held (&ns->ctrl->namespaces_mutex); - -if (kill) { -blk_set_queue_dying (ns->queue); - -/* -* The controller was shutdown first if we got here through -* device removal. The shutdown may requeue outstanding -* requests. These need to be aborted immediately so -* del_gendisk doesn't block indefinitely for their completion. -*/ -blk_mq_abort_requeue_list (ns->queue); - } -if (ns->disk->flags &GENHD_FL_UP ) { -if (blk_get_integrity (ns->disk)) -blk_integrity_unregister (ns->disk); -sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, - &nvme_ns_attr_group); -del_gendisk (ns->disk); - } -if (kill || !blk_queue_dying (ns->queue)) { -blk_mq_abort_requeue_list (ns->queue); -blk_cleanup_queue (ns->queue); - } -list_del_init (&ns->list); -nvme_put_ns (ns); +if (!test_and_set_bit (NVME_NS_REMOVING , &ns->flags)) +queue_work (ns->ctrl->ns_workq, &ns->remove_work); } static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid) @@ -1305,6 +1314,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl) list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
nvme_ns_remove(ns);
mutex_unlock(&ctrl->namespaces_mutex);
+
+ flush_workqueue(ctrl->ns_workq);
}
static DEFINE_IDA(nvme_instance_ida);
@@ -1337,12 +1348,14 @@ static void nvme_release_instance(struct nvme_ctrl *ctrl) }
void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
- {
+{
device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
spin_lock(&dev_list_lock);
list_del(&ctrl->node);
spin_unlock(&dev_list_lock);
+
+ destroy_workqueue(ctrl->ns_workq);
}
static void nvme_free_ctrl(struct kref *kref) @@ -1392,11 +1405,19 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
get_device(ctrl->device);
dev_set_drvdata(ctrl->device, ctrl);
+ ctrl->ns_workq = alloc_workqueue("nvme-ns", WQ_UNBOUND, 0);
+ if (!ctrl->ns_workq) {
+ ret = -ENOMEM;
+ goto out_destroy_device;
+ }
+
spin_lock(&dev_list_lock); list_add_tail(&ctrl->node, &nvme_ctrl_list); spin_unlock(&dev_list_lock);
return 0;
+out_destroy_device:
+ device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
out_release_instance:
nvme_release_instance(ctrl);
out:
drivers/nvme/host/nvme.h +6 -0
@@ -72,6 +72,7 @@ struct nvme_ctrl {struct mutex namespaces_mutex;struct device *device;/* char device */ struct list_head node; +struct workqueue_struct *ns_workq;char name[12 ];char serial[20 ];
@@ -112,6 +113,11 @@ struct nvme_ns {bool ext;u8 pi_type;int type; +struct work_struct remove_work; +unsigned long flags; + +#define NVME_NS_REMOVING 0 +u64 mode_select_num_blocks;u32 mode_select_block_len; };
--
2.6.2.307.g37023ba
PATCH 6/13 NVMe: Remove WQ_MEM_RECLAIM from nvme work queue
In reply to Keith BuschThis isn't used for work in the memory reclaim path, and we may need to sync with work queues that also are not flagged memory relaim. This fixes a kernel warning if we ever do sync with such a work queue.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -2227,7 +2227,7 @@ static int __init nvme_init(void)init_waitqueue_head (&nvme_kthread_wait); - nvme_workq =alloc_workqueue ("nvme" ,WQ_UNBOUND |WQ_MEM_RECLAIM ,0 ); + nvme_workq =alloc_workqueue ("nvme" ,WQ_UNBOUND ,0 );if (!nvme_workq)return -ENOMEM ;
Signature · 1 line
2.6.2.307.g37023ba
PATCH 6/13 NVMe: Remove WQ_MEM_RECLAIM from nvme work queue
Quoted 3 lines · Keith Busch
This isn't used for work in the memory reclaim path, and we may need to sync with work queues that also are not flagged memory relaim. This fixes a kernel warning if we ever do sync with such a work queue.
We do need it during memory reclaim: memory reclaim in general does I/O, which can be on NVMe. We then need the workqueue to abort a command or reset an overloaded controller to make progress. Not having WQ_MEM_RECLAIM risks deadlocks in heavily loaded systems.
PATCH 6/13 NVMe: Remove WQ_MEM_RECLAIM from nvme work queue
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Keith Busch
This isn't used for work in the memory reclaim path, and we may need to sync with work queues that also are not flagged memory relaim. This fixes a kernel warning if we ever do sync with such a work queue.
We do need it during memory reclaim: memory reclaim in general does I/O, which can be on NVMe. We then need the workqueue to abort a command or reset an overloaded controller to make progress. Not having
WQ_MEM_RECLAIMrisks deadlocks in heavily loaded systems.
Darn. Invalidating a disk drains lru, which syncs with work scheduled on the system_wq. Syncing with that from a memory reclaim work queue hits a kernel warning.
That lru drain work is reclaiming memory, though. Does this need to be using a WQ_MEM_RECLAIM queue, then?
This is the alternate patch I didn't plan to submit:
---
include/linux/workqueue.h +1 -0
@@ -356,6 +359,7 @@ extern struct workqueue_struct *system_unbound_wq;extern struct workqueue_struct *system_freezable_wq;extern struct workqueue_struct *system_power_efficient_wq;extern struct workqueue_struct *system_freezable_power_efficient_wq; +extern struct workqueue_struct *system_mem_wq;extern struct workqueue_struct * __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
kernel/workqueue.c +4 -1
@@ -5483,10 +5483,13 @@ static int __init init_workqueues(void) system_freezable_power_efficient_wq =alloc_workqueue ("events_freezable_power_efficient" ,WQ_FREEZABLE |WQ_POWER_EFFICIENT ,0 ); + system_mem_wq =alloc_workqueue ("events_mem_unbound" ,WQ_UNBOUND |WQ_MEM_RECLAIM , +WQ_UNBOUND_MAX_ACTIVE );BUG_ON (!system_wq || !system_highpri_wq || !system_long_wq || !system_unbound_wq || !system_freezable_wq || !system_power_efficient_wq || - !system_freezable_power_efficient_wq); + !system_freezable_power_efficient_wq || + !system_mem_wq);wq_watchdog_init ();
mm/swap.c +1 -1
@@ -685,7 +685,7 @@ void lru_add_drain_all(void) pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) || need_activate_page_drain(cpu)) {INIT_WORK (work, lru_add_drain_per_cpu); -schedule_work_on (cpu, work); +queue_work_on (cpu, system_mem_wq, work);cpumask_set_cpu (cpu, &has_work); } }
--
PATCH 6/13 NVMe: Remove WQ_MEM_RECLAIM from nvme work queue
In reply to Christoph HellwigQuoted 9 lines · Christoph Hellwig
Quoted 3 lines · Keith Busch
This isn't used for work in the memory reclaim path, and we may need to sync with work queues that also are not flagged memory relaim. This fixes a kernel warning if we ever do sync with such a work queue.
We do need it during memory reclaim: memory reclaim in general does I/O, which can be on NVMe. We then need the workqueue to abort a command or reset an overloaded controller to make progress. Not having
WQ_MEM_RECLAIMrisks deadlocks in heavily loaded systems.
There was a more obvious way to do this that took me another day to see. We schedule remove_work on the system work queue, and all this cleanup can happen there instead of in the reset_work on the nvme work queue.
I will drop the unneeded patches from this series, and resubmit the rest with the reviews, fixes, and recommended code changes and documentation.
PATCH 7/13 NVMe: Requeue requests on suspended queues
In reply to Keith BuschIt's possible a request may get to the driver after the nvme queue was disabled. This has the request requeue if that happens.
Note the request is still "started" by the driver, but requeuing will clear the start state for timeout handling.
--- drivers/nvme/host/pci.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/nvme/host/pci.c +5 -0
@@ -678,6 +678,11 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,blk_mq_start_request (req);spin_lock_irq (&nvmeq->q_lock); +if (unlikely (nvmeq->cq_vector <0 )) { + ret =BLK_MQ_RQ_QUEUE_BUSY ; +spin_unlock_irq (&nvmeq->q_lock); + goto out; + }__nvme_submit_cmd (nvmeq, &cmnd);nvme_process_cq (nvmeq);spin_unlock_irq (&nvmeq->q_lock);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 7/13 NVMe: Requeue requests on suspended queues
Quoted 7 lines · Keith Busch
It's possible a request may get to the driver after the nvme queue was disabled. This has the request requeue if that happens.
Note the request is still "started" by the driver, but requeuing will clear the start state for timeout handling.
Signed-off-by Keith Busch <keith.busch at intel.com>
Looks fine,
PATCH 7/13 NVMe: Requeue requests on suspended queues
In reply to Keith BuschQuoted 7 lines · Keith Busch
It's possible a request may get to the driver after the nvme queue was disabled. This has the request requeue if that happens.
Note the request is still "started" by the driver, but requeuing will clear the start state for timeout handling.
Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 7/13 NVMe: Requeue requests on suspended queues
In reply to Keith BuschLooks ok,
PATCH 7/13 NVMe: Requeue requests on suspended queues
In reply to Keith BuschIf adding synchronize_rcu and blk_sync_queue, I guess this is not needed.
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, February 11, 2016 2:17 AM
To: linux-nvme at lists.infradead.org; Jens Axboe
Cc: Christoph Hellwig; Sagi Grimberg; Wenbo Wang; Keith Busch
Subject: [PATCH for-4.5 07/13] NVMe: Requeue requests on suspended queues
It's possible a request may get to the driver after the nvme queue was disabled. This has the request requeue if that happens.
Note the request is still "started" by the driver, but requeuing will clear the start state for timeout handling.
--- drivers/nvme/host/pci.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/nvme/host/pci.c +5 -0
@@ -678,6 +678,11 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,blk_mq_start_request (req);spin_lock_irq (&nvmeq->q_lock); +if (unlikely (nvmeq->cq_vector <0 )) { + ret =BLK_MQ_RQ_QUEUE_BUSY ; +spin_unlock_irq (&nvmeq->q_lock); + goto out; + }__nvme_submit_cmd (nvmeq, &cmnd);nvme_process_cq (nvmeq);spin_unlock_irq (&nvmeq->q_lock);
--
2.6.2.307.g37023ba
PATCH 7/13 NVMe: Requeue requests on suspended queues
Quoted 1 line · Wenbo Wang
If adding
synchronize_rcuandblk_sync_queue, I guess this is not needed.
That was to sync IO tasks to make sure they saw the h/w cxt STOPPED flag. We still need to restart those queues at some point to flush queued requests to completion, and this patch is just prepping for that.
I used "blk_queue_dying" in a previous patch series. I went going back and forth on using that criteria. SCSI has device states, so trying to apply those ideas to namespaces.
PATCH 7/13 NVMe: Requeue requests on suspended queues
If device is removed, I think nvme_clear_queue cleans all those pending requests. If devices is restarted, should not cq_vector be setup before enabling the h/w ctx?
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Friday, February 12, 2016 1:00 AM
To: Wenbo Wang
Cc: linux-nvme at lists.infradead.org; Jens Axboe; Christoph Hellwig; Sagi Grimberg
Subject: Re: [PATCH for-4.5 07/13] NVMe: Requeue requests on suspended queues
Quoted 1 line · Wenbo Wang
If adding
synchronize_rcuandblk_sync_queue, I guess this is not needed.
That was to sync IO tasks to make sure they saw the h/w cxt STOPPED flag. We still need to restart those queues at some point to flush queued requests to completion, and this patch is just prepping for that.
I used "blk_queue_dying" in a previous patch series. I went going back and forth on using that criteria. SCSI has device states, so trying to apply those ideas to namespaces.
PATCH 8/13 NVMe: Poll device while still active during remove
In reply to Keith BuschA device failure or link down wouldn't have been detected during namespace removal. This patch keeps the device in the list for polling so that the thread may see such failure and initiate a reset. The device is removed from the list after disable, so we can safely flush the reset work as it can't be requeued when disable completes.
--- drivers/nvme/host/pci.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/nvme/host/pci.c +1 -5
@@ -2116,16 +2116,12 @@ static void nvme_remove(struct pci_dev *pdev) {struct nvme_dev *dev =pci_get_drvdata (pdev); -spin_lock (&dev_list_lock); -list_del_init (&dev->node); -spin_unlock (&dev_list_lock); -pci_set_drvdata (pdev,NULL ); -flush_work (&dev->reset_work);flush_work (&dev->scan_work);nvme_remove_namespaces (&dev->ctrl);nvme_uninit_ctrl (&dev->ctrl);nvme_dev_disable (dev, true); +flush_work (&dev->reset_work);nvme_dev_remove_admin (dev);nvme_free_queues (dev,0 );nvme_release_cmb (dev);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 8/13 NVMe: Poll device while still active during remove
Looks fine,
PATCH 8/13 NVMe: Poll device while still active during remove
In reply to Keith BuschQuoted 7 lines · Keith Busch
A device failure or link down wouldn't have been detected during namespace removal. This patch keeps the device in the list for polling so that the thread may see such failure and initiate a reset. The device is removed from the list after disable, so we can safely flush the reset work as it can't be requeued when disable completes.
Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 8/13 NVMe: Poll device while still active during remove
In reply to Keith BuschLookd good,
PATCH 9/13 NVMe: Simplify device reset failure
In reply to Keith BuschA reset failure schedules the device to unbind from the driver through the pci driver's remove. This cleans up all intialization, so there is no need to duplicate the potentially racy cleanup.
To help understand why a reset failed, the status is logged with the existed warning message.
--- drivers/nvme/host/pci.c | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-)
drivers/nvme/host/pci.c +16 -27
@@ -86,7 +86,6 @@ struct nvme_queue;static int nvme_reset (struct nvme_dev *dev);static void nvme_process_cq (struct nvme_queue *nvmeq); -static void nvme_remove_dead_ctrl (struct nvme_dev *dev);static void nvme_dev_disable (struct nvme_dev *dev,bool shutdown);/*
@@ -1899,10 +1898,19 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)kfree (dev); } +static void nvme_remove_dead_ctrl (struct nvme_dev *dev,int status) +{ +dev_warn (dev->dev,"Removing after probe failure status:%d\n" , status); + +kref_get (&dev->ctrl.kref); +if (!schedule_work (&dev->remove_work)) +nvme_put_ctrl (&dev->ctrl); +} +static void nvme_reset_work (struct work_struct *work) { struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work); -int result; +int result =ENODEV ;if (WARN_ON (test_bit (NVME_CTRL_RESETTING , &dev->flags))) goto out;
@@ -1922,26 +1930,26 @@ static void nvme_reset_work(struct work_struct *work) result =nvme_configure_admin_queue (dev);if (result) - goto unmap; + goto out;nvme_init_queue (dev->queues[0 ],0 ); result =nvme_alloc_admin_tags (dev);if (result) - goto disable; + goto out; result =nvme_init_identify (&dev->ctrl);if (result) - goto free_tags; + goto out; result =nvme_setup_io_queues (dev);if (result) - goto free_tags; + goto out; dev->ctrl.event_limit =NVME_NR_AEN_COMMANDS ; result =nvme_dev_list_add (dev);if (result) - goto remove; + goto out;/* * Keep the controller around but remove all namespaces if we don't have
@@ -1958,19 +1966,8 @@ static void nvme_reset_work(struct work_struct *work)clear_bit (NVME_CTRL_RESETTING , &dev->flags);return ; - remove: -nvme_dev_list_remove (dev); - free_tags: -nvme_dev_remove_admin (dev); -blk_put_queue (dev->ctrl.admin_q); - dev->ctrl.admin_q =NULL ; - dev->queues[0 ]->tags =NULL ; - disable: -nvme_disable_admin_queue (dev, false); - unmap: -nvme_dev_unmap (dev); out: -nvme_remove_dead_ctrl (dev); +nvme_remove_dead_ctrl (dev, result); }static void nvme_remove_dead_ctrl_work (struct work_struct *work)
@@ -1983,14 +1980,6 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)nvme_put_ctrl (&dev->ctrl); } -static void nvme_remove_dead_ctrl (struct nvme_dev *dev) -{ -dev_warn (dev->dev,"Removing after probe failure\n" ); -kref_get (&dev->ctrl.kref); -if (!schedule_work (&dev->remove_work)) -nvme_put_ctrl (&dev->ctrl); -} -static int nvme_reset (struct nvme_dev *dev) {if (!dev->ctrl.admin_q ||blk_queue_dying (dev->ctrl.admin_q))
Signature · 1 line
2.6.2.307.g37023ba
PATCH 9/13 NVMe: Simplify device reset failure
drivers/nvme/host/pci.c static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl) · line 1903
Quoted 1 line
drivers/nvme/host/pci.c +1
-0+dev_warn (dev->dev,"Removing after probe failure status:%d\n" , status);
Why not space after the colon?
drivers/nvme/host/pci.c static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl) · line 1913
Quoted 5 lines
static void
nvme_reset_work(struct work_struct *work){drivers/nvme/host/pci.c +1
-1struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work); -int result; +int result =ENODEV ;
negative errors, please.
Quoted 4 lines
- remove:
drivers/nvme/host/pci.c +0
-3-nvme_dev_list_remove (dev); - free_tags: -nvme_dev_remove_admin (dev);
drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1970
Quoted 10 lines
drivers/nvme/host/pci.c +1
-8-blk_put_queue (dev->ctrl.admin_q); - dev->ctrl.admin_q =NULL ; - dev->queues[0 ]->tags =NULL ; - disable: -nvme_disable_admin_queue (dev, false); - unmap: -nvme_dev_unmap (dev); out: -nvme_remove_dead_ctrl (dev); +nvme_remove_dead_ctrl (dev, result);
So this relies on the cleanup path to handle all states of partial initialization. I suspect you've audited this in the current version, but the remove path could use a comment stating that any edit will have to obey this restriction.
PATCH 9/13 NVMe: Simplify device reset failure
In reply to Keith BuschQuoted 8 lines · Keith Busch
A reset failure schedules the device to unbind from the driver through the pci driver's remove. This cleans up all intialization, so there is no need to duplicate the potentially racy cleanup.
To help understand why a reset failed, the status is logged with the existed warning message.
Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 10/13 NVMe: Move error handling to failed reset handler
In reply to Keith BuschThis moves the dead queue handling out of the namespace removal path and into the reset failure path. It fixes a deadlock condition if the controller fails or link down during del_gendisk.
--- drivers/nvme/host/core.c | 19 ++----------------- drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 17 deletions(-)
drivers/nvme/host/core.c +2 -17
@@ -64,7 +64,7 @@ static void nvme_free_ns(struct kref *kref)kfree (ns); } -static void nvme_put_ns (struct nvme_ns *ns) +void nvme_put_ns (struct nvme_ns *ns) {kref_put (&ns->kref, nvme_free_ns); }
@@ -1113,28 +1113,13 @@ static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)static void nvme_ns_remove_work (struct work_struct *work) { struct nvme_ns *ns = container_of(work, struct nvme_ns, remove_work); -bool kill =nvme_io_incapable (ns->ctrl) && - !blk_queue_dying (ns->queue); - -if (kill) { -blk_set_queue_dying (ns->queue); - -/* -* The controller was shutdown first if we got here through -* device removal. The shutdown may requeue outstanding -* requests. These need to be aborted immediately so -* del_gendisk doesn't block indefinitely for their completion. -*/ -blk_mq_abort_requeue_list (ns->queue); - } +if (ns->disk->flags &GENHD_FL_UP ) {if (blk_get_integrity (ns->disk))blk_integrity_unregister (ns->disk);sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, &nvme_ns_attr_group);del_gendisk (ns->disk); - } -if (kill || !blk_queue_dying (ns->queue)) {blk_mq_abort_requeue_list (ns->queue);blk_cleanup_queue (ns->queue); }
drivers/nvme/host/nvme.h +1 -0
@@ -270,6 +270,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,int nvme_set_features (struct nvme_ctrl *dev,unsigned fid,unsigned dword11,dma_addr_t dma_addr,u32 *result);int nvme_set_queue_count (struct nvme_ctrl *ctrl,int *count); +void nvme_put_ns (struct nvme_ns *ns);extern spinlock_t dev_list_lock;
drivers/nvme/host/pci.c +22 -0
@@ -1898,11 +1898,33 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)kfree (dev); } +static void nvme_kill_ns_queues (struct nvme_dev *dev) +{ +struct nvme_ns *ns; +struct nvme_ctrl *ctrl = &dev->ctrl; + +nvme_dev_disable (dev, false); + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (!kref_get_unless_zero (&ns->kref)) +continue ; + +blk_set_queue_dying (ns->queue); +blk_mq_abort_requeue_list (ns->queue); +blk_mq_start_stopped_hw_queues (ns->queue, true); + +nvme_put_ns (ns); + } +mutex_unlock (&ctrl->namespaces_mutex); +} +static void nvme_remove_dead_ctrl (struct nvme_dev *dev,int status) {dev_warn (dev->dev,"Removing after probe failure status:%d\n" , status);kref_get (&dev->ctrl.kref); +nvme_kill_ns_queues (dev);if (!schedule_work (&dev->remove_work))nvme_put_ctrl (&dev->ctrl); }
Signature · 1 line
2.6.2.307.g37023ba
PATCH 10/13 NVMe: Move error handling to failed reset handler
Quoted 5 lines · Keith Busch
This moves the dead queue handling out of the namespace removal path and into the reset failure path. It fixes a deadlock condition if the controller fails or link down during
del_gendisk.Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 10/13 NVMe: Move error handling to failed reset handler
In reply to Keith BuschQuoted 3 lines · Keith Busch
This moves the dead queue handling out of the namespace removal path and into the reset failure path. It fixes a deadlock condition if the controller fails or link down during
del_gendisk.
How does it fix the deadlock?
Quoted 82 lines
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 19 ++----------------- drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 17 deletions(-)drivers/nvme/host/core.c +2
-17@@ -64,7 +64,7 @@ static void nvme_free_ns(struct kref *kref)kfree (ns); } -static void nvme_put_ns (struct nvme_ns *ns) +void nvme_put_ns (struct nvme_ns *ns) {kref_put (&ns->kref, nvme_free_ns); }@@ -1113,28 +1113,13 @@ static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)static void nvme_ns_remove_work (struct work_struct *work) { struct nvme_ns *ns = container_of(work, struct nvme_ns, remove_work); -bool kill =nvme_io_incapable (ns->ctrl) && - !blk_queue_dying (ns->queue); - -if (kill) { -blk_set_queue_dying (ns->queue); - -/* -* The controller was shutdown first if we got here through -* device removal. The shutdown may requeue outstanding -* requests. These need to be aborted immediately so -* del_gendisk doesn't block indefinitely for their completion. -*/ -blk_mq_abort_requeue_list (ns->queue); - } +if (ns->disk->flags &GENHD_FL_UP ) {if (blk_get_integrity (ns->disk))blk_integrity_unregister (ns->disk);sysfs_remove_group (&disk_to_dev (ns->disk)->kobj, &nvme_ns_attr_group);del_gendisk (ns->disk); - } -if (kill || !blk_queue_dying (ns->queue)) {blk_mq_abort_requeue_list (ns->queue);blk_cleanup_queue (ns->queue); }drivers/nvme/host/nvme.h +1
-0@@ -270,6 +270,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,int nvme_set_features (struct nvme_ctrl *dev,unsigned fid,unsigned dword11,dma_addr_t dma_addr,u32 *result);int nvme_set_queue_count (struct nvme_ctrl *ctrl,int *count); +void nvme_put_ns (struct nvme_ns *ns);extern spinlock_t dev_list_lock;drivers/nvme/host/pci.c +11
-0@@ -1898,11 +1898,33 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)kfree (dev); } +static void nvme_kill_ns_queues (struct nvme_dev *dev) +{ +struct nvme_ns *ns; +struct nvme_ctrl *ctrl = &dev->ctrl; + +nvme_dev_disable (dev, false); + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (!kref_get_unless_zero (&ns->kref)) +continue ;
drivers/nvme/host/pci.c static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl) · line 1921
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+ +blk_set_queue_dying (ns->queue); +blk_mq_abort_requeue_list (ns->queue); +blk_mq_start_stopped_hw_queues (ns->queue, true); + +nvme_put_ns (ns); + } +mutex_unlock (&ctrl->namespaces_mutex); +} +
Why on earth is this pci specific? This should be in the core. Aside from that, I'd really prefer if the core can handle this without having the pci (or other) triggering it explicitly, but if this must move out of the ns remove then we need documentation on what are the rules of when the driver needs to call it.
PATCH 10/13 NVMe: Move error handling to failed reset handler
Quoted 6 lines · Sagi Grimberg
Quoted 3 lines · Keith Busch
This moves the dead queue handling out of the namespace removal path and into the reset failure path. It fixes a deadlock condition if the controller fails or link down during
del_gendisk.How does it fix the deadlock?
Previously the queues were setup for failure prior to calling del_gendisk only if the controller was broken. If the controller happened to be optimal, this process would have been skipped. If the controller then failed, the queues wouldn't be killed.
Quoted 14 lines
Quoted 14 lines
drivers/nvme/host/pci.c +14
-0+nvme_dev_disable (dev, false); + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (!kref_get_unless_zero (&ns->kref)) +continue ; + +blk_set_queue_dying (ns->queue); +blk_mq_abort_requeue_list (ns->queue); +blk_mq_start_stopped_hw_queues (ns->queue, true); + +nvme_put_ns (ns); + } +mutex_unlock (&ctrl->namespaces_mutex);
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+} +Why on earth is this pci specific? This should be in the core. Aside from that, I'd really prefer if the core can handle this without having the pci (or other) triggering it explicitly, but if this must move out of the ns remove then we need documentation on what are the rules of when the driver needs to call it.
It's PCI specific only because of the potential need to disable the controller first (nvme_dev_disable), which is currently PCI specific.
PATCH 11/13 NVMe: Dead namespace handling
In reply to Keith BuschThis adds a "dead" state to a namespace and revalidates such a namespace to 0 capacity. This will force buffered writers to stop writing pages that can't be synced, and ends requests in failure if any is submitted to such a namespace.
--- drivers/nvme/host/core.c | 4 ++++ drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 12 +++++++++++- 3 files changed, 16 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +4 -0
@@ -560,6 +560,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)u16 old_ms;unsigned short bs; +if (test_bit (NVME_NS_DEAD , &ns->flags)) { +set_capacity (disk,0 ); +return -ENODEV ; + }if (nvme_identify_ns (ns->ctrl, ns->ns_id, &id)) {dev_warn (ns->ctrl->dev,"%s: Identify failure nvme%dn%d\n" , __func__, ns->ctrl->instance, ns->ns_id);
drivers/nvme/host/nvme.h +1 -0
@@ -117,6 +117,7 @@ struct nvme_ns {unsigned long flags;#define NVME_NS_REMOVING 0 +#define NVME_NS_DEAD 1u64 mode_select_num_blocks;u32 mode_select_block_len;
drivers/nvme/host/pci.c +11 -1
@@ -678,7 +678,9 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { - ret =BLK_MQ_RQ_QUEUE_BUSY ; + ret =test_bit (NVME_NS_DEAD , &ns->flags) ? +BLK_MQ_RQ_QUEUE_ERROR : +BLK_MQ_RQ_QUEUE_BUSY ;spin_unlock_irq (&nvmeq->q_lock); goto out; }
@@ -1910,6 +1912,14 @@ static void nvme_kill_ns_queues(struct nvme_dev *dev)if (!kref_get_unless_zero (&ns->kref))continue ; +/* +* Revalidating a dead namespace sets capacity to 0. This will +* end buffered writers dirtying more pages that can't be +* synced. +*/ +if (!test_and_set_bit (NVME_NS_DEAD , &ns->flags)) +revalidate_disk (ns->disk); +blk_set_queue_dying (ns->queue);blk_mq_abort_requeue_list (ns->queue);blk_mq_start_stopped_hw_queues (ns->queue, true);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 11/13 NVMe: Dead namespace handling
Quoted 6 lines · Keith Busch
This adds a "dead" state to a namespace and revalidates such a namespace to 0 capacity. This will force buffered writers to stop writing pages that can't be synced, and ends requests in failure if any is submitted to such a namespace.
Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 11/13 NVMe: Dead namespace handling
In reply to Keith BuschQuoted 5 lines
This adds a "dead" state to a namespace and revalidates such a namespace to 0 capacity. This will force buffered writers to stop writing pages that can't be synced, and ends requests in failure if any is submitted to such a namespace.
This sorta going towards a namespace state machine (like scsi). Maybe we need to centralize it correctly instead of adding states that are relevant is sporadic areas?
Quoted 40 lines
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 4 ++++ drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 12 +++++++++++- 3 files changed, 16 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +4
-0@@ -560,6 +560,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)u16 old_ms;unsigned short bs; +if (test_bit (NVME_NS_DEAD , &ns->flags)) { +set_capacity (disk,0 ); +return -ENODEV ; + }if (nvme_identify_ns (ns->ctrl, ns->ns_id, &id)) {dev_warn (ns->ctrl->dev,"%s: Identify failure nvme%dn%d\n" , __func__, ns->ctrl->instance, ns->ns_id);drivers/nvme/host/nvme.h +1
-0@@ -117,6 +117,7 @@ struct nvme_ns {unsigned long flags;#define NVME_NS_REMOVING 0 +#define NVME_NS_DEAD 1u64 mode_select_num_blocks;u32 mode_select_block_len;drivers/nvme/host/pci.c +0
-0
drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 686
Quoted 10 lines
drivers/nvme/host/pci.c +3
-1@@ -678,7 +678,9 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { - ret =BLK_MQ_RQ_QUEUE_BUSY ; + ret =test_bit (NVME_NS_DEAD , &ns->flags) ? +BLK_MQ_RQ_QUEUE_ERROR : +BLK_MQ_RQ_QUEUE_BUSY ;spin_unlock_irq (&nvmeq->q_lock); goto out; }
I can't say I'm a fan of doing all this in queue_rq...
besides why is this state check under the cq_vector < 0 condition? This is really confusing...
PATCH 11/13 NVMe: Dead namespace handling
Quoted 8 lines · Sagi Grimberg
Quoted 4 lines
This adds a "dead" state to a namespace and revalidates such a namespace to 0 capacity. This will force buffered writers to stop writing pages that can't be synced, and ends requests in failure if any is submitted to such a namespace.
This sorta going towards a namespace state machine (like scsi). Maybe we need to centralize it correctly instead of adding states that are relevant is sporadic areas?
Yeah, we need to borrow the better ideas from scsi. Slowly moving to a state machine.
Quoted 11 lines
drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 686
Quoted 8 lines
drivers/nvme/host/pci.c +3
-1if (unlikely (nvmeq->cq_vector <0 )) { - ret =BLK_MQ_RQ_QUEUE_BUSY ; + ret =test_bit (NVME_NS_DEAD , &ns->flags) ? +BLK_MQ_RQ_QUEUE_ERROR : +BLK_MQ_RQ_QUEUE_BUSY ;spin_unlock_irq (&nvmeq->q_lock); goto out; }I can't say I'm a fan of doing all this in
queue_rq...
Quoted 2 lines
besides why is this state check under the
cq_vector< 0 condition? This is really confusing...
Agreed, that's not very clear. We will potentially have an option to disable interrupt vectors entirely in the future, so this won't be appropriate criteria if/when that happens.
I've a follow up series that uses queue states instead of irq and namespace states, but planned to target 4.6 unless an alternate proposal surfaces.
PATCH 12/13 NVMe: Mark queues as dead on degraded controller
In reply to Keith BuschIf a previously optimal controller goes degraded, the IO queues will stop working, but the controller is still managable. Prior to removing namespace, we need to fail the queues so queued and new IO will fail and allow namespace removal to progress.
--- drivers/nvme/host/pci.c | 1 + 1 file changed, 1 insertion(+)
drivers/nvme/host/pci.c +1 -0
@@ -1989,6 +1989,7 @@ static void nvme_reset_work(struct work_struct *work)*/ if (dev->online_queues <2 ) {dev_warn (dev->dev,"IO queues not created\n" ); +nvme_kill_ns_queues (dev);nvme_remove_namespaces (&dev->ctrl); }else {nvme_start_queues (&dev->ctrl);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 12/13 NVMe: Mark queues as dead on degraded controller
Quoted 6 lines · Keith Busch
If a previously optimal controller goes degraded, the IO queues will stop working, but the controller is still managable. Prior to removing namespace, we need to fail the queues so queued and new IO will fail and allow namespace removal to progress.
Signed-off-by Keith Busch <keith.busch at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 12/13 NVMe: Mark queues as dead on degraded controller
In reply to Keith BuschQuoted 16 lines
If a previously optimal controller goes degraded, the IO queues will stop working, but the controller is still managable. Prior to removing namespace, we need to fail the queues so queued and new IO will fail and allow namespace removal to progress.
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/pci.c | 1 + 1 file changed, 1 insertion(+)drivers/nvme/host/pci.c +0
-0
drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1992
Quoted 4 lines
drivers/nvme/host/pci.c +1
-0@@ -1989,6 +1989,7 @@ static void nvme_reset_work(struct work_struct *work)*/ if (dev->online_queues <2 ) {dev_warn (dev->dev,"IO queues not created\n" ); +nvme_kill_ns_queues (dev);
Another example why we need to document when should a driver call this....
PATCH 13/13 NVMe: Rate limit nvme IO warnings
In reply to Keith BuschWe don't need to spam the kernel logs with thousands of messages. We can infer all IO's are being cancelled with fewer messages.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -1005,7 +1005,7 @@ static void nvme_cancel_queue_ios(struct request *req, void *data, bool reservedif (!blk_mq_request_started (req))return ; -dev_warn (nvmeq->q_dmadev, +dev_warn_ratelimited (nvmeq->q_dmadev,"Cancelling I/O %d QID %d\n" , req->tag, nvmeq->qid); status =NVME_SC_ABORT_REQ ;
Signature · 1 line
2.6.2.307.g37023ba
PATCH 13/13 NVMe: Rate limit nvme IO warnings
Quoted 2 lines · Keith Busch
We don't need to spam the kernel logs with thousands of messages. We can infer all IO's are being cancelled with fewer messages.
Do we need this message at all?
PATCH 13/13 NVMe: Rate limit nvme IO warnings
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch
We don't need to spam the kernel logs with thousands of messages. We can infer all IO's are being cancelled with fewer messages.
Do we need this message at all?
I don't find it useful for anything other than confirming a test hit that path. Delete it?
PATCH 13/13 NVMe: Rate limit nvme IO warnings
Quoted 5 lines
Quoted 2 lines
Do we need this message at all?
I don't find it useful for anything other than confirming a test hit that path. Delete it?
Perhaps we can have it dev_dbg.
PATCH 13/13 NVMe: Rate limit nvme IO warnings
Quoted 7 lines · Sagi Grimberg
Quoted 5 lines
Quoted 2 lines
Do we need this message at all?
I don't find it useful for anything other than confirming a test hit that path. Delete it?
Perhaps we can have it
dev_dbg.
Thanks, sounds like the right compromise to me.
PATCH 13/13 NVMe: Rate limit nvme IO warnings
Quoted 10 lines · Keith Busch
Quoted 7 lines · Sagi Grimberg
Quoted 5 lines
Quoted 2 lines
Do we need this message at all?
I don't find it useful for anything other than confirming a test hit that path. Delete it?
Perhaps we can have it
dev_dbg.Thanks, sounds like the right compromise to me.
Fine with me.