[PATCH 0/2] nvme: enable to inject errors into admin commands

[PATCH 0/2] nvme: enable to inject errors into admin commands

[Akinobu Mita]

Currenlty fault injection support for nvme only enables to inject errors
into the commands submitted to I/O queues.

This enables to inject errors into the commands submitted to the admin
queue.
    
It is useful to test error handling in the controller initialization.

	# echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
	# echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
	# echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
	# nvme reset /dev/nvme0
	# dmesg
	...
	nvme nvme0: Could not set queue count (16385)
	nvme nvme0: IO queues not created

Akinobu Mita (2):
  nvme: prepare for fault injection into admin commands
  nvme: enable to inject errors into admin commands

 drivers/nvme/host/core.c         |  7 +++++--
 drivers/nvme/host/fault_inject.c | 33 +++++++++++++++++----------------
 drivers/nvme/host/nvme.h         | 36 +++++++++++++++++++++---------------
 3 files changed, 43 insertions(+), 33 deletions(-)

Cc: Thomas Tai <thomas.tai at oracle.com>
Cc: Keith Busch <kbusch at kernel.org>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
-- 
2.7.4

[PATCH 1/2] nvme: prepare for fault injection into admin commands

[Akinobu Mita]

Currenlty fault injection support for nvme only enables to inject errors
into the commands submitted to I/O queues.

In preparation for fault injection into the admin commands, this makes
the helper functions independent of struct nvme_ns.

Cc: Thomas Tai <thomas.tai at oracle.com>
Cc: Keith Busch <kbusch at kernel.org>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/nvme/host/core.c         |  4 ++--
 drivers/nvme/host/fault_inject.c | 28 +++++++++++++++-------------
 drivers/nvme/host/nvme.h         | 20 ++++++++++++--------
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 26c8b59..9fca8457 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3698,7 +3698,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
 
 	nvme_mpath_add_disk(ns, id);
-	nvme_fault_inject_init(ns);
+	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
 	kfree(id);
 
 	return 0;
@@ -3723,7 +3723,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
 		return;
 
-	nvme_fault_inject_fini(ns);
+	nvme_fault_inject_fini(&ns->fault_inject);
 	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
 		del_gendisk(ns->disk);
 		blk_cleanup_queue(ns->queue);
diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
index 4cfd2c9..e8d8da9 100644
--- a/drivers/nvme/host/fault_inject.c
+++ b/drivers/nvme/host/fault_inject.c
@@ -15,11 +15,10 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
 static char *fail_request;
 module_param(fail_request, charp, 0000);
 
-void nvme_fault_inject_init(struct nvme_ns *ns)
+void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
+			    const char *name)
 {
 	struct dentry *dir, *parent;
-	char *name = ns->disk->disk_name;
-	struct nvme_fault_inject *fault_inj = &ns->fault_inject;
 	struct fault_attr *attr = &fault_inj->attr;
 
 	/* set default fault injection attribute */
@@ -40,7 +39,7 @@ void nvme_fault_inject_init(struct nvme_ns *ns)
 		debugfs_remove_recursive(parent);
 		return;
 	}
-	ns->fault_inject.parent = parent;
+	fault_inj->parent = parent;
 
 	/* create debugfs for status code and dont_retry */
 	fault_inj->status = NVME_SC_INVALID_OPCODE;
@@ -49,29 +48,32 @@ void nvme_fault_inject_init(struct nvme_ns *ns)
 	debugfs_create_bool("dont_retry", 0600, dir, &fault_inj->dont_retry);
 }
 
