[PATCHv2 0/2] nvme: fixup MD RAID usage

[PATCHv2 0/2] nvme: fixup MD RAID usage

[Hannes Reinecke]

Hi all,

ever since the implementation NVMe-oF does not work together with MD RAID.
MD RAID expects the device to return an I/O error on failure, and to remove
the block device if the underlying hardware is removed.
This is contrary to the implementation of NVMe-oF, which will keep on retrying
I/O while the controller is being reset, and will only remove the block device
once the last _user_ is gone.

While we already have a 'fast_io_fail_tmo' setting which would ensure
that I/O errors are returned after the timeout expired, this attribute
has to be set during the initial 'connect' call.
So the first patch adds a 'fasi_io_fail_tmo' controller sysfs
attribute which allows the admin to modify it during runtime.
To fixup the second issue a new attribute 'no_path_detach' is
implemented, which will cause the disk to be removed as once the
last controller holding a path to the namespace is removed (ie after
all reconnect attempts for that controllers are exhausted).

This is a rework of the earlier path by Keith Busch ('nvme-mpath: delete disk
after last connection'). Kudos to him for suggesting this approach.

Changes to v1:
- Integrated reviews from Sagi
- Use fast_io_fail_tmo instead of a new sysfs attribute
- Rename attribute to 'no_path_detach'

Hannes Reinecke (2):
  nvme: add 'fast_io_fail_tmo' controller sysfs attribute
  nvme: delete disk when last path is gone

 drivers/nvme/host/core.c      | 41 ++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/multipath.c | 31 +++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h      | 21 +++++++++++++++---
 3 files changed, 88 insertions(+), 5 deletions(-)

-- 
2.29.2


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

[PATCH 1/2] nvme: add 'fast_io_fail_tmo' controller sysfs attribute

[Hannes Reinecke]

Add a 'fast_io_fail_tmo' controller sysfs attribute to make the
setting configurable during runtime.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4de6a3a13575..ba639049e385 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3658,6 +3658,37 @@ static ssize_t nvme_ctrl_loss_tmo_store(struct device *dev,
 static DEVICE_ATTR(ctrl_loss_tmo, S_IRUGO | S_IWUSR,
 	nvme_ctrl_loss_tmo_show, nvme_ctrl_loss_tmo_store);
 
+static ssize_t nvme_fast_io_fail_tmo_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct nvmf_ctrl_options *opts = ctrl->opts;
+
+	if (opts->fast_io_fail_tmo == -1)
+		return sprintf(buf, "off\n");
+	return sprintf(buf, "%d\n", opts->fast_io_fail_tmo);
+}
+
+static ssize_t nvme_fast_io_fail_tmo_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct nvmf_ctrl_options *opts = ctrl->opts;
+	int fast_io_fail_tmo, err;
+
+	err = kstrtoint(buf, 10, &fast_io_fail_tmo);
+	if (err)
+		return -EINVAL;
+
+	else if (fast_io_fail_tmo < 0)
+		opts->fast_io_fail_tmo = -1;
+	else
+		opts->fast_io_fail_tmo = fast_io_fail_tmo;
+	return count;
+}
+static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
+	nvme_fast_io_fail_tmo_show, nvme_fast_io_fail_tmo_store);
+
 static ssize_t nvme_ctrl_reconnect_delay_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -3703,6 +3734,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_hostnqn.attr,
 	&dev_attr_hostid.attr,
 	&dev_attr_ctrl_loss_tmo.attr,
+	&dev_attr_fast_io_fail_tmo.attr,
 	&dev_attr_reconnect_delay.attr,
 	NULL
 };
-- 
2.29.2


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

[PATCH 2/2] nvme: delete disk when last path is gone

[Hannes Reinecke]

The multipath code currently deletes the disk only after all references
to it are dropped rather than when the last path to that disk is lost.
This differs from the behaviour in the non-multipathed case where the
disk is deleted once the controller is removed.
This has been reported to cause problems with some use cases like MD RAID.

This patch implements an alternative behaviour of deleting the disk when
the last path is gone, ie the same behaviour as non-multipathed nvme
devices. The alternative behaviour can be enabled with the new sysfs
attribute 'no_path_detach'.

Suggested-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c      |  9 ++++++++-
 drivers/nvme/host/multipath.c | 31 ++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h      | 21 ++++++++++++++++++---
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ba639049e385..7af6ba18f461 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -541,7 +541,9 @@ static void nvme_free_ns_head(struct kref *ref)
 	struct nvme_ns_head *head =
 		container_of(ref, struct nvme_ns_head, ref);
 
-	nvme_mpath_remove_disk(head);
+	if (!test_bit(NVME_NSHEAD_NO_PATH_DETACH, &head->flags))
+		nvme_mpath_remove_disk(head);
+	nvme_mpath_put_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	cleanup_srcu_struct(&head->srcu);
 	nvme_put_subsystem(head->subsys);
