Re: [PATCH] nvme-multipath: add fail_io_now sysfs attribute to fail queued I/O

[Nilay Shroff <nilay@linux.ibm.com>]

On 9/4/26 8:56 AM, Krishna Iyer wrote:
> When all paths to a multipath namespace are down, I/O is held on the
> head requeue list until a path returns. With ctrl_loss_tmo=-1 the
> controllers reconnect forever, so during a long fabric outage the I/O
> is held indefinitely and any process waiting on it sleeps in D state
> until the fabric heals or the host is rebooted. We hit this on
> virtualization hosts, where a SIGKILLed VM process cannot exit because
> it is still draining I/O to an unreachable NVMe/TCP target.
> 
> There is currently no way to fail this I/O without tearing something
> down. Deleting the controller (or letting ctrl_loss_tmo expire) works
> but takes every namespace on the controller with it and requires a
> manual reconnect afterwards. fast_io_fail_tmo only arms on the
> RESETTING -> CONNECTING transition, so it cannot be set once the
> outage has started. delayed_removal_secs only matters after all
> controllers are gone, which never happens with ctrl_loss_tmo=-1.
> dm-multipath has had "dmsetup message <dev> 0 fail_if_no_path" for
> this for decades; nvme multipath has no equivalent.
> 
> Add a fail_io_now attribute on the ns-head disk. Writing a true value
> sets NVME_NSHEAD_FAIL_IO_NOW, synchronizes SRCU so submitters see it,
> and kicks the requeue work. nvme_available_path() treats the flag as
> no path available, so the existing bio_io_error() branch fails the
> parked and any newly arriving I/O, for that namespace only. Controller
> state is not touched: reconnect attempts continue and other namespaces
> on the controller keep queueing. The flag is cleared in
> nvme_mpath_set_live() when a path comes back, like
> NVME_CTRL_FAILFAST_EXPIRED.
> 
> Locking, SRCU usage and sysfs visibility follow the neighboring
> delayed_removal_secs attribute; input parsing follows
> io_passthru_err_log_enabled (kstrtobool, shows on/off). Validated on
> real hardware with a 6.17 backport of this change.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Krishna Iyer <kiyer@crusoe.ai>
> ---
> Testing notes: the 6.17 backport was exercised on a virtualization
> host with a two-path NVMe/TCP namespace connected with
> ctrl_loss_tmo=-1. With both target portals firewalled off and a
> SIGKILLed VM process stuck in D state on the parked I/O, the process
> stayed unreapable for over six minutes; delayed_removal_secs=60,
> armed before the outage, never triggered since the controllers were
> CONNECTING throughout. Writing fail_io_now released the process in
> about two seconds, the reconnect loop was undisturbed, and once the
> firewall was removed the paths came back live and the attribute read
> back off on its own. A namespace on a second subsystem kept the
> default queueing behavior throughout. This posting is compile-tested
> (including W=1) on nvme-next.
> 
>   drivers/nvme/host/multipath.c | 62 +++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h      |  2 ++
>   drivers/nvme/host/sysfs.c     |  4 ++-
>   3 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index fc6800a9f7f9..a026bdfb9d7f 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -482,6 +482,15 @@ static bool nvme_available_path(struct nvme_ns_head *head)
>   	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags))
>   		return false;
>   
> +	/*
> +	 * The user requested any I/O queued or arriving while no path is
> +	 * usable to be failed immediately (e.g. to release I/O held for a
> +	 * fabric that retries reconnection indefinitely). The flag is
> +	 * cleared when a path becomes live again.
> +	 */
> +	if (test_bit(NVME_NSHEAD_FAIL_IO_NOW, &head->flags))
> +		return false;
> +
Does the intention here is to force I/O to fail irrespective of the
controller state, or the intention here's to fail I/O only when no
usable path exist? If it's latter then I believe this is not the right
place to enforce this policy as since this check makes nvme_available_path()
return false unconditionally when NVME_NSHEAD_FAIL_IO_NOW is set, without
considering whether a usable path exists.

>   	list_for_each_entry_srcu(ns, &head->list, siblings,
>   				 srcu_read_lock_held(&head->srcu)) {
>   		if (test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ns->ctrl->flags))
> @@ -780,6 +789,12 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
>   	if (!head->disk)
>   		return;
>   
> +	/*
> +	 * A path is usable again, restore the default queue-if-no-path
> +	 * behavior in case fail_io_now was set during a fabric outage.
> +	 */
> +	clear_bit(NVME_NSHEAD_FAIL_IO_NOW, &head->flags);
> +
>   	/*
>   	 * test_and_set_bit() is used because it is protecting against two nvme
>   	 * paths simultaneously calling device_add_disk() on the same namespace
> @@ -1168,6 +1183,53 @@ static ssize_t delayed_removal_secs_store(struct device *dev,
>   
>   DEVICE_ATTR_RW(delayed_removal_secs);
>   
> +static ssize_t fail_io_now_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 sysfs_emit(buf, test_bit(NVME_NSHEAD_FAIL_IO_NOW,
> +			&head->flags) ? "on\n" : "off\n");
> +}
> +
> +static ssize_t fail_io_now_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;
> +	bool enable;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&head->subsys->lock);
> +	if (enable)
> +		set_bit(NVME_NSHEAD_FAIL_IO_NOW, &head->flags);
> +	else
> +		clear_bit(NVME_NSHEAD_FAIL_IO_NOW, &head->flags);
> +	mutex_unlock(&head->subsys->lock);
> +
> +	/*
> +	 * Ensure that update to NVME_NSHEAD_FAIL_IO_NOW is seen
> +	 * by its reader.
> +	 */
> +	synchronize_srcu(&head->srcu);
> +
> +	/*
> +	 * Kick the requeue list so already-queued I/O re-evaluates path
> +	 * availability and fails immediately.
> +	 */
> +	if (enable)
> +		kblockd_schedule_work(&head->requeue_work);
> +
> +	return count;
> +}
> +
> +DEVICE_ATTR_RW(fail_io_now);
> +
>   static int nvme_lookup_ana_group_desc(struct nvme_ctrl *ctrl,
>   		struct nvme_ana_group_desc *desc, void *data)
>   {
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index eeabc72863d8..ca93a8934123 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -566,6 +566,7 @@ struct nvme_ns_head {
>   	unsigned int		delayed_removal_secs;
>   #define NVME_NSHEAD_DISK_LIVE		0
>   #define NVME_NSHEAD_QUEUE_IF_NO_PATH	1
> +#define NVME_NSHEAD_FAIL_IO_NOW		2
>   	struct nvme_ns __rcu	*current_path[];
>   #endif
>   };
> @@ -1067,6 +1068,7 @@ extern struct device_attribute dev_attr_ana_state;
>   extern struct device_attribute dev_attr_queue_depth;
>   extern struct device_attribute dev_attr_numa_nodes;
>   extern struct device_attribute dev_attr_delayed_removal_secs;
> +extern struct device_attribute dev_attr_fail_io_now;
>   extern struct device_attribute subsys_attr_iopolicy;
>   
>   static inline bool nvme_disk_is_ns_head(struct gendisk *disk)
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 93513c17ad5f..c154cc78c290 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -261,6 +261,7 @@ static struct attribute *nvme_ns_attrs[] = {
>   	&dev_attr_queue_depth.attr,
>   	&dev_attr_numa_nodes.attr,
>   	&dev_attr_delayed_removal_secs.attr,
> +	&dev_attr_fail_io_now.attr,
>   #endif
>   	&dev_attr_io_passthru_err_log_enabled.attr,
>   	NULL,
> @@ -297,7 +298,8 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,
>   		if (nvme_disk_is_ns_head(dev_to_disk(dev)))
>   			return 0;
>   	}
> -	if (a == &dev_attr_delayed_removal_secs.attr) {
> +	if (a == &dev_attr_delayed_removal_secs.attr ||
> +	    a == &dev_attr_fail_io_now.attr) {

This attribute should be only exposed for fabric controller.
It looks this is being exported for non-fabric controller
as well. Moreover, I like attribute fail_if_no_path better
than fail_io_now, since it describes the actual policy being
enabled: when no usable path exists, fail I/O instead of
queueing it.

>   		struct gendisk *disk = dev_to_disk(dev);
>   
>   		if (!nvme_disk_is_ns_head(disk))
> 
> base-commit: 011e0880d366be065d273c22ad1638934748d3e0

This patch appears to be based off older kernel branch. Please
rebase it against the nvme-7.3 branch.

Thanks,
--Nilay