-void nvme_fault_inject_fini(struct nvme_ns *ns)
+void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inject)
 {
 	/* remove debugfs directories */
-	debugfs_remove_recursive(ns->fault_inject.parent);
+	debugfs_remove_recursive(fault_inject->parent);
 }
 
 void nvme_should_fail(struct request *req)
 {
 	struct gendisk *disk = req->rq_disk;
-	struct nvme_ns *ns = NULL;
+	struct nvme_fault_inject *fault_inject = NULL;
 	u16 status;
 
 	/*
 	 * make sure this request is coming from a valid namespace
 	 */
-	if (!disk)
-		return;
+	if (disk) {
+		struct nvme_ns *ns = disk->private_data;
+
+		if (ns)
+			fault_inject = &ns->fault_inject;
+	}
 
-	ns = disk->private_data;
-	if (ns && should_fail(&ns->fault_inject.attr, 1)) {
+	if (fault_inject && should_fail(&fault_inject->attr, 1)) {
 		/* inject status code and DNR bit */
-		status = ns->fault_inject.status;
-		if (ns->fault_inject.dont_retry)
+		status = fault_inject->status;
+		if (fault_inject->dont_retry)
 			status |= NVME_SC_DNR;
 		nvme_req(req)->status =	status;
 	}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 53f0b24..3a48b94 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -323,14 +323,14 @@ struct nvme_ns_head {
 #endif
 };
 
-#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
 struct nvme_fault_inject {
+#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
 	struct fault_attr attr;
 	struct dentry *parent;
 	bool dont_retry;	/* DNR, do not retry */
 	u16 status;		/* status code */
-};
 #endif
+};
 
 struct nvme_ns {
 	struct list_head list;
@@ -359,9 +359,7 @@ struct nvme_ns {
 #define NVME_NS_ANA_PENDING	2
 	u16 noiob;
 
-#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
 	struct nvme_fault_inject fault_inject;
-#endif
 
 };
 
@@ -382,12 +380,18 @@ struct nvme_ctrl_ops {
 };
 
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
-void nvme_fault_inject_init(struct nvme_ns *ns);
-void nvme_fault_inject_fini(struct nvme_ns *ns);
+void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
+			    const char *name);
+void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inject);
 void nvme_should_fail(struct request *req);
 #else
-static inline void nvme_fault_inject_init(struct nvme_ns *ns) {}
-static inline void nvme_fault_inject_fini(struct nvme_ns *ns) {}
+static inline void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
+					  const char *name)
+{
+}
+static inline void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inj)
+{
+}
 static inline void nvme_should_fail(struct request *req) {}
 #endif
 
-- 
2.7.4

[PATCH 2/2] nvme: enable to inject errors into admin commands

[Akinobu Mita]

This enables to inject errors into the commands submitted to the admin
queue.

It is useful to test error handling in the controller initialization.

	# echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
	# echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
	# echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
	# nvme reset /dev/nvme0
	# dmesg
	...
	nvme nvme0: Could not set queue count (16385)
	nvme nvme0: IO queues not created

Cc: Thomas Tai <thomas.tai at oracle.com>
Cc: Keith Busch <kbusch at kernel.org>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/nvme/host/core.c         |  3 +++
 drivers/nvme/host/fault_inject.c |  5 ++---
 drivers/nvme/host/nvme.h         | 20 +++++++++++---------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9fca8457..66d8199 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4093,6 +4093,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
+	nvme_fault_inject_fini(&ctrl->fault_inject);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
 	cdev_device_del(&ctrl->cdev, ctrl->device);
 }
@@ -4188,6 +4189,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	dev_pm_qos_update_user_latency_tolerance(ctrl->device,
 		min(default_ps_max_latency_us, (unsigned long)S32_MAX));
 
+	nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
+
 	return 0;
 out_free_name:
 	kfree_const(ctrl->device->kobj.name);
diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
index e8d8da9..2f038e7 100644
--- a/drivers/nvme/host/fault_inject.c
+++ b/drivers/nvme/host/fault_inject.c
@@ -60,14 +60,13 @@ void nvme_should_fail(struct request *req)
 	struct nvme_fault_inject *fault_inject = NULL;
 	u16 status;
 