@@ -3464,6 +3466,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
 #ifdef CONFIG_NVME_MULTIPATH
 	&dev_attr_ana_grpid.attr,
 	&dev_attr_ana_state.attr,
+	&dev_attr_no_path_detach.attr,
 #endif
 	NULL,
 };
@@ -3494,6 +3497,10 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
 		if (!nvme_ctrl_use_ana(nvme_get_ns_from_dev(dev)->ctrl))
 			return 0;
 	}
+	if (a == &dev_attr_no_path_detach.attr) {
+		if (dev_to_disk(dev)->fops == &nvme_bdev_ops)
+			return 0;
+	}
 #endif
 	return a->mode;
 }
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 0696319adaf6..7d38f9272490 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -641,6 +641,36 @@ static ssize_t ana_state_show(struct device *dev, struct device_attribute *attr,
 }
 DEVICE_ATTR_RO(ana_state);
 
+static ssize_t no_path_detach_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	struct nvme_ns_head *head = disk->private_data;
+
+	return sprintf(buf, "%d\n",
+		       test_bit(NVME_NSHEAD_NO_PATH_DETACH, &head->flags) ?
+		       1 : 0);
+}
+
+static ssize_t no_path_detach_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	struct nvme_ns_head *head = disk->private_data;
+	int err, no_path_detach;
+
+	err = kstrtoint(buf, 10, &no_path_detach);
+	if (err || no_path_detach < 0)
+		return -EINVAL;
+	if (no_path_detach)
+		set_bit(NVME_NSHEAD_NO_PATH_DETACH, &head->flags);
+	else
+		clear_bit(NVME_NSHEAD_NO_PATH_DETACH, &head->flags);
+	return count;
+}
+DEVICE_ATTR(no_path_detach, S_IRUGO | S_IWUSR,
+	no_path_detach_show, no_path_detach_store);
+
 static int nvme_lookup_ana_group_desc(struct nvme_ctrl *ctrl,
 		struct nvme_ana_group_desc *desc, void *data)
 {
@@ -702,7 +732,6 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 		 */
 		head->disk->queue = NULL;
 	}
-	put_disk(head->disk);
 }
 
 int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 07b34175c6ce..317f8a6cb7f4 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -417,7 +417,8 @@ struct nvme_ns_head {
 	struct work_struct	requeue_work;
 	struct mutex		lock;
 	unsigned long		flags;
-#define NVME_NSHEAD_DISK_LIVE	0
+#define NVME_NSHEAD_DISK_LIVE		0
+#define NVME_NSHEAD_NO_PATH_DETACH	1
 	struct nvme_ns __rcu	*current_path[];
 #endif
 };
@@ -680,8 +681,12 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
 {
 	struct nvme_ns_head *head = ns->head;
 
-	if (head->disk && list_empty(&head->list))
-		kblockd_schedule_work(&head->requeue_work);
+	if (head->disk && list_empty(&head->list)) {
+		if (test_bit(NVME_NSHEAD_NO_PATH_DETACH, &head->flags))
+			nvme_mpath_remove_disk(head);
+		else
+			kblockd_schedule_work(&head->requeue_work);
+	}
 }
 
 static inline void nvme_trace_bio_complete(struct request *req)
@@ -692,8 +697,15 @@ static inline void nvme_trace_bio_complete(struct request *req)
 		trace_block_bio_complete(ns->head->disk->queue, req->bio);
 }
 
+static inline void nvme_mpath_put_disk(struct nvme_ns_head *head)
+{
+	if (head->disk)
+		put_disk(head->disk);
+}
+
 extern struct device_attribute dev_attr_ana_grpid;
 extern struct device_attribute dev_attr_ana_state;
+extern struct device_attribute dev_attr_no_path_detach;
 extern struct device_attribute subsys_attr_iopolicy;
 
 #else
@@ -729,6 +741,9 @@ static inline void nvme_mpath_add_disk(struct nvme_ns *ns,
 static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 }
+static inline void nvme_mpath_put_disk(struct nvme_ns_head *head)
+{
+}
 static inline bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
 {
 	return false;
-- 
2.29.2


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

Re: [PATCH 2/2] nvme: delete disk when last path is gone

[Keith Busch]

On Thu, Feb 25, 2021 at 12:05:34PM +0100, Hannes Reinecke wrote:
> The multipath code currently deletes the disk only after all references
> to it are dropped rather than when the last path to that disk is lost.
> This differs from the behaviour in the non-multipathed case where the
> disk is deleted once the controller is removed.
> This has been reported to cause problems with some use cases like MD RAID.
> 
> This patch implements an alternative behaviour of deleting the disk when
> the last path is gone, ie the same behaviour as non-multipathed nvme
> devices. The alternative behaviour can be enabled with the new sysfs
> attribute 'no_path_detach'.

This looks ok to me. I have heard from a few people that they expected
it to work this way with the option enabled, but I suppose we do need to
retain the old behavior as default.

Reviewed-by: Keith Busch <kbusch@kernel.org>

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

Re: [PATCH 2/2] nvme: delete disk when last path is gone

[Hannes Reinecke]

On 2/25/21 5:59 PM, Keith Busch wrote:
> On Thu, Feb 25, 2021 at 12:05:34PM +0100, Hannes Reinecke wrote:
>> The multipath code currently deletes the disk only after all references
>> to it are dropped rather than when the last path to that disk is lost.
>> This differs from the behaviour in the non-multipathed case where the
>> disk is deleted once the controller is removed.
>> This has been reported to cause problems with some use cases like MD RAID.
>>
>> This patch implements an alternative behaviour of deleting the disk when
>> the last path is gone, ie the same behaviour as non-multipathed nvme
>> devices. The alternative behaviour can be enabled with the new sysfs
>> attribute 'no_path_detach'.
> 
> This looks ok to me. I have heard from a few people that they expected
> it to work this way with the option enabled, but I suppose we do need to
> retain the old behavior as default.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> 
Oh, I would _love_ to kill the old behaviour.
Especially as we now have fast_io_fail_tmo and ctrl_loss_tmo, which 
gives us enough control on how the controller and the remaining paths 
should behave (and which weren't present when fabrics got implemented).

We can make this behaviour the default, and kill the old approach next 
year if there are no complaints :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

Re: [PATCHv2 0/2] nvme: fixup MD RAID usage

[Minwoo Im]

On 21-02-25 12:05:32, Hannes Reinecke wrote:
> Hi all,
> 
> ever since the implementation NVMe-oF does not work together with MD RAID.
> MD RAID expects the device to return an I/O error on failure, and to remove
> the block device if the underlying hardware is removed.
> This is contrary to the implementation of NVMe-oF, which will keep on retrying
> I/O while the controller is being reset, and will only remove the block device
> once the last _user_ is gone.
> 
> While we already have a 'fast_io_fail_tmo' setting which would ensure
> that I/O errors are returned after the timeout expired, this attribute
> has to be set during the initial 'connect' call.
> So the first patch adds a 'fasi_io_fail_tmo' controller sysfs
> attribute which allows the admin to modify it during runtime.
> To fixup the second issue a new attribute 'no_path_detach' is
> implemented, which will cause the disk to be removed as once the
> last controller holding a path to the namespace is removed (ie after
> all reconnect attempts for that controllers are exhausted).
> 
> This is a rework of the earlier path by Keith Busch ('nvme-mpath: delete disk
> after last connection'). Kudos to him for suggesting this approach.
> 
> Changes to v1:
> - Integrated reviews from Sagi
> - Use fast_io_fail_tmo instead of a new sysfs attribute
> - Rename attribute to 'no_path_detach'
> 
> Hannes Reinecke (2):
>   nvme: add 'fast_io_fail_tmo' controller sysfs attribute
>   nvme: delete disk when last path is gone
> 
>  drivers/nvme/host/core.c      | 41 ++++++++++++++++++++++++++++++++++-
>  drivers/nvme/host/multipath.c | 31 +++++++++++++++++++++++++-
>  drivers/nvme/host/nvme.h      | 21 +++++++++++++++---
>  3 files changed, 88 insertions(+), 5 deletions(-)

Looks good to me.

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

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

Re: [PATCH 1/2] nvme: add 'fast_io_fail_tmo' controller sysfs attribute

[Sagi Grimberg]

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

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

Re: [PATCH 2/2] nvme: delete disk when last path is gone

[Sagi Grimberg]

>>> The multipath code currently deletes the disk only after all references
>>> to it are dropped rather than when the last path to that disk is lost.
>>> This differs from the behaviour in the non-multipathed case where the
>>> disk is deleted once the controller is removed.
>>> This has been reported to cause problems with some use cases like MD 
>>> RAID.
>>>
>>> This patch implements an alternative behaviour of deleting the disk when
>>> the last path is gone, ie the same behaviour as non-multipathed nvme
>>> devices. The alternative behaviour can be enabled with the new sysfs
>>> attribute 'no_path_detach'.
>>
>> This looks ok to me. I have heard from a few people that they expected
>> it to work this way with the option enabled, but I suppose we do need to
>> retain the old behavior as default.
>>
>> Reviewed-by: Keith Busch <kbusch@kernel.org>
>>
> Oh, I would _love_ to kill the old behaviour.
> Especially as we now have fast_io_fail_tmo and ctrl_loss_tmo, which 
> gives us enough control on how the controller and the remaining paths 
> should behave (and which weren't present when fabrics got implemented).
> 
> We can make this behaviour the default, and kill the old approach next 
> year if there are no complaints :-)

If this is broken, and this behavior doesn't break anything else then I
don't see why we shouldn't do that.

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