mbox series

[v3,00/19] Use block pr_ops in LIO

Message ID 20221026231945.6609-1-michael.christie@oracle.com (mailing list archive)
Headers show
Series Use block pr_ops in LIO | expand

Message

Mike Christie Oct. 26, 2022, 11:19 p.m. UTC 
The following patches were built over Linus's tree and this patchset

https://lore.kernel.org/all/20221023030403.33845-24-michael.christie@oracle.com/t/

which allows the SCSI layer passthrough users to control retries for
commands like PRs used in this patchset.

The patches in this thread allow us to use the block pr_ops with LIO's
target_core_iblock module to support cluster applications in VMs.
Currently, to use windows clustering or linux clustering (pacemaker +
cluster labs scsi fence agents) in VMs with LIO and vhost-scsi, you have
to use tcmu or pscsi or use a cluster aware FS/framework for the LIO pr
file. Setting up a cluster FS/framework is pain and waste when your real
backend device is already a distributed device, and pscsi and tcmu are
nice for specific use cases, but iblock gives you the best performance and
allows you to use stacked devices like dm-multipath. So these patches
allow iblock to work like pscsi/tcmu where they can pass a PR command to
the backend module. And then iblock will use the pr_ops to pass the PR
command to the real devices similar to what we do for unmap today.

The patches are separated in the following groups:

patches 1 - 11
- Add callouts to read a reservation and it's keys.

patches 12 - 15
- Have pr_ops return a blk_status_t.

patches 16 - 19
- Support for target_core_iblock to bypass the emulate PR code and call
the pr_ops.

This patchset has been tested with the libiscsi PGR ops and with window's
failover cluster verification test.

v3:
- Fix patch subject formatting.
- Fix coding style.
- Rearrange patches so helpers are added with users to avoid compilation
errors.
- Move pr type conversion to array and add nvme_pr_type.
- Add Extended Data Structure control flag enum and use in code for checks.
- Move nvme pr code to new file.
- Add more info to patch subjects about why we need to add blk_status
to pr_ops.
- Use generic SCSI passthrough error handling interface.
- Fix checkpatch --strict errors. Note that I kept the existing coding
style that it complained about because it looked like it was the preferred
style for the code and I didn't want a mix and match.

v2:
- Drop BLK_STS_NEXUS rename changes. Will do separately.
- Add NVMe support.
- Fixed bug in target_core_iblock where a variable was not initialized
mentioned by Christoph.
- Fixed sd pr_ops UA handling issue found when running libiscsi PGR tests.
- Added patches to allow pr_ops to pass up a BLK_STS so we could return
a RESERVATION_CONFLICT status when a pr_ops callout fails.




--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Comments

Keith Busch Oct. 27, 2022, 5:05 p.m. UTC | #1 
On Wed, Oct 26, 2022 at 06:19:33PM -0500, Mike Christie wrote:
> Reservation Report support needs to pass in a variable sized buffer, so
> this patch has the pr command helpers take a data length argument.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Keith Busch Oct. 28, 2022, 4:38 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 11:06:29AM -0500, Mike Christie wrote:
> On 10/27/22 12:06 PM, Keith Busch wrote:
> > On Wed, Oct 26, 2022 at 06:19:34PM -0500, Mike Christie wrote:
> >> This patch moves the pr code to it's own file because I'm going to be
> >> adding more functions and core.c is getting bigger.
> >>
> >> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> > 
> > Good idea.
> 
> Credit goes to Chaitanya.
> 
> One question for you. I wasn't sure about the copyright. I saw
> you added the pr_ops code in 2015 when you were at Intel. Do you
> want me to add:
> 
> Copyright (c) 2015, Intel Corporation.
> 
> to the new pr.c file?

