* [PATCH 0/2] nvme: enable to inject errors into admin commands
@ 2019-05-28 17:03 Akinobu Mita
2019-05-28 17:03 ` [PATCH 1/2] nvme: prepare for fault injection " Akinobu Mita
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Akinobu Mita @ 2019-05-28 17:03 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] nvme: prepare for fault injection into admin commands
2019-05-28 17:03 [PATCH 0/2] nvme: enable to inject errors into admin commands Akinobu Mita
@ 2019-05-28 17:03 ` Akinobu Mita
2019-05-29 14:46 ` Minwoo Im
2019-05-28 17:03 ` [PATCH 2/2] nvme: enable to inject errors " Akinobu Mita
2019-05-28 17:47 ` [PATCH 0/2] " Thomas Tai
2 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2019-05-28 17:03 UTC (permalink / raw)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] nvme: enable to inject errors into admin commands
2019-05-28 17:03 [PATCH 0/2] nvme: enable to inject errors into admin commands Akinobu Mita
2019-05-28 17:03 ` [PATCH 1/2] nvme: prepare for fault injection " Akinobu Mita
@ 2019-05-28 17:03 ` Akinobu Mita
2019-05-29 14:52 ` Minwoo Im
2019-05-28 17:47 ` [PATCH 0/2] " Thomas Tai
2 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2019-05-28 17:03 UTC (permalink / raw)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/2] nvme: enable to inject errors into admin commands
2019-05-28 17:03 [PATCH 0/2] nvme: enable to inject errors into admin commands Akinobu Mita
2019-05-28 17:03 ` [PATCH 1/2] nvme: prepare for fault injection " Akinobu Mita
2019-05-28 17:03 ` [PATCH 2/2] nvme: enable to inject errors " Akinobu Mita
@ 2019-05-28 17:47 ` Thomas Tai
2019-05-29 14:39 ` Akinobu Mita
2 siblings, 1 reply; 11+ messages in thread
From: Thomas Tai @ 2019-05-28 17:47 UTC (permalink / raw)
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>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2] nvme: enable to inject errors into admin commands
2019-05-28 17:47 ` [PATCH 0/2] " Thomas Tai
@ 2019-05-29 14:39 ` Akinobu Mita
2019-05-31 23:37 ` Sagi Grimberg
0 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2019-05-29 14:39 UTC (permalink / raw)
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:
...
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] nvme: prepare for fault injection into admin commands
2019-05-28 17:03 ` [PATCH 1/2] nvme: prepare for fault injection " Akinobu Mita
@ 2019-05-29 14:46 ` Minwoo Im
2019-05-29 16:39 ` Akinobu Mita
0 siblings, 1 reply; 11+ messages in thread
From: Minwoo Im @ 2019-05-29 14:46 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] nvme: enable to inject errors into admin commands
2019-05-28 17:03 ` [PATCH 2/2] nvme: enable to inject errors " Akinobu Mita
@ 2019-05-29 14:52 ` Minwoo Im
0 siblings, 0 replies; 11+ messages in thread
From: Minwoo Im @ 2019-05-29 14:52 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] nvme: prepare for fault injection into admin commands
2019-05-29 14:46 ` Minwoo Im
@ 2019-05-29 16:39 ` Akinobu Mita
2019-05-30 10:20 ` Minwoo Im
0 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2019-05-29 16:39 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] nvme: prepare for fault injection into admin commands
2019-05-29 16:39 ` Akinobu Mita
@ 2019-05-30 10:20 ` Minwoo Im
2019-05-30 13:53 ` Akinobu Mita
0 siblings, 1 reply; 11+ messages in thread
From: Minwoo Im @ 2019-05-30 10:20 UTC (permalink / raw)
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] nvme: prepare for fault injection into admin commands
2019-05-30 10:20 ` Minwoo Im
@ 2019-05-30 13:53 ` Akinobu Mita
0 siblings, 0 replies; 11+ messages in thread
From: Akinobu Mita @ 2019-05-30 13:53 UTC (permalink / raw)
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".
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2] nvme: enable to inject errors into admin commands
2019-05-29 14:39 ` Akinobu Mita
@ 2019-05-31 23:37 ` Sagi Grimberg
0 siblings, 0 replies; 11+ messages in thread
From: Sagi Grimberg @ 2019-05-31 23:37 UTC (permalink / raw)
Hi Akinobu,
This looks fine to me as well,
Feel free to add to your v2:
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-05-31 23:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 17:03 [PATCH 0/2] nvme: enable to inject errors into admin commands Akinobu Mita
2019-05-28 17:03 ` [PATCH 1/2] nvme: prepare for fault injection " Akinobu Mita
2019-05-29 14:46 ` Minwoo Im
2019-05-29 16:39 ` Akinobu Mita
2019-05-30 10:20 ` Minwoo Im
2019-05-30 13:53 ` Akinobu Mita
2019-05-28 17:03 ` [PATCH 2/2] nvme: enable to inject errors " Akinobu Mita
2019-05-29 14:52 ` Minwoo Im
2019-05-28 17:47 ` [PATCH 0/2] " Thomas Tai
2019-05-29 14:39 ` Akinobu Mita
2019-05-31 23:37 ` Sagi Grimberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.