[PATCH 0/6] NVMe patches for kernel v5.1

[PATCH 0/6] NVMe patches for kernel v5.1

[Bart Van Assche]

Hi Christoph,

The patches in this series are what I came up with after having analyzed the
output of gcc W=1, sparse, smatch and blktests. Please consider these patches
for kernel v5.1.

Thanks,

Bart.

Bart Van Assche (6):
  nvmet: Fix indentation
  nvme-fabrics: Document the poll function argument
  nvme-pci: Check kstrtoint() return value in queue_count_set()
  nvme: Unexport nvme_delete_ctrl_sync()
  nvme: Introduce a helper function for controller deletion
  nvme: Avoid that deleting a controller triggers a circular locking
    complaint

 drivers/nvme/host/core.c        | 21 +++++++++++++--------
 drivers/nvme/host/fabrics.c     |  1 +
 drivers/nvme/host/nvme.h        |  1 -
 drivers/nvme/host/pci.c         |  2 ++
 drivers/nvme/target/discovery.c |  2 +-
 5 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 1/6] nvmet: Fix indentation

[Bart Van Assche]

This patch avoids that smatch complains about inconsistent indentation.

Fixes: a07b4970f464 ("nvmet: add a generic NVMe target") # v4.10
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index d2cb71a0b419..a34cf4986a49 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -331,7 +331,7 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 			       cmd->get_log_page.lid);
 			req->error_loc =
 				offsetof(struct nvme_get_log_page_command, lid);
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 		}
 	case nvme_admin_identify:
 		req->data_len = NVME_IDENTIFY_DATA_SIZE;
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 1/6] nvmet: Fix indentation

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Thursday, February 14, 2019 2:50 PM
To: Christoph Hellwig
Cc: Keith Busch; Sagi Grimberg; linux-nvme at lists.infradead.org; Bart Van Assche
Subject: [PATCH 1/6] nvmet: Fix indentation
?
This patch avoids that smatch complains about inconsistent indentation.

Fixes: a07b4970f464 ("nvmet: add a generic NVMe target") # v4.10
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/target/discovery.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index d2cb71a0b419..a34cf4986a49 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -331,7 +331,7 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
??????????????????????????????? cmd->get_log_page.lid);
???????????????????????? req->error_loc =
???????????????????????????????? offsetof(struct nvme_get_log_page_command, lid);
-?????????????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+?????????????????????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
???????????????? }
???????? case nvme_admin_identify:
???????????????? req->data_len = NVME_IDENTIFY_DATA_SIZE;
-- 
2.21.0.rc0.258.g878e2cd30e-goog


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

[PATCH 1/6] nvmet: Fix indentation

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 2/6] nvme-fabrics: Document the poll function argument

[Bart Van Assche]

This patch avoids that the kernel-doc tool reports a warning when
building with W=1.

Fixes: 26c682274e0a ("nvme-fabrics: allow nvmf_connect_io_queue to poll") # v5.0-rc1
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/fabrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 3eb908c50e1a..70c09abcfcbf 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -430,6 +430,7 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);
  * @qid:	NVMe I/O queue number for the new I/O connection between
  *		host and target (note qid == 0 is illegal as this is
  *		the Admin queue, per NVMe standard).
+ * @poll:	Whether or not to poll for the completion of the connect cmd.
  *
  * This function issues a fabrics-protocol connection
  * of a NVMe I/O queue (via NVMe Fabrics "Connect" command)
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 2/6] nvme-fabrics: Document the poll function argument

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Thursday, February 14, 2019 2:50 PM
To: Christoph Hellwig
Cc: Keith Busch; Sagi Grimberg; linux-nvme at lists.infradead.org; Bart Van Assche
Subject: [PATCH 2/6] nvme-fabrics: Document the poll function argument
?
This patch avoids that the kernel-doc tool reports a warning when
building with W=1.