I think I was just the last person to touch the code, but it was mostly
developed elsewhere. So "no", don't bother propagating the (c).

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mike Christie Oct. 30, 2022, 8:47 p.m. UTC | #3 
On 10/30/22 3:17 AM, Christoph Hellwig wrote:
> On Wed, Oct 26, 2022 at 06:19:35PM -0500, Mike Christie wrote:
>> This patch adds support for the pr_ops read_keys callout by calling the
>> NVMe Reservation Report helper, then parsing that info to get the
>> controller's registered keys. Because the callout is only used in the
>> kernel where the callers do not know about controller/host IDs, the
>> callout just returns the registered keys which is required by the SCSI PR
>> in READ KEYS command.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>  drivers/nvme/host/pr.c | 73 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/nvme.h   |  4 +++
>>  2 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
>> index aa88c55879b2..df7eb2440c67 100644
>> --- a/drivers/nvme/host/pr.c
>> +++ b/drivers/nvme/host/pr.c
>> @@ -118,10 +118,83 @@ static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type
>>  	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
>>  }
>>  
>> +static int nvme_pr_resv_report(struct block_device *bdev, u8 *data,
>> +		u32 data_len, bool *eds)
> 
> Is there any good reason this method seems to take a u8 instead of a void
> pointer?  As that seems to make a few things a bit messy.

I did it because the helper functions nvme_send_ns_head_pr_command/
nvme_send_ns_pr_command took a u8.

Looking at it some more I see those functions use nvme_submit_sync_cmd
which then takes a avoid pointer.

To handle your comment about the helper below I'll fix all that up to
take a void pointer.


> 
>> +	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);
> 
> Can you please add a hlper for this logic?

Will do.

> 
>> +	for (i = 0; i < num_ret_keys; i++) {
>> +		if (eds) {
>> +			keys_info->keys[i] =
>> +					le64_to_cpu(status->regctl_eds[i].rkey);
>> +		} else {
>> +			keys_info->keys[i] =
>> +					le64_to_cpu(status->regctl_ds[i].rkey);
>> +		}
> 
> If you shorten the status variable name to something like rs this becomes
> much easier to follow :)


Will do.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mike Christie Oct. 30, 2022, 11:05 p.m. UTC | #4 
On 10/30/22 3:20 AM, Christoph Hellwig wrote:
> On Wed, Oct 26, 2022 at 06:19:38PM -0500, Mike Christie wrote:
>> To handle both cases and keep userspace compatibility, this patch adds a
>> blk_status_t arg to the pr_ops callouts. The lower levels will convert
>> their device specific error to the blk_status_t then the upper levels
>> can easily check that code without knowing the device type. Adding the
>> extra return value will then allow us to not break userspace which expects
>> a negative -Exyz error code if the command fails before it's sent to the
>> device or a device/driver specific value if the error is > 0.
> 
> I really hate this double error return.  What -E* statuses that matter
> can be returned without a BLK_STS_* equivalent that we couldn't convert
> to and from?

Hey,

I didn't fully understand the question. The specific issue I'm hitting is
that if a scsi/nvme device returns SAM_STAT_RESERVATION_CONFLICT/
NVME_SC_RESERVATION_CONFLICT then in lio I need to be able to detect that
and return SAM_STAT_RESERVATION_CONFLICT. So I only care about
-EBADE/BLK_STS_NEXUS right now. So are you asking about -EBADE?

Do you mean I could have sd_pr_out_command return -EBADE when it gets
a SAM_STAT_RESERVATION_CONFLICT (nvme would do the equivalent). Then in
lio do:

ret = ops->pr_register()
if (ret == -EBADE)
	return SAM_STAT_RESERVATION_CONFLICT;

The problem I hit is that in the ioctl code I then have to do:

@@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
 
 	if (reg.flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+	ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+	if (ret == -EBADE) {
+		if (bdev_is_nvme(bdev))
+			ret = NVME_SC_RESERVATION_CONFLICT;
+		else if (bdev_is_scsi(bdev)
+			ret = SAM_STAT_RESERVATION_CONFLICT;
+	}
+	return ret;
 }
 

or I could convert the scsi/nvme or code to always use BLK_STS errors.
In LIO I can easily check for BLK_STS_NEXUS like with the -EBADE example. In
the ioctl code then for common errors I can go from BLK_STS using the
blk_status_to_errno helper. However, for some scsi/nvme specific errors we
would want to do:

@@ -269,7 +270,36 @@ static int blkdev_pr_register(struct block_device *bdev,
 
 	if (reg.flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+	ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+	switch (ret) {
+	/* there could be nvme/scsi helper functions for this which would
+	 * be the reverse of nvme_error_status/ */
+	case BLK_STS_NEXUS:
+		if (bdev_is_nvme(bdev))
+			ret = NVME_SC_RESERVATION_CONFLICT;
+		else if (bdev_is_scsi(bdev)
+			ret = SAM_STAT_RESERVATION_CONFLICT;
+		break;
+	case BLK_STS_TRANSPORT:
+		if (bdev_is_nvme(bdev))
+			ret = NVME_SC_HOST_PATH_ERROR;
+		else if (bdev_is_scsi(bdev)
+			ret = DID_TRANSPORT_FAILFAST or DID_TRANSPORT_MARGINAL;
+		break;
+	case BLK_STS_NOTSUPP:
+		if (bdev_is_nvme(bdev))
+			ret = NVME_SC_BAD_ATTRIBUTES or
+				NVME_SC_ONCS_NOT_SUPPORTED or
+				NVME_SC_INVALID_OPCODE or
+				NVME_SC_INVALID_FIELD or
+				NVME_SC_INVALID_NS
+		else if (bdev_is_scsi(bdev)
+			ret = We don't have a way to support this in SCSI yet
				because it would be in the sense/asc/ascq.
+		break;
+	default:
+		ret = blk_status_to_errno(ret);
+	}
+	return ret;
 }
 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig Nov. 1, 2022, 10:15 a.m. UTC | #5 
On Sun, Oct 30, 2022 at 06:05:35PM -0500, Mike Christie wrote:
> The problem I hit is that in the ioctl code I then have to do:
> 
> @@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
>  
>  	if (reg.flags & ~PR_FL_IGNORE_KEY)
>  		return -EOPNOTSUPP;
> -	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> +	ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> +	if (ret == -EBADE) {
> +		if (bdev_is_nvme(bdev))
> +			ret = NVME_SC_RESERVATION_CONFLICT;
> +		else if (bdev_is_scsi(bdev)
> +			ret = SAM_STAT_RESERVATION_CONFLICT;
> +	}
> +	return ret;

Eww.  We should have never leaked protocol specific values out to
userspace.  This is an original bug I introduceѕ, so all blame is on me.

I suspect the right way to fix is is to keep the numeric value of
SAM_STAT_RESERVATION_CONFLICT and give it a new constant exposed in
the uapi header, assuming that SCSI is the thing people actually
used the PR API for, and nvme was just an nice little add-on.

Now if an errno value or blk_status_t is returned from the method
should not matter for this fix, but in the long run I think the
blk_status_t would be cleaner than the int used for errno, and
that will also prevent us from returning accidental non-intended
values.

Btw, I also thing we should rename BLK_STS_NEXUS to
BLK_STS_RESERVATION_CONFLICT (assuming s390 is ok with that), as that
has much better documentary value.

> +	case BLK_STS_TRANSPORT:
> +		if (bdev_is_nvme(bdev))
> +			ret = NVME_SC_HOST_PATH_ERROR;
> +		else if (bdev_is_scsi(bdev)
> +			ret = DID_TRANSPORT_FAILFAST or DID_TRANSPORT_MARGINAL;
> +		break;

And we'll need an uapi value for this as well.

> +	case BLK_STS_NOTSUPP:

and maybe this unless we can just get away with the negative errno
value.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mike Christie Nov. 5, 2022, 6:36 p.m. UTC | #6 
On 11/1/22 5:15 AM, Christoph Hellwig wrote:
> On Sun, Oct 30, 2022 at 06:05:35PM -0500, Mike Christie wrote:
>> The problem I hit is that in the ioctl code I then have to do:
>>
>> @@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
>>  
>>  	if (reg.flags & ~PR_FL_IGNORE_KEY)
>>  		return -EOPNOTSUPP;
>> -	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
>> +	ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
>> +	if (ret == -EBADE) {
>> +		if (bdev_is_nvme(bdev))
>> +			ret = NVME_SC_RESERVATION_CONFLICT;
>> +		else if (bdev_is_scsi(bdev)
>> +			ret = SAM_STAT_RESERVATION_CONFLICT;
>> +	}
>> +	return ret;
> 
> Eww.  We should have never leaked protocol specific values out to
> userspace.  This is an original bug I introduceѕ, so all blame is on me.
> 
> I suspect the right way to fix is is to keep the numeric value of
> SAM_STAT_RESERVATION_CONFLICT and give it a new constant exposed in
> the uapi header, assuming that SCSI is the thing people actually
> used the PR API for, and nvme was just an nice little add-on.
> 

Do you mean just doing this:

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index 727123c611e6..6ac70514170d 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -72,12 +72,17 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c,
 static int nvme_send_pr_command(struct block_device *bdev,
 		struct nvme_command *c, void *data, unsigned int data_len)
 {
+	int ret;
+
 	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
 	    bdev->bd_disk->fops == &nvme_ns_head_ops)
-		return nvme_send_ns_head_pr_command(bdev, c, data, data_len);
+		ret = nvme_send_ns_head_pr_command(bdev, c, data, data_len);
 	else
-		return nvme_send_ns_pr_command(bdev->bd_disk->private_data, c,
-					       data, data_len);
+		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, c,
+					      data, data_len);
+	if (ret == NVME_SC_RESERVATION_CONFLICT)
+		ret = PR_STS_RESERVATION_CONFLICT;
+	return ret;
 }
 
 static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c0a614efcfce..c7621d8ffa51 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1840,7 +1840,8 @@ static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 	    scsi_sense_valid(&sshdr)) {
 		sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
 		scsi_print_sense_hdr(sdev, NULL, &sshdr);
-	}
+	} else if (__get_status_byte(result) == SAM_STAT_RESERVATION_CONFLICT)
+		result = PR_STS_RESERVATION_CONFLICT;
 
 	return result;
 }
diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..7a637f9e5b49 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -13,6 +13,15 @@ enum pr_type {
 	PR_EXCLUSIVE_ACCESS_ALL_REGS	= 6,
 };
 
+enum pr_status {
+	PR_STS_SUCCESS			= 0x0,
+	/*
+	 * The error codes are based on SCSI, because it was the primary user
+	 * and had userspace users.
+	 */
+	PR_STS_RESERVATION_CONFLICT	= 0x18,
+};
+
 struct pr_reservation {
 	__u64	key;
 	__u32	type;


---------------------------------------------------------------------------


Or we could add a new flag and make it nicer for the user in the future,
but also uglier for the drivers but a little less uglier than adding the
blk_sts field to the calls:



diff --git a/block/ioctl.c b/block/ioctl.c
index 60121e89052b..cae58f57ea13 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -269,7 +269,8 @@ static int blkdev_pr_register(struct block_device *bdev,
 
 	if (reg.flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+	return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags,
+				reg.flags & PR_FL_PR_STAT);
 }
 
 static int blkdev_pr_reserve(struct block_device *bdev,
@@ -287,7 +288,8 @@ static int blkdev_pr_reserve(struct block_device *bdev,
 
 	if (rsv.flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return ops->pr_reserve(bdev, rsv.key, rsv.type, rsv.flags);
+	return ops->pr_reserve(bdev, rsv.key, rsv.type, rsv.flags,
+			       rsv.flags & PR_FL_PR_STAT)
 }
 
 static int blkdev_pr_release(struct block_device *bdev,
@@ -305,7 +307,8 @@ static int blkdev_pr_release(struct block_device *bdev,
 
 	if (rsv.flags)
 		return -EOPNOTSUPP;
-	return ops->pr_release(bdev, rsv.key, rsv.type);
+	return ops->pr_release(bdev, rsv.key, rsv.type,
+			       rsv.flags & PR_FL_PR_STAT);
 }
 
 static int blkdev_pr_preempt(struct block_device *bdev,
@@ -323,7 +326,8 @@ static int blkdev_pr_preempt(struct block_device *bdev,
 
 	if (p.flags)
 		return -EOPNOTSUPP;
-	return ops->pr_preempt(bdev, p.old_key, p.new_key, p.type, abort);
+	return ops->pr_preempt(bdev, p.old_key, p.new_key, p.type, abort,
+			       p.flags & PR_FL_PR_STAT)
 }
 
 static int blkdev_pr_clear(struct block_device *bdev,
@@ -341,7 +345,7 @@ static int blkdev_pr_clear(struct block_device *bdev,
 
 	if (c.flags)
 		return -EOPNOTSUPP;
-	return ops->pr_clear(bdev, c.key);
+	return ops->pr_clear(bdev, c.key, c.flags & PR_FL_PR_STAT);
 }
 
 static int blkdev_flushbuf(struct block_device *bdev, fmode_t mode,
diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index 727123c611e6..7c317b646cde 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -70,18 +70,44 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c,
 }
 
 static int nvme_send_pr_command(struct block_device *bdev,
-		struct nvme_command *c, void *data, unsigned int data_len)
+		struct nvme_command *c, void *data, unsigned int data_len,
+		bool use_pr_sts)
 {
+	int ret;
+
 	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
 	    bdev->bd_disk->fops == &nvme_ns_head_ops)
-		return nvme_send_ns_head_pr_command(bdev, c, data, data_len);
+		ret = nvme_send_ns_head_pr_command(bdev, c, data, data_len);
 	else
-		return nvme_send_ns_pr_command(bdev->bd_disk->private_data, c,
-					       data, data_len);
+		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, c,
+					      data, data_len);
+	if (!ret || ret < 0 || (ret > 0 && !use_pr_sts))
+		return ret;
+
+	switch (ret) {
+	case NVME_SC_RESERVATION_CONFLICT:
+		ret = PR_STS_RESERVATION_CONFLICT;
+		break;
+	case NVME_SC_HOST_PATH_ERROR:
+		ret = PR_STS_PATH_FAILURE;
+		break
+	case NVME_SC_BAD_ATTRIBUTES:
+	case NVME_SC_ONCS_NOT_SUPPORTED:
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
+		ret = PR_STS_INVALID_OP;
+		break;
+	default:
+		ret = PR_STS_IOERR;
+		break;
+	}
+
+	return ret;
 }
 
 static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
-				u64 key, u64 sa_key, u8 op)
+				u64 key, u64 sa_key, u8 op, bool use_pr_sts)
 {
 	struct nvme_command c = { };
 	u8 data[16] = { 0, };
@@ -92,11 +118,11 @@ static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
 	c.common.opcode = op;
 	c.common.cdw10 = cpu_to_le32(cdw10);
 
-	return nvme_send_pr_command(bdev, &c, data, sizeof(data));
+	return nvme_send_pr_command(bdev, &c, data, sizeof(data), use_pr_sts);
 }
 
 static int nvme_pr_register(struct block_device *bdev, u64 old,
-		u64 new, unsigned flags)
+		u64 new, unsigned flags, bool use_pr_sts)
 {
 	u32 cdw10;
 
@@ -106,11 +132,12 @@ static int nvme_pr_register(struct block_device *bdev, u64 old,
 	cdw10 = old ? 2 : 0;
 	cdw10 |= (flags & PR_FL_IGNORE_KEY) ? 1 << 3 : 0;
 	cdw10 |= (1 << 30) | (1 << 31); /* PTPL=1 */
-	return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_register);
+	return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_register,
+			       use_pr_sts);
 }
 
 static int nvme_pr_reserve(struct block_device *bdev, u64 key,
-		enum pr_type type, unsigned flags)
+		enum pr_type type, unsigned flags, bool use_pr_sts)
 {
 	u32 cdw10;
 
@@ -119,29 +146,34 @@ static int nvme_pr_reserve(struct block_device *bdev, u64 key,
 
 	cdw10 = nvme_pr_type_from_blk(type) << 8;
 	cdw10 |= ((flags & PR_FL_IGNORE_KEY) ? 1 << 3 : 0);
-	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_acquire);
+	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_acquire,
+			       use_pr_sts);
 }
 
 static int nvme_pr_preempt(struct block_device *bdev, u64 old, u64 new,
-		enum pr_type type, bool abort)
+		enum pr_type type, bool abort, bool use_pr_sts)
 {
 	u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (abort ? 2 : 1);
 
-	return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_acquire);
+	return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_acquire,
+			       use_pr_sts);
 }
 