-	/*
-	 * make sure this request is coming from a valid namespace
-	 */
 	if (disk) {
 		struct nvme_ns *ns = disk->private_data;
 
 		if (ns)
 			fault_inject = &ns->fault_inject;
+	} else {
+		fault_inject = &nvme_req(req)->ctrl->fault_inject;
 	}
 
 	if (fault_inject && should_fail(&fault_inject->attr, 1)) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 3a48b94..6535fd8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -147,6 +147,15 @@ enum nvme_ctrl_state {
 	NVME_CTRL_DEAD,
 };
 
+struct nvme_fault_inject {
+#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
+	struct fault_attr attr;
+	struct dentry *parent;
+	bool dont_retry;	/* DNR, do not retry */
+	u16 status;		/* status code */
+#endif
+};
+
 struct nvme_ctrl {
 	bool comp_seen;
 	enum nvme_ctrl_state state;
@@ -257,6 +266,8 @@ struct nvme_ctrl {
 	 */
 	struct thermal_zone_device *tzdev[9];
 #endif
+
+	struct nvme_fault_inject fault_inject;
 };
 
 enum nvme_iopolicy {
@@ -323,15 +334,6 @@ struct nvme_ns_head {
 #endif
 };
 
-struct nvme_fault_inject {
-#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
-	struct fault_attr attr;
-	struct dentry *parent;
-	bool dont_retry;	/* DNR, do not retry */
-	u16 status;		/* status code */
-#endif
-};
-
 struct nvme_ns {
 	struct list_head list;
 
-- 
2.7.4

[PATCH 0/2] nvme: enable to inject errors into admin commands

[Thomas Tai]

On 5/28/19 1:03 PM, Akinobu Mita wrote:
> Currenlty fault injection support for nvme only enables to inject errors
> into the commands submitted to I/O queues.
> 
> This enables to inject errors into the commands submitted to the admin
> queue.
>      
> It is useful to test error handling in the controller initialization.
> 
> 	# echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
> 	# echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
> 	# echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
> 	# nvme reset /dev/nvme0
> 	# dmesg

Hi Akinobu,
That sounds like a good idea. would you kindly update the document in 
Documentation/fault-injection/nvme-fault-injection.txt with your example?

Thank you,
Thomas

> 	...
> 	nvme nvme0: Could not set queue count (16385)
> 	nvme nvme0: IO queues not created
> 
> Akinobu Mita (2):
>    nvme: prepare for fault injection into admin commands
>    nvme: enable to inject errors into admin commands
> 
>   drivers/nvme/host/core.c         |  7 +++++--
>   drivers/nvme/host/fault_inject.c | 33 +++++++++++++++++----------------
>   drivers/nvme/host/nvme.h         | 36 +++++++++++++++++++++---------------
>   3 files changed, 43 insertions(+), 33 deletions(-)
> 
> Cc: Thomas Tai <thomas.tai at oracle.com>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> 

[PATCH 0/2] nvme: enable to inject errors into admin commands

[Akinobu Mita]

2019?5?29?(?) 2:47 Thomas Tai <thomas.tai at oracle.com>:
>
> On 5/28/19 1:03 PM, Akinobu Mita wrote:
> > Currenlty fault injection support for nvme only enables to inject errors
> > into the commands submitted to I/O queues.
> >
> > This enables to inject errors into the commands submitted to the admin
> > queue.
> >
> > It is useful to test error handling in the controller initialization.
> >
> >       # echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
> >       # echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
> >       # echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
> >       # nvme reset /dev/nvme0
> >       # dmesg
>
> Hi Akinobu,
> That sounds like a good idea. would you kindly update the document in
> Documentation/fault-injection/nvme-fault-injection.txt with your example?

Sure. I'll add the following exmple.

Example 3: Inject an error into the 10th admin command
------------------------------------------------------

echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
nvme reset /dev/nvme0

Expected Result:

After NVMe controller reset, the reinitialization may or may not succeed.
It depends on which admin command is actually forced to fail.

Message from dmesg:
...

[PATCH 1/2] nvme: prepare for fault injection into admin commands

[Minwoo Im]

On 19-05-29 02:03:37, Akinobu Mita wrote:
> Currenlty fault injection support for nvme only enables to inject errors
> into the commands submitted to I/O queues.
> 
> In preparation for fault injection into the admin commands, this makes
> the helper functions independent of struct nvme_ns.
> 
> Cc: Thomas Tai <thomas.tai at oracle.com>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> ---
>  drivers/nvme/host/core.c         |  4 ++--
>  drivers/nvme/host/fault_inject.c | 28 +++++++++++++++-------------
>  drivers/nvme/host/nvme.h         | 20 ++++++++++++--------
>  3 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 26c8b59..9fca8457 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3698,7 +3698,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  	device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
>  
>  	nvme_mpath_add_disk(ns, id);
> -	nvme_fault_inject_init(ns);
> +	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
>  	kfree(id);
>  
>  	return 0;
> @@ -3723,7 +3723,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>  		return;
>  
> -	nvme_fault_inject_fini(ns);
> +	nvme_fault_inject_fini(&ns->fault_inject);
>  	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>  		del_gendisk(ns->disk);
>  		blk_cleanup_queue(ns->queue);
> diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
> index 4cfd2c9..e8d8da9 100644
> --- a/drivers/nvme/host/fault_inject.c
> +++ b/drivers/nvme/host/fault_inject.c
> @@ -15,11 +15,10 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
>  static char *fail_request;
>  module_param(fail_request, charp, 0000);
>  
> -void nvme_fault_inject_init(struct nvme_ns *ns)
> +void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
> +			    const char *name)

Hi Akinobu,

Just a simple proposal here.  Can we have a name for the disk name with:
	const char *disk_name ?
I think the name of variable "name" can make some confusions to whom want
to use this feature at later time :)

Otherwise, in the perspective of this single patch, looks good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH 2/2] nvme: enable to inject errors into admin commands

