On 8/9/22 09:21, Mike Christie wrote:
> On 8/9/22 9:51 AM, Keith Busch wrote:
>> On Tue, Aug 09, 2022 at 10:56:55AM +0000, Chaitanya Kulkarni wrote:
>>> On 8/8/22 17:04, Mike Christie wrote:
>>>> +
>>>> + c.common.opcode = nvme_cmd_resv_report;
>>>> + c.common.cdw10 = cpu_to_le32(nvme_bytes_to_numd(data_len));
>>>> + c.common.cdw11 = 1;
>>>> + *eds = true;
>>>> +
>>>> +retry:
>>>> + if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
>>>> + bdev->bd_disk->fops == &nvme_ns_head_ops)
>>>> + ret = nvme_send_ns_head_pr_command(bdev, &c, data, data_len);
>>>> + else
>>>> + ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c,
>>>> + data, data_len);
>>>> + if (ret == NVME_SC_HOST_ID_INCONSIST && c.common.cdw11) {
>>>> + c.common.cdw11 = 0;
>>>> + *eds = false;
>>>> + goto retry;
>>>
>>> Unconditional retries without any limit can create problems,
>>> perhaps consider adding some soft limits.
>>
>> It's already conditioned on cdw11, which is cleared to 0 on the 2nd try. Not
>> that that's particularly clear. I'd suggest naming an enum value for it so the
>> code tells us what the signficance of cdw11 is in this context (it's the
>> Extended Data Structure control flag).
>
true, my concern is if controller went bad (not a common case but it is
H/W afterall) then we should have some soft limit to avoid infinite
retries.
> Will do that.
>
> Chaitanya for your comment, with a bad device we could hit an issue where we
> we cleared the Extended Data Structure control flag and it also returned
> NVME_SC_HOST_ID_INCONSIST and we'd be in an infinite loop, so I'll handle that.
>
that will be great.
-ck
