Re: [PATCH] nvme: add 'fail_if_no_path' sysfs attribute

Keith Busch
2021-02-19T23:17:43Z

On Fri, Feb 19, 2021 at 03:46:25PM +0100, Hannes Reinecke wrote:
> In some setups like RAID we need to return an I/O error
> once all paths are unavailable to allow the upper layers
> to start their own error recovery (like redirecting I/O
> to other mirrors).
> This patch adds a sysfs attribute 'fail_if_no_path' to
> allow the admin to enable that behaviour instead of the
> current 'queue until a path becomes available' policy.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Sounds okay to me, just some minor nits below.

> @@ -283,10 +283,13 @@ static bool nvme_available_path(struct nvme_ns_head *head)
> continue;
> switch (ns->ctrl->state) {
> case NVME_CTRL_LIVE:
> + return true;
> case NVME_CTRL_RESETTING:
> case NVME_CTRL_CONNECTING:
> /* fallthru */
> - return true;
> + if (!test_bit(NVME_NSHEAD_FAIL_IF_NO_PATH,
> + &head->flags))
> + return true;

It doesn't look like the 'fallthru' is in the right place anymore,
though I can't tell why it is there in the first place.

> default:
> break;
> }
> @@ -641,6 +644,37 @@ static ssize_t ana_state_show(struct device *dev, struct device_attribute *attr,
> }
> DEVICE_ATTR_RO(ana_state);
>
> +static ssize_t fail_if_no_path_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_FAIL_IF_NO_PATH, &head->flags) ?
> + 1 : 0);
> +}
> +
> +static ssize_t fail_if_no_path_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 fail_if_no_path, err;
> +
> + err = kstrtoint(buf, 10, &fail_if_no_path);
> + if (err)
> + return -EINVAL;
> +
> + else if (fail_if_no_path <= 0)
> + clear_bit(NVME_NSHEAD_FAIL_IF_NO_PATH, &head->flags);
> + else
> + set_bit(NVME_NSHEAD_FAIL_IF_NO_PATH, &head->flags);

The extra line above the 'else if' should be removed, or just change it
to simply 'if'.