[Minwoo Im]

On 19-05-29 02:03:38, Akinobu Mita wrote:
> This enables to inject errors into the commands submitted to the admin
> queue.
> 
> It is useful to test error handling in the controller initialization.
> 
> 	# echo 100 > /sys/kernel/debug/nvme0/fault_inject/probability
> 	# echo 1 > /sys/kernel/debug/nvme0/fault_inject/times
> 	# echo 10 > /sys/kernel/debug/nvme0/fault_inject/space
> 	# nvme reset /dev/nvme0
> 	# dmesg
> 	...
> 	nvme nvme0: Could not set queue count (16385)
> 	nvme nvme0: IO queues not created
> 
> Cc: Thomas Tai <thomas.tai at oracle.com>
> Cc: Keith Busch <kbusch at kernel.org>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> ---
>  drivers/nvme/host/core.c         |  3 +++
>  drivers/nvme/host/fault_inject.c |  5 ++---
>  drivers/nvme/host/nvme.h         | 20 +++++++++++---------
>  3 files changed, 16 insertions(+), 12 deletions(-)

This looks good to me also.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH 1/2] nvme: prepare for fault injection into admin commands

[Akinobu Mita]

2019?5?29?(?) 23:46 Minwoo Im <minwoo.im.dev at gmail.com>:
>
> On 19-05-29 02:03:37, Akinobu Mita wrote:
> > Currenlty fault injection support for nvme only enables to inject errors
> > into the commands submitted to I/O queues.
> >
> > In preparation for fault injection into the admin commands, this makes
> > the helper functions independent of struct nvme_ns.
> >
> > Cc: Thomas Tai <thomas.tai at oracle.com>
> > Cc: Keith Busch <kbusch at kernel.org>
> > Cc: Jens Axboe <axboe at fb.com>
> > Cc: Christoph Hellwig <hch at lst.de>
> > Cc: Sagi Grimberg <sagi at grimberg.me>
> > Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> > ---
> >  drivers/nvme/host/core.c         |  4 ++--
> >  drivers/nvme/host/fault_inject.c | 28 +++++++++++++++-------------
> >  drivers/nvme/host/nvme.h         | 20 ++++++++++++--------
> >  3 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 26c8b59..9fca8457 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3698,7 +3698,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> >       device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
> >
> >       nvme_mpath_add_disk(ns, id);
> > -     nvme_fault_inject_init(ns);
> > +     nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> >       kfree(id);
> >
> >       return 0;
> > @@ -3723,7 +3723,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> >       if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
> >               return;
> >
> > -     nvme_fault_inject_fini(ns);
> > +     nvme_fault_inject_fini(&ns->fault_inject);
> >       if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> >               del_gendisk(ns->disk);
> >               blk_cleanup_queue(ns->queue);
> > diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
> > index 4cfd2c9..e8d8da9 100644
> > --- a/drivers/nvme/host/fault_inject.c
> > +++ b/drivers/nvme/host/fault_inject.c
> > @@ -15,11 +15,10 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
> >  static char *fail_request;
> >  module_param(fail_request, charp, 0000);
> >
> > -void nvme_fault_inject_init(struct nvme_ns *ns)
> > +void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
> > +                         const char *name)
>
> Hi Akinobu,
>
> Just a simple proposal here.  Can we have a name for the disk name with:
>         const char *disk_name ?
> I think the name of variable "name" can make some confusions to whom want
> to use this feature at later time :)

As long as ns->disk->disk_name is passed to this argument, 'disk_name' is
good.

However, dev_name(ctrl->device) will be passed to the argument in the patch
2/2.  It's not 'disk_name' anymore, so I think 'name' or 'dev_name' is
better than 'disk_name'.

> Otherwise, in the perspective of this single patch, looks good to me.
>
> Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH 1/2] nvme: prepare for fault injection into admin commands

[Minwoo Im]

On 19-05-30 01:39:38, Akinobu Mita wrote:
> 2019?5?29?(?) 23:46 Minwoo Im <minwoo.im.dev at gmail.com>:
> >
> > On 19-05-29 02:03:37, Akinobu Mita wrote:
> > > Currenlty fault injection support for nvme only enables to inject errors
> > > into the commands submitted to I/O queues.
> > >
> > > In preparation for fault injection into the admin commands, this makes
> > > the helper functions independent of struct nvme_ns.
> > >
> > > Cc: Thomas Tai <thomas.tai at oracle.com>
> > > Cc: Keith Busch <kbusch at kernel.org>
> > > Cc: Jens Axboe <axboe at fb.com>
> > > Cc: Christoph Hellwig <hch at lst.de>
> > > Cc: Sagi Grimberg <sagi at grimberg.me>
> > > Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> > > ---
> > >  drivers/nvme/host/core.c         |  4 ++--
> > >  drivers/nvme/host/fault_inject.c | 28 +++++++++++++++-------------
> > >  drivers/nvme/host/nvme.h         | 20 ++++++++++++--------
> > >  3 files changed, 29 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index 26c8b59..9fca8457 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -3698,7 +3698,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> > >       device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
> > >
> > >       nvme_mpath_add_disk(ns, id);
> > > -     nvme_fault_inject_init(ns);
> > > +     nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> > >       kfree(id);
> > >
> > >       return 0;
> > > @@ -3723,7 +3723,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> > >       if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
> > >               return;
> > >
> > > -     nvme_fault_inject_fini(ns);
> > > +     nvme_fault_inject_fini(&ns->fault_inject);
> > >       if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> > >               del_gendisk(ns->disk);
> > >               blk_cleanup_queue(ns->queue);
> > > diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
> > > index 4cfd2c9..e8d8da9 100644
> > > --- a/drivers/nvme/host/fault_inject.c
> > > +++ b/drivers/nvme/host/fault_inject.c
> > > @@ -15,11 +15,10 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
> > >  static char *fail_request;
> > >  module_param(fail_request, charp, 0000);
> > >
> > > -void nvme_fault_inject_init(struct nvme_ns *ns)
> > > +void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
> > > +                         const char *name)
> >
> > Hi Akinobu,
> >
> > Just a simple proposal here.  Can we have a name for the disk name with:
> >         const char *disk_name ?
> > I think the name of variable "name" can make some confusions to whom want
> > to use this feature at later time :)
> 
> As long as ns->disk->disk_name is passed to this argument, 'disk_name' is
> good.
> 
> However, dev_name(ctrl->device) will be passed to the argument in the patch
> 2/2.  It's not 'disk_name' anymore, so I think 'name' or 'dev_name' is
> better than 'disk_name'.

