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

[Hannes Reinecke <hare@suse.de>]

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

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