Fixes: 26c682274e0a ("nvme-fabrics: allow nvmf_connect_io_queue to poll") # v5.0-rc1
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/host/fabrics.c | 1 +
?1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 3eb908c50e1a..70c09abcfcbf 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -430,6 +430,7 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);
? * @qid:?????? NVMe I/O queue number for the new I/O connection between
? *???????????? host and target (note qid == 0 is illegal as this is
? *???????????? the Admin queue, per NVMe standard).
+ * @poll:????? Whether or not to poll for the completion of the connect cmd.
? *
? * This function issues a fabrics-protocol connection
? * of a NVMe I/O queue (via NVMe Fabrics "Connect" command)
-- 
2.21.0.rc0.258.g878e2cd30e-goog


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

[PATCH 2/6] nvme-fabrics: Document the poll function argument

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 3/6] nvme-pci: Check kstrtoint() return value in queue_count_set()

[Bart Van Assche]

This patch avoids that the compiler complains about 'ret' being set
but not being used when building with W=1.

Fixes: 3b6592f70ad7 ("nvme: utilize two queue maps, one for reads and one for writes") # v5.0-rc1
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9bc585415d9b..03b9524a1e0b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -157,6 +157,8 @@ static int queue_count_set(const char *val, const struct kernel_param *kp)
 	int n = 0, ret;
 
 	ret = kstrtoint(val, 10, &n);
+	if (ret)
+		return ret;
 	if (n > num_possible_cpus())
 		n = num_possible_cpus();
 
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 3/6] nvme-pci: Check kstrtoint() return value in queue_count_set()

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Thursday, February 14, 2019 2:50 PM
To: Christoph Hellwig
Cc: Keith Busch; Sagi Grimberg; linux-nvme at lists.infradead.org; Bart Van Assche
Subject: [PATCH 3/6] nvme-pci: Check kstrtoint() return value in queue_count_set()
?
This patch avoids that the compiler complains about 'ret' being set
but not being used when building with W=1.

Fixes: 3b6592f70ad7 ("nvme: utilize two queue maps, one for reads and one for writes") # v5.0-rc1
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/host/pci.c | 2 ++
?1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9bc585415d9b..03b9524a1e0b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -157,6 +157,8 @@ static int queue_count_set(const char *val, const struct kernel_param *kp)
???????? int n = 0, ret;
?
???????? ret = kstrtoint(val, 10, &n);
+?????? if (ret)
+?????????????? return ret;
???????? if (n > num_possible_cpus())
???????????????? n = num_possible_cpus();
?
-- 
2.21.0.rc0.258.g878e2cd30e-goog


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

[PATCH 3/6] nvme-pci: Check kstrtoint() return value in queue_count_set()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 4/6] nvme: Unexport nvme_delete_ctrl_sync()

[Bart Van Assche]

Since nvme_delete_ctrl_sync() is not called from any other kernel module,
unexport it.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/core.c | 3 +--
 drivers/nvme/host/nvme.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c365f0aa9433..17356c51901e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -177,7 +177,7 @@ int nvme_delete_ctrl(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_delete_ctrl);
 
-int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
+static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 {
 	int ret = 0;
 
@@ -192,7 +192,6 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 	nvme_put_ctrl(ctrl);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
 
 static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
 {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a0cc733c753e..d0c113a1388b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -457,7 +457,6 @@ void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
 int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
 int nvme_delete_ctrl(struct nvme_ctrl *ctrl);
-int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
 
 int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
 		void *log, size_t size, u64 offset);
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 4/6] nvme: Unexport nvme_delete_ctrl_sync()

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Thursday, February 14, 2019 2:50 PM
To: Christoph Hellwig
Cc: Keith Busch; Sagi Grimberg; linux-nvme at lists.infradead.org; Bart Van Assche
Subject: [PATCH 4/6] nvme: Unexport nvme_delete_ctrl_sync()
?
Since nvme_delete_ctrl_sync() is not called from any other kernel module,
unexport it.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/host/core.c | 3 +--
?drivers/nvme/host/nvme.h | 1 -
?2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c365f0aa9433..17356c51901e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -177,7 +177,7 @@ int nvme_delete_ctrl(struct nvme_ctrl *ctrl)
?}
?EXPORT_SYMBOL_GPL(nvme_delete_ctrl);
?
-int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
+static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
?{
???????? int ret = 0;
?
@@ -192,7 +192,6 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
???????? nvme_put_ctrl(ctrl);
???????? return ret;
?}
-EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
?
?static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
?{
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a0cc733c753e..d0c113a1388b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -457,7 +457,6 @@ void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
?int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
?int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
?int nvme_delete_ctrl(struct nvme_ctrl *ctrl);
-int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
?
?int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
???????????????? void *log, size_t size, u64 offset);
-- 
2.21.0.rc0.258.g878e2cd30e-goog


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

[PATCH 4/6] nvme: Unexport nvme_delete_ctrl_sync()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 5/6] nvme: Introduce a helper function for controller deletion

[Bart Van Assche]

This patch does not change any functionality but makes the next patch
in this series easier to read.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 17356c51901e..c99905bcf1e2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -151,11 +151,8 @@ int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_reset_ctrl_sync);
 
-static void nvme_delete_ctrl_work(struct work_struct *work)
+static void nvme_do_delete_ctrl(struct nvme_ctrl *ctrl)
 {
-	struct nvme_ctrl *ctrl =
-		container_of(work, struct nvme_ctrl, delete_work);
-
 	dev_info(ctrl->device,
 		 "Removing ctrl: NQN \"%s\"\n", ctrl->opts->subsysnqn);
 
@@ -167,6 +164,14 @@ static void nvme_delete_ctrl_work(struct work_struct *work)
 	nvme_put_ctrl(ctrl);
 }
 
+static void nvme_delete_ctrl_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl =
+		container_of(work, struct nvme_ctrl, delete_work);
+
+	nvme_do_delete_ctrl(ctrl);
+}
+
 int nvme_delete_ctrl(struct nvme_ctrl *ctrl)
 {
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 5/6] nvme: Introduce a helper function for controller deletion

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 6/6] nvme: Avoid that deleting a controller triggers a circular locking complaint

[Bart Van Assche]

Rework nvme_delete_ctrl_sync() such that it does not have to wait for
queued work. This patch avoids that test nvme/008 triggers the following
complaint:

WARNING: possible circular locking dependency detected
5.0.0-rc6-dbg+ #10 Not tainted
------------------------------------------------------
nvme/7918 is trying to acquire lock:
000000009a1a7b69 ((work_completion)(&ctrl->delete_work)){+.+.}, at: __flush_work+0x379/0x410

but task is already holding lock:
00000000ef5a45b4 (kn->count#389){++++}, at: kernfs_remove_self+0x196/0x210

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (kn->count#389){++++}:
       lock_acquire+0xc5/0x1e0
       __kernfs_remove+0x42a/0x4a0
       kernfs_remove_by_name_ns+0x45/0x90
       remove_files.isra.1+0x3a/0x90
       sysfs_remove_group+0x5c/0xc0
       sysfs_remove_groups+0x39/0x60
       device_remove_attrs+0x68/0xb0
       device_del+0x24d/0x570
       cdev_device_del+0x1a/0x50
       nvme_delete_ctrl_work+0xbd/0xe0
       process_one_work+0x4f1/0xa40
       worker_thread+0x67/0x5b0
       kthread+0x1cf/0x1f0
       ret_from_fork+0x24/0x30

-> #0 ((work_completion)(&ctrl->delete_work)){+.+.}:
       __lock_acquire+0x1323/0x17b0
       lock_acquire+0xc5/0x1e0
       __flush_work+0x399/0x410
       flush_work+0x10/0x20
       nvme_delete_ctrl_sync+0x65/0x70
       nvme_sysfs_delete+0x4f/0x60
       dev_attr_store+0x3e/0x50
       sysfs_kf_write+0x87/0xa0
       kernfs_fop_write+0x186/0x240
       __vfs_write+0xd7/0x430
       vfs_write+0xfa/0x260
       ksys_write+0xab/0x130
       __x64_sys_write+0x43/0x50
       do_syscall_64+0x71/0x210
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(kn->count#389);
                               lock((work_completion)(&ctrl->delete_work));
                               lock(kn->count#389);
  lock((work_completion)(&ctrl->delete_work));

 *** DEADLOCK ***

3 locks held by nvme/7918:
 #0: 00000000e2223b44 (sb_writers#6){.+.+}, at: vfs_write+0x1eb/0x260
 #1: 000000003404976f (&of->mutex){+.+.}, at: kernfs_fop_write+0x128/0x240
 #2: 00000000ef5a45b4 (kn->count#389){++++}, at: kernfs_remove_self+0x196/0x210

stack backtrace:
CPU: 4 PID: 7918 Comm: nvme Not tainted 5.0.0-rc6-dbg+ #10
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 dump_stack+0x86/0xca
 print_circular_bug.isra.36.cold.54+0x173/0x1d5
 check_prev_add.constprop.45+0x996/0x1110
 __lock_acquire+0x1323/0x17b0
 lock_acquire+0xc5/0x1e0
 __flush_work+0x399/0x410
 flush_work+0x10/0x20
 nvme_delete_ctrl_sync+0x65/0x70
 nvme_sysfs_delete+0x4f/0x60
 dev_attr_store+0x3e/0x50
 sysfs_kf_write+0x87/0xa0
 kernfs_fop_write+0x186/0x240
 __vfs_write+0xd7/0x430
 vfs_write+0xfa/0x260
 ksys_write+0xab/0x130
 __x64_sys_write+0x43/0x50
 do_syscall_64+0x71/0x210
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c99905bcf1e2..b35d6849dc81 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -191,9 +191,10 @@ static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 	 * can free the controller.
 	 */
 	nvme_get_ctrl(ctrl);
-	ret = nvme_delete_ctrl(ctrl);
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
+		ret = -EBUSY;
 	if (!ret)
-		flush_work(&ctrl->delete_work);
+		nvme_do_delete_ctrl(ctrl);
 	nvme_put_ctrl(ctrl);
 	return ret;
 }
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 6/6] nvme: Avoid that deleting a controller triggers a circular locking complaint

[Christoph Hellwig]

Keith, Sagi,

can you look over this as you two have been most involved with the state
machine lately?

On Thu, Feb 14, 2019@02:50:57PM -0800, Bart Van Assche wrote:
> Rework nvme_delete_ctrl_sync() such that it does not have to wait for
> queued work. This patch avoids that test nvme/008 triggers the following
> complaint:
> 
> WARNING: possible circular locking dependency detected
> 5.0.0-rc6-dbg+ #10 Not tainted
> ------------------------------------------------------
> nvme/7918 is trying to acquire lock:
> 000000009a1a7b69 ((work_completion)(&ctrl->delete_work)){+.+.}, at: __flush_work+0x379/0x410
> 
> but task is already holding lock:
> 00000000ef5a45b4 (kn->count#389){++++}, at: kernfs_remove_self+0x196/0x210
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (kn->count#389){++++}:
>        lock_acquire+0xc5/0x1e0
>        __kernfs_remove+0x42a/0x4a0
>        kernfs_remove_by_name_ns+0x45/0x90
>        remove_files.isra.1+0x3a/0x90
>        sysfs_remove_group+0x5c/0xc0
>        sysfs_remove_groups+0x39/0x60
>        device_remove_attrs+0x68/0xb0
>        device_del+0x24d/0x570
>        cdev_device_del+0x1a/0x50
>        nvme_delete_ctrl_work+0xbd/0xe0
>        process_one_work+0x4f1/0xa40
>        worker_thread+0x67/0x5b0
>        kthread+0x1cf/0x1f0
>        ret_from_fork+0x24/0x30
> 
> -> #0 ((work_completion)(&ctrl->delete_work)){+.+.}:
>        __lock_acquire+0x1323/0x17b0
>        lock_acquire+0xc5/0x1e0
>        __flush_work+0x399/0x410
>        flush_work+0x10/0x20
>        nvme_delete_ctrl_sync+0x65/0x70
>        nvme_sysfs_delete+0x4f/0x60
>        dev_attr_store+0x3e/0x50
>        sysfs_kf_write+0x87/0xa0
>        kernfs_fop_write+0x186/0x240
>        __vfs_write+0xd7/0x430
>        vfs_write+0xfa/0x260
>        ksys_write+0xab/0x130
>        __x64_sys_write+0x43/0x50
>        do_syscall_64+0x71/0x210
>        entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> other info that might help us debug this:
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(kn->count#389);
>                                lock((work_completion)(&ctrl->delete_work));
>                                lock(kn->count#389);
>   lock((work_completion)(&ctrl->delete_work));
> 
>  *** DEADLOCK ***
> 
> 3 locks held by nvme/7918:
>  #0: 00000000e2223b44 (sb_writers#6){.+.+}, at: vfs_write+0x1eb/0x260
>  #1: 000000003404976f (&of->mutex){+.+.}, at: kernfs_fop_write+0x128/0x240
>  #2: 00000000ef5a45b4 (kn->count#389){++++}, at: kernfs_remove_self+0x196/0x210
> 
> stack backtrace:
> CPU: 4 PID: 7918 Comm: nvme Not tainted 5.0.0-rc6-dbg+ #10
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> Call Trace:
>  dump_stack+0x86/0xca
>  print_circular_bug.isra.36.cold.54+0x173/0x1d5
>  check_prev_add.constprop.45+0x996/0x1110
>  __lock_acquire+0x1323/0x17b0
>  lock_acquire+0xc5/0x1e0
>  __flush_work+0x399/0x410
>  flush_work+0x10/0x20
>  nvme_delete_ctrl_sync+0x65/0x70
>  nvme_sysfs_delete+0x4f/0x60
>  dev_attr_store+0x3e/0x50
>  sysfs_kf_write+0x87/0xa0
>  kernfs_fop_write+0x186/0x240
>  __vfs_write+0xd7/0x430
>  vfs_write+0xfa/0x260
>  ksys_write+0xab/0x130
>  __x64_sys_write+0x43/0x50
>  do_syscall_64+0x71/0x210
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>
> ---
>  drivers/nvme/host/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c99905bcf1e2..b35d6849dc81 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -191,9 +191,10 @@ static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>  	 * can free the controller.
>  	 */
>  	nvme_get_ctrl(ctrl);
> -	ret = nvme_delete_ctrl(ctrl);
> +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
> +		ret = -EBUSY;
>  	if (!ret)
> -		flush_work(&ctrl->delete_work);
> +		nvme_do_delete_ctrl(ctrl);
>  	nvme_put_ctrl(ctrl);
>  	return ret;
>  }
> -- 
> 2.21.0.rc0.258.g878e2cd30e-goog
---end quoted text---

[PATCH 6/6] nvme: Avoid that deleting a controller triggers a circular locking complaint

[Keith Busch]

On Tue, Feb 19, 2019@04:16:55PM +0100, Christoph Hellwig wrote:
> Keith, Sagi,
> 
> can you look over this as you two have been most involved with the state
> machine lately?

This looks correct. I had to double check 'ret' was initilized to 0
outside what the diff is showing, so maybe it'd be more obvious if
nvme_do_delete_ctrl() was in an 'else' condition. Beyond that nit,
the series looks good to me.

Reviewed-by: Keith Busch <keith.busch at intel.com>
 
> On Thu, Feb 14, 2019@02:50:57PM -0800, Bart Van Assche wrote:
> > @@ -191,9 +191,10 @@ static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
> >  	 * can free the controller.
> >  	 */
> >  	nvme_get_ctrl(ctrl);
> > -	ret = nvme_delete_ctrl(ctrl);
> > +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
> > +		ret = -EBUSY;
> >  	if (!ret)
> > -		flush_work(&ctrl->delete_work);
> > +		nvme_do_delete_ctrl(ctrl);
> >  	nvme_put_ctrl(ctrl);
> >  	return ret;
> >  }

[PATCH 6/6] nvme: Avoid that deleting a controller triggers a circular locking complaint

[Sagi Grimberg]

> This looks correct. I had to double check 'ret' was initilized to 0
> outside what the diff is showing, so maybe it'd be more obvious if
> nvme_do_delete_ctrl() was in an 'else' condition.

Agreed.

[PATCH 6/6] nvme: Avoid that deleting a controller triggers a circular locking complaint

[Christoph Hellwig]

Thanks,

applied to nvme-5.1.

[PATCH 0/6] NVMe patches for kernel v5.1

[Christoph Hellwig]

Thanks,

applied 1-5 to nvme-5.1, waiting for reviews on 6.