Then can we have it "dev_name"?

> 
> > Otherwise, in the perspective of this single patch, looks good to me.
> >
> > Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH 1/2] nvme: prepare for fault injection into admin commands

[Akinobu Mita]

2019?5?30?(?) 19:20 Minwoo Im <minwoo.im.dev at gmail.com>:
>
> On 19-05-30 01:39:38, Akinobu Mita wrote:
> > 2019?5?29?(?) 23:46 Minwoo Im <minwoo.im.dev at gmail.com>:
> > >
> > > On 19-05-29 02:03:37, Akinobu Mita wrote:
> > > > Currenlty fault injection support for nvme only enables to inject errors
> > > > into the commands submitted to I/O queues.
> > > >
> > > > In preparation for fault injection into the admin commands, this makes
> > > > the helper functions independent of struct nvme_ns.
> > > >
> > > > Cc: Thomas Tai <thomas.tai at oracle.com>
> > > > Cc: Keith Busch <kbusch at kernel.org>
> > > > Cc: Jens Axboe <axboe at fb.com>
> > > > Cc: Christoph Hellwig <hch at lst.de>
> > > > Cc: Sagi Grimberg <sagi at grimberg.me>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> > > > ---
> > > >  drivers/nvme/host/core.c         |  4 ++--
> > > >  drivers/nvme/host/fault_inject.c | 28 +++++++++++++++-------------
> > > >  drivers/nvme/host/nvme.h         | 20 ++++++++++++--------
> > > >  3 files changed, 29 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > > index 26c8b59..9fca8457 100644
> > > > --- a/drivers/nvme/host/core.c
> > > > +++ b/drivers/nvme/host/core.c
> > > > @@ -3698,7 +3698,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> > > >       device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
> > > >
> > > >       nvme_mpath_add_disk(ns, id);
> > > > -     nvme_fault_inject_init(ns);
> > > > +     nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> > > >       kfree(id);
> > > >
> > > >       return 0;
> > > > @@ -3723,7 +3723,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> > > >       if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
> > > >               return;
> > > >
> > > > -     nvme_fault_inject_fini(ns);
> > > > +     nvme_fault_inject_fini(&ns->fault_inject);
> > > >       if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> > > >               del_gendisk(ns->disk);
> > > >               blk_cleanup_queue(ns->queue);
> > > > diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
> > > > index 4cfd2c9..e8d8da9 100644
> > > > --- a/drivers/nvme/host/fault_inject.c
> > > > +++ b/drivers/nvme/host/fault_inject.c
> > > > @@ -15,11 +15,10 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
> > > >  static char *fail_request;
> > > >  module_param(fail_request, charp, 0000);
> > > >
> > > > -void nvme_fault_inject_init(struct nvme_ns *ns)
> > > > +void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
> > > > +                         const char *name)
> > >
> > > Hi Akinobu,
> > >
> > > Just a simple proposal here.  Can we have a name for the disk name with:
> > >         const char *disk_name ?
> > > I think the name of variable "name" can make some confusions to whom want
> > > to use this feature at later time :)
> >
> > As long as ns->disk->disk_name is passed to this argument, 'disk_name' is
> > good.
> >
> > However, dev_name(ctrl->device) will be passed to the argument in the patch
> > 2/2.  It's not 'disk_name' anymore, so I think 'name' or 'dev_name' is
> > better than 'disk_name'.
>
> Then can we have it "dev_name"?

OK. I have no problem with the "dev_name".

[PATCH 0/2] nvme: enable to inject errors into admin commands

[Sagi Grimberg]

Hi Akinobu,

This looks fine to me as well,

Feel free to add to your v2:
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>