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

Hannes Reinecke
2021-02-20T14:59:39Z

On 2/20/21 12:16 AM, Keith Busch wrote:
> 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.
>
Indeed, the 'fallthru' is misplaced; it needs to go after
NVME_CTRL_RESETTING.

>> 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'.
>
Sure.

I'll be resending.

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