-static int nvme_pr_clear(struct block_device *bdev, u64 key)
+static int nvme_pr_clear(struct block_device *bdev, u64 key, bool use_pr_sts)
 {
 	u32 cdw10 = 1 | (key ? 0 : 1 << 3);
 
-	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
+	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release,
+			       use_pr_sts);
 }
 
-static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
+static int nvme_pr_release(struct block_device *bdev, u64 key,
+		enum pr_type type, bool use_pr_sts)
 {
 	u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (key ? 0 : 1 << 3);
 
-	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
+	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release,
+			       use_pr_sts);
 }
 
 static int nvme_pr_resv_report(struct block_device *bdev, void *data,
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c0a614efcfce..46393bdd7427 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1797,7 +1797,8 @@ static int sd_pr_read_reservation(struct block_device *bdev,
 }
 
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
-		u64 key, u64 sa_key, enum scsi_pr_type type, u8 flags)
+		u64 key, u64 sa_key, enum scsi_pr_type type, u8 flags,
+		bool use_pr_sts)
 {
 	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
 	struct scsi_device *sdev = sdkp->device;
@@ -1842,44 +1843,74 @@ static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		scsi_print_sense_hdr(sdev, NULL, &sshdr);
 	}
 
+	if (!result || result < 0 || (result > 0 && !use_pr_sts))
+		return result;
+
+	result = PR_STS_IOERR;
+
+	switch host_byte(result) {
+	case DID_TRANSPORT_FAILFAST:
+	case DID_TRANSPORT_MARGINAL:
+	case DID_TRANSPORT_DISRUPTED:
+		result = PR_STS_PATH_FAILURE;
+		goto done;
+	}
+
+	switch (__get_status_byte(result)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		result = PR_STS_RESERVATION_CONFLICT;
+		goto done;
+	case SAM_STAT_CHECK_CONDITION:
+		if (!scsi_sense_valid(&sshdr))
+			goto done;
+
+		if (sshdr.sense_key == ILLEGAL_REQUEST &&
+		    (sshdr.asc == 0x26 || sshdr.asc == 0x24)) {
+			result = PR_STS_INVALID_OP;
+			goto done;
+		}
+	}
+
+done:
 	return result;
 }
 
 static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key,
-		u32 flags)
+		u32 flags, bool use_pr_sts)
 {
 	if (flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
 	return sd_pr_out_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00,
-			old_key, new_key, 0,
-			(1 << 0) /* APTPL */);
+				 old_key, new_key, 0, (1 << 0) /* APTPL */,
+				 use_pr_sts);
 }
 
 static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
-		u32 flags)
+		u32 flags, bool use_pr_sts)
 {
 	if (flags)
 		return -EOPNOTSUPP;
 	return sd_pr_out_command(bdev, 0x01, key, 0,
-				 block_pr_type_to_scsi(type), 0);
+				 block_pr_type_to_scsi(type), 0, use_pr_sts)
 }
 
-static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
+static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type,
+			 bool use_pr_sts)
 {
 	return sd_pr_out_command(bdev, 0x02, key, 0,
-				 block_pr_type_to_scsi(type), 0);
+				 block_pr_type_to_scsi(type), 0, use_pr_sts);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
-		enum pr_type type, bool abort)
+		enum pr_type type, bool abort, bool use_pr_sts)
 {
 	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
-				 block_pr_type_to_scsi(type), 0);
+				 block_pr_type_to_scsi(type), 0, use_pr_sts);
 }
 
-static int sd_pr_clear(struct block_device *bdev, u64 key)
+static int sd_pr_clear(struct block_device *bdev, u64 key, bool use_pr_sts)
 {
-	return sd_pr_out_command(bdev, 0x03, key, 0, 0, 0);
+	return sd_pr_out_command(bdev, 0x03, key, 0, 0, 0, use_pr_sts);
 }
 
 static const struct pr_ops sd_pr_ops = {
diff --git a/include/linux/pr.h b/include/linux/pr.h
index 3003daec28a5..51e03e73a87f 100644
--- a/include/linux/pr.h
+++ b/include/linux/pr.h
@@ -18,14 +18,14 @@ struct pr_held_reservation {
 
 struct pr_ops {
 	int (*pr_register)(struct block_device *bdev, u64 old_key, u64 new_key,
-			u32 flags);
+			u32 flags, bool use_pr_sts);
 	int (*pr_reserve)(struct block_device *bdev, u64 key,
-			enum pr_type type, u32 flags);
+			enum pr_type type, u32 flags, bool use_pr_sts);
 	int (*pr_release)(struct block_device *bdev, u64 key,
-			enum pr_type type);
+			enum pr_type type, bool use_pr_sts);
 	int (*pr_preempt)(struct block_device *bdev, u64 old_key, u64 new_key,
-			enum pr_type type, bool abort);
-	int (*pr_clear)(struct block_device *bdev, u64 key);
+			enum pr_type type, bool abort, bool use_pr_sts);
+	int (*pr_clear)(struct block_device *bdev, u64 key, bool use_pr_sts);
 	/*
 	 * pr_read_keys - Read the registered keys and return them in the
 	 * pr_keys->keys array. The keys array will have been allocated at the
diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..ac8f7fc404ff 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -13,6 +13,14 @@ enum pr_type {
 	PR_EXCLUSIVE_ACCESS_ALL_REGS	= 6,
 };
 
+enum pr_status {
+	PR_STS_SUCCESS,
+	PR_STS_IOERR,
+	PR_STS_RESERVATION_CONFLICT,
+	PR_STS_PATH_FAILURE,
+	PR_STS_INVALID_OP,
+};
+
 struct pr_reservation {
 	__u64	key;
 	__u32	type;
@@ -40,6 +48,7 @@ struct pr_clear {
 };
 
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
+#define PR_FL_PR_STAT		(1 << 1)	/* Convert device errors to pr_status */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
 #define IOC_PR_RESERVE		_IOW('p', 201, struct pr_reservation)


Bart, if this is what you were suggesting I misunderstood you. I thought you wanted to
only pass the new code in the kernel so that's why I was saying we still have the problem
of converting the error back when passing it back to userspace.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig Nov. 7, 2022, 9:16 a.m. UTC | #7 
On Sat, Nov 05, 2022 at 01:36:18PM -0500, Mike Christie wrote:
> Do you mean just doing this:

That would be the minimal fix.  We'd then still need to enumerate
the allowed positive return values and check noting else is returned.

I don't like the opt in in the other version.  The SCSI return values are
the defactor API, and we need to switch NVMe to align with it ASAP
instead of keeping the broken old version around.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel