Re: [PATCH 09/11] block, nvme: Add error for reservation conflicts.

From: michael.christie@oracle.com
To: Hannes Reinecke <hare@suse.de>, Keith Busch <kbusch@kernel.org>
Cc: linux-block@vger.kernel.org, dm-devel@redhat.com,
	snitzer@kernel.org, hch@lst.de, axboe@kernel.dk,
	martin.petersen@oracle.com,
	james.bottomley@hansenpartnership.com,
	linux-scsi@vger.kernel.org, target-devel@vger.kernel.org
Subject: Re: [PATCH 09/11] block, nvme: Add error for reservation conflicts.
Date: Sat, 4 Jun 2022 12:13:50 -0500	[thread overview]
Message-ID: <6cc818cf-fac4-d485-ea9d-0b8597f24a33@oracle.com> (raw)
In-Reply-To: <fdd77f3b-691a-a9bf-b31f-0aa845e68e59@suse.de>

On 6/4/22 2:38 AM, Hannes Reinecke wrote:
> On 6/3/22 21:45, Keith Busch wrote:
>> On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote:
>>> @@ -171,6 +171,7 @@ static const struct {
>>>       /* zone device specific errors */
>>>       [BLK_STS_ZONE_OPEN_RESOURCE]    = { -ETOOMANYREFS, "open zones exceeded" },
>>>       [BLK_STS_ZONE_ACTIVE_RESOURCE]    = { -EOVERFLOW, "active zones exceeded" },
>>> +    [BLK_STS_RSV_CONFLICT]    = { -EBADE,    "resevation conflict" },
>>
>> You misspelled "reservation". :)
>>
>> And since you want a different error, why reuse EBADE for the errno? That is
>> already used for BLK_STS_NEXUS that you're trying to differentiate from, right?
>> At least for nvme, this error code is returned when the host lacks sufficient
>> rights, so something like EACCESS might make sense.
>>
>> Looks good otherwise.
> 
> Welll ... BLK_STS_NEXUS _is_ the reservation error.

I was not sure of xen/virtio scsi uses of BLK_STS_NEXUS/DID_NEXUS_FAILURE.
The virtio spec's description for VIRTIO_SCSI_S_NEXUS_FAILURE:

    if the nexus is suffering a failure but retrying on other paths might
    yield a different result. 

looks like the description for DID_NEXUS_FAILURE in scsi_status.h.
To me the the description sounded generic where it could used for
other errors like the endpoint/port for the I_T is removed.

However, the qemu code only uses VIRTIO_SCSI_S_NEXUS_FAILURE for
reservation conflicts. If we are saying that is always the case in
other virt implementations, I don't even need this patch :) and we
can do what you requested and do more of a rename.