[PATCH 0/8] Use block pr_ops in LIO

[PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

The following patches were built over Linus's tree. They allow us to use
the block pr_ops with LIO's target_core_iblock module to support cluster
applications in VMs.

Currently, to use something like windows clustering 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.

Note that this is patchset does not attempt to support every PR SCSI
feature in iblock. It has the same limitations as tcmu and pscsi where
you can have a single I_T nexus per device and only supports what is
needed for windows clustering right now.

[dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

The following patches were built over Linus's tree. They allow us to use
the block pr_ops with LIO's target_core_iblock module to support cluster
applications in VMs.

Currently, to use something like windows clustering 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.

Note that this is patchset does not attempt to support every PR SCSI
feature in iblock. It has the same limitations as tcmu and pscsi where
you can have a single I_T nexus per device and only supports what is
needed for windows clustering right now.



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

[PATCH 01/11] scsi: target: Rename sbc_ops to exec_cmd_ops

[Mike Christie]

The next patches allow us to call the block layer's pr_ops from the
backends. This will require allowing the backends to hook into the cmd
processing for SPC commands, so this renames sbc_ops to a more generic
exec_cmd_ops.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_file.c    |  4 ++--
 drivers/target/target_core_iblock.c  |  4 ++--
 drivers/target/target_core_rd.c      |  4 ++--
 drivers/target/target_core_sbc.c     | 13 +++++++------
 include/target/target_core_backend.h |  4 ++--
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 8190b840065f..4f00563c5016 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -900,7 +900,7 @@ static void fd_free_prot(struct se_device *dev)
 	fd_dev->fd_prot_file = NULL;
 }
 
-static struct sbc_ops fd_sbc_ops = {
+static struct exec_cmd_ops fd_exec_cmd_ops = {
 	.execute_rw		= fd_execute_rw,
 	.execute_sync_cache	= fd_execute_sync_cache,
 	.execute_write_same	= fd_execute_write_same,
@@ -910,7 +910,7 @@ static struct sbc_ops fd_sbc_ops = {
 static sense_reason_t
 fd_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &fd_sbc_ops);
+	return sbc_parse_cdb(cmd, &fd_exec_cmd_ops);
 }
 
 static const struct target_backend_ops fileio_ops = {
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 87ede165ddba..9b0d788f6744 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -871,7 +871,7 @@ static unsigned int iblock_get_io_opt(struct se_device *dev)
 	return bdev_io_opt(bd);
 }
 
-static struct sbc_ops iblock_sbc_ops = {
+static struct exec_cmd_ops iblock_exec_cmd_ops = {
 	.execute_rw		= iblock_execute_rw,
 	.execute_sync_cache	= iblock_execute_sync_cache,
 	.execute_write_same	= iblock_execute_write_same,
@@ -881,7 +881,7 @@ static struct sbc_ops iblock_sbc_ops = {
 static sense_reason_t
 iblock_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &iblock_sbc_ops);
+	return sbc_parse_cdb(cmd, &iblock_exec_cmd_ops);
 }
 
 static bool iblock_get_write_cache(struct se_device *dev)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 6648c1c90e19..6f67cc09c2b5 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -643,14 +643,14 @@ static void rd_free_prot(struct se_device *dev)
 	rd_release_prot_space(rd_dev);
 }
 
-static struct sbc_ops rd_sbc_ops = {
+static struct exec_cmd_ops rd_exec_cmd_ops = {
 	.execute_rw		= rd_execute_rw,
 };
 
 static sense_reason_t
 rd_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &rd_sbc_ops);
+	return sbc_parse_cdb(cmd, &rd_exec_cmd_ops);
 }
 
 static const struct target_backend_ops rd_mcp_ops = {
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index ca1b2312d6e7..e08d06dacbfc 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -192,7 +192,7 @@ EXPORT_SYMBOL(sbc_get_write_same_sectors);
 static sense_reason_t
 sbc_execute_write_same_unmap(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	sector_t nolb = sbc_get_write_same_sectors(cmd);
 	sense_reason_t ret;
 
@@ -279,7 +279,8 @@ static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
 }
 
 static sense_reason_t
-sbc_setup_write_same(struct se_cmd *cmd, unsigned char flags, struct sbc_ops *ops)
+sbc_setup_write_same(struct se_cmd *cmd, unsigned char flags,
+		     struct exec_cmd_ops *ops)
 {
 	struct se_device *dev = cmd->se_dev;
 	sector_t end_lba = dev->transport->get_blocks(dev) + 1;
@@ -404,7 +405,7 @@ static sense_reason_t xdreadwrite_callback(struct se_cmd *cmd, bool success,
 static sense_reason_t
 sbc_execute_rw(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 
 	return ops->execute_rw(cmd, cmd->t_data_sg, cmd->t_data_nents,
 			       cmd->data_direction);
@@ -620,7 +621,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 static sense_reason_t
 sbc_compare_and_write(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 	int rc;
@@ -818,7 +819,7 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
 }
 
 sense_reason_t
-sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
+sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
 {
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *cdb = cmd->t_task_cdb;
@@ -1167,7 +1168,7 @@ EXPORT_SYMBOL(sbc_get_device_type);
 static sense_reason_t
 sbc_execute_unmap(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *buf, *ptr = NULL;
 	sector_t lba;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 675f3a1fe613..4e6cf1bf3b87 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -61,7 +61,7 @@ struct target_backend_ops {
 	struct configfs_attribute **tb_dev_action_attrs;
 };
 
-struct sbc_ops {
+struct exec_cmd_ops {
 	sense_reason_t (*execute_rw)(struct se_cmd *cmd, struct scatterlist *,
 				     u32, enum dma_data_direction);
 	sense_reason_t (*execute_sync_cache)(struct se_cmd *cmd);
@@ -85,7 +85,7 @@ sense_reason_t	spc_emulate_report_luns(struct se_cmd *cmd);
 sense_reason_t	spc_emulate_inquiry_std(struct se_cmd *, unsigned char *);
 sense_reason_t	spc_emulate_evpd_83(struct se_cmd *, unsigned char *);
 
-sense_reason_t	sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops);
+sense_reason_t	sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops);
 u32	sbc_get_device_rev(struct se_device *dev);
 u32	sbc_get_device_type(struct se_device *dev);
 sector_t	sbc_get_write_same_sectors(struct se_cmd *cmd);
-- 
2.25.1

[dm-devel] [PATCH 01/11] scsi: target: Rename sbc_ops to exec_cmd_ops

[Mike Christie]

The next patches allow us to call the block layer's pr_ops from the
backends. This will require allowing the backends to hook into the cmd
processing for SPC commands, so this renames sbc_ops to a more generic
exec_cmd_ops.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_file.c    |  4 ++--
 drivers/target/target_core_iblock.c  |  4 ++--
 drivers/target/target_core_rd.c      |  4 ++--
 drivers/target/target_core_sbc.c     | 13 +++++++------
 include/target/target_core_backend.h |  4 ++--
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 8190b840065f..4f00563c5016 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -900,7 +900,7 @@ static void fd_free_prot(struct se_device *dev)
 	fd_dev->fd_prot_file = NULL;
 }
 
-static struct sbc_ops fd_sbc_ops = {
+static struct exec_cmd_ops fd_exec_cmd_ops = {
 	.execute_rw		= fd_execute_rw,
 	.execute_sync_cache	= fd_execute_sync_cache,
 	.execute_write_same	= fd_execute_write_same,
@@ -910,7 +910,7 @@ static struct sbc_ops fd_sbc_ops = {
 static sense_reason_t
 fd_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &fd_sbc_ops);
+	return sbc_parse_cdb(cmd, &fd_exec_cmd_ops);
 }
 
 static const struct target_backend_ops fileio_ops = {
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 87ede165ddba..9b0d788f6744 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -871,7 +871,7 @@ static unsigned int iblock_get_io_opt(struct se_device *dev)
 	return bdev_io_opt(bd);
 }
 
-static struct sbc_ops iblock_sbc_ops = {
+static struct exec_cmd_ops iblock_exec_cmd_ops = {
 	.execute_rw		= iblock_execute_rw,
 	.execute_sync_cache	= iblock_execute_sync_cache,
 	.execute_write_same	= iblock_execute_write_same,
@@ -881,7 +881,7 @@ static struct sbc_ops iblock_sbc_ops = {
 static sense_reason_t
 iblock_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &iblock_sbc_ops);
+	return sbc_parse_cdb(cmd, &iblock_exec_cmd_ops);
 }
 
 static bool iblock_get_write_cache(struct se_device *dev)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 6648c1c90e19..6f67cc09c2b5 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -643,14 +643,14 @@ static void rd_free_prot(struct se_device *dev)
 	rd_release_prot_space(rd_dev);
 }
 
-static struct sbc_ops rd_sbc_ops = {
+static struct exec_cmd_ops rd_exec_cmd_ops = {
 	.execute_rw		= rd_execute_rw,
 };
 
 static sense_reason_t
 rd_parse_cdb(struct se_cmd *cmd)
 {
-	return sbc_parse_cdb(cmd, &rd_sbc_ops);
+	return sbc_parse_cdb(cmd, &rd_exec_cmd_ops);
 }
 
 static const struct target_backend_ops rd_mcp_ops = {
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index ca1b2312d6e7..e08d06dacbfc 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -192,7 +192,7 @@ EXPORT_SYMBOL(sbc_get_write_same_sectors);
 static sense_reason_t
 sbc_execute_write_same_unmap(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	sector_t nolb = sbc_get_write_same_sectors(cmd);
 	sense_reason_t ret;
 
@@ -279,7 +279,8 @@ static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
 }
 
 static sense_reason_t
-sbc_setup_write_same(struct se_cmd *cmd, unsigned char flags, struct sbc_ops *ops)
+sbc_setup_write_same(struct se_cmd *cmd, unsigned char flags,
+		     struct exec_cmd_ops *ops)
 {
 	struct se_device *dev = cmd->se_dev;
 	sector_t end_lba = dev->transport->get_blocks(dev) + 1;
@@ -404,7 +405,7 @@ static sense_reason_t xdreadwrite_callback(struct se_cmd *cmd, bool success,
 static sense_reason_t
 sbc_execute_rw(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 
 	return ops->execute_rw(cmd, cmd->t_data_sg, cmd->t_data_nents,
 			       cmd->data_direction);
@@ -620,7 +621,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 static sense_reason_t
 sbc_compare_and_write(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 	int rc;
@@ -818,7 +819,7 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
 }
 
 sense_reason_t
-sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
+sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
 {
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *cdb = cmd->t_task_cdb;
@@ -1167,7 +1168,7 @@ EXPORT_SYMBOL(sbc_get_device_type);
 static sense_reason_t
 sbc_execute_unmap(struct se_cmd *cmd)
 {
-	struct sbc_ops *ops = cmd->protocol_data;
+	struct exec_cmd_ops *ops = cmd->protocol_data;
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *buf, *ptr = NULL;
 	sector_t lba;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 675f3a1fe613..4e6cf1bf3b87 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -61,7 +61,7 @@ struct target_backend_ops {
 	struct configfs_attribute **tb_dev_action_attrs;
 };
 
-struct sbc_ops {
+struct exec_cmd_ops {
 	sense_reason_t (*execute_rw)(struct se_cmd *cmd, struct scatterlist *,
 				     u32, enum dma_data_direction);
 	sense_reason_t (*execute_sync_cache)(struct se_cmd *cmd);
@@ -85,7 +85,7 @@ sense_reason_t	spc_emulate_report_luns(struct se_cmd *cmd);
 sense_reason_t	spc_emulate_inquiry_std(struct se_cmd *, unsigned char *);
 sense_reason_t	spc_emulate_evpd_83(struct se_cmd *, unsigned char *);
 
-sense_reason_t	sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops);
+sense_reason_t	sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops);
 u32	sbc_get_device_rev(struct se_device *dev);
 u32	sbc_get_device_type(struct se_device *dev);
 sector_t	sbc_get_write_same_sectors(struct se_cmd *cmd);
-- 
2.25.1

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

Re: [PATCH 01/11] scsi: target: Rename sbc_ops to exec_cmd_ops

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 01/11] scsi: target: Rename sbc_ops to exec_cmd_ops

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 02/11] scsi: Rename sd_pr_command.

[Mike Christie]

Rename sd_pr_command to sd_pr_out_command to match a
sd_pr_in_command helper added in the next patches.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dc6e55761fd1..24e61647064c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1703,7 +1703,7 @@ static char sd_pr_type(enum pr_type type)
 	}
 };
 
-static int sd_pr_command(struct block_device *bdev, u8 sa,
+static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
 	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
@@ -1739,7 +1739,7 @@ static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key,
 {
 	if (flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return sd_pr_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00,
+	return sd_pr_out_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00,
 			old_key, new_key, 0,
 			(1 << 0) /* APTPL */);
 }
@@ -1749,24 +1749,24 @@ static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
 {
 	if (flags)
 		return -EOPNOTSUPP;
-	return sd_pr_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
 }
 
 static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	return sd_pr_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
 		enum pr_type type, bool abort)
 {
-	return sd_pr_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
+	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
 			     sd_pr_type(type), 0);
 }
 
 static int sd_pr_clear(struct block_device *bdev, u64 key)
 {
-	return sd_pr_command(bdev, 0x03, key, 0, 0, 0);
+	return sd_pr_out_command(bdev, 0x03, key, 0, 0, 0);
 }
 
 static const struct pr_ops sd_pr_ops = {
-- 
2.25.1

[dm-devel] [PATCH 02/11] scsi: Rename sd_pr_command.

[Mike Christie]

Rename sd_pr_command to sd_pr_out_command to match a
sd_pr_in_command helper added in the next patches.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dc6e55761fd1..24e61647064c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1703,7 +1703,7 @@ static char sd_pr_type(enum pr_type type)
 	}
 };
 
-static int sd_pr_command(struct block_device *bdev, u8 sa,
+static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
 	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
@@ -1739,7 +1739,7 @@ static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key,
 {
 	if (flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
-	return sd_pr_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00,
+	return sd_pr_out_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00,
 			old_key, new_key, 0,
 			(1 << 0) /* APTPL */);
 }
@@ -1749,24 +1749,24 @@ static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
 {
 	if (flags)
 		return -EOPNOTSUPP;
-	return sd_pr_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
 }
 
 static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	return sd_pr_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
 		enum pr_type type, bool abort)
 {
-	return sd_pr_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
+	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
 			     sd_pr_type(type), 0);
 }
 
 static int sd_pr_clear(struct block_device *bdev, u64 key)
 {
-	return sd_pr_command(bdev, 0x03, key, 0, 0, 0);
+	return sd_pr_out_command(bdev, 0x03, key, 0, 0, 0);
 }
 
 static const struct pr_ops sd_pr_ops = {
-- 
2.25.1

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

Re: [PATCH 02/11] scsi: Rename sd_pr_command.

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 02/11] scsi: Rename sd_pr_command.

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Mike Christie]

LIO is going to want to do the same block to/from SCSI pr types as sd.c
so this moves the sd_pr_type helper to a new file and adds a helper to
go from the SCSI value to the block one.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c            | 29 +++++-----------------
 include/scsi/scsi_block_pr.h | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 23 deletions(-)
 create mode 100644 include/scsi/scsi_block_pr.h

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 24e61647064c..765fca0f38c7 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -67,6 +67,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_block_pr.h>
 
 #include "sd.h"
 #include "scsi_priv.h"
@@ -1683,26 +1684,6 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
 	return ret;
 }
 
-static char sd_pr_type(enum pr_type type)
-{
-	switch (type) {
-	case PR_WRITE_EXCLUSIVE:
-		return 0x01;
-	case PR_EXCLUSIVE_ACCESS:
-		return 0x03;
-	case PR_WRITE_EXCLUSIVE_REG_ONLY:
-		return 0x05;
-	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
-		return 0x06;
-	case PR_WRITE_EXCLUSIVE_ALL_REGS:
-		return 0x07;
-	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
-		return 0x08;
-	default:
-		return 0;
-	}
-};
-
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1749,19 +1730,21 @@ static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
 {
 	if (flags)
 		return -EOPNOTSUPP;
-	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x01, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x02, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
 		enum pr_type type, bool abort)
 {
 	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
-			     sd_pr_type(type), 0);
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_clear(struct block_device *bdev, u64 key)
diff --git a/include/scsi/scsi_block_pr.h b/include/scsi/scsi_block_pr.h
new file mode 100644
index 000000000000..7d137c067331
--- /dev/null
+++ b/include/scsi/scsi_block_pr.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _SCSI_BLOCK_PR_H
+#define _SCSI_BLOCK_PR_H
+
+#include <uapi/linux/pr.h>
+
+static inline char block_pr_type_to_scsi(enum pr_type type)
+{
+	switch (type) {
+	case PR_WRITE_EXCLUSIVE:
+		return 0x01;
+	case PR_EXCLUSIVE_ACCESS:
+		return 0x03;
+	case PR_WRITE_EXCLUSIVE_REG_ONLY:
+		return 0x05;
+	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
+		return 0x06;
+	case PR_WRITE_EXCLUSIVE_ALL_REGS:
+		return 0x07;
+	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
+		return 0x08;
+	default:
+		return 0;
+	}
+};
+
+static inline enum pr_type scsi_pr_type_to_block(char type)
+{
+	switch (type) {
+	case 0x01:
+		return PR_WRITE_EXCLUSIVE;
+	case 0x03:
+		return PR_EXCLUSIVE_ACCESS;
+	case 0x05:
+		return PR_WRITE_EXCLUSIVE_REG_ONLY;
+	case 0x06:
+		return PR_EXCLUSIVE_ACCESS_REG_ONLY;
+	case 0x07:
+		return PR_WRITE_EXCLUSIVE_ALL_REGS;
+	case 0x08:
+		return PR_EXCLUSIVE_ACCESS_ALL_REGS;
+	default:
+		return 0;
+	}
+}
+
+#endif
-- 
2.25.1

[dm-devel] [PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Mike Christie]

LIO is going to want to do the same block to/from SCSI pr types as sd.c
so this moves the sd_pr_type helper to a new file and adds a helper to
go from the SCSI value to the block one.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c            | 29 +++++-----------------
 include/scsi/scsi_block_pr.h | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 23 deletions(-)
 create mode 100644 include/scsi/scsi_block_pr.h

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 24e61647064c..765fca0f38c7 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -67,6 +67,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_block_pr.h>
 
 #include "sd.h"
 #include "scsi_priv.h"
@@ -1683,26 +1684,6 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
 	return ret;
 }
 
-static char sd_pr_type(enum pr_type type)
-{
-	switch (type) {
-	case PR_WRITE_EXCLUSIVE:
-		return 0x01;
-	case PR_EXCLUSIVE_ACCESS:
-		return 0x03;
-	case PR_WRITE_EXCLUSIVE_REG_ONLY:
-		return 0x05;
-	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
-		return 0x06;
-	case PR_WRITE_EXCLUSIVE_ALL_REGS:
-		return 0x07;
-	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
-		return 0x08;
-	default:
-		return 0;
-	}
-};
-
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1749,19 +1730,21 @@ static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
 {
 	if (flags)
 		return -EOPNOTSUPP;
-	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x01, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x02, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
 		enum pr_type type, bool abort)
 {
 	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
-			     sd_pr_type(type), 0);
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_clear(struct block_device *bdev, u64 key)
diff --git a/include/scsi/scsi_block_pr.h b/include/scsi/scsi_block_pr.h
new file mode 100644
index 000000000000..7d137c067331
--- /dev/null
+++ b/include/scsi/scsi_block_pr.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _SCSI_BLOCK_PR_H
+#define _SCSI_BLOCK_PR_H
+
+#include <uapi/linux/pr.h>
+
+static inline char block_pr_type_to_scsi(enum pr_type type)
+{
+	switch (type) {
+	case PR_WRITE_EXCLUSIVE:
+		return 0x01;
+	case PR_EXCLUSIVE_ACCESS:
+		return 0x03;
+	case PR_WRITE_EXCLUSIVE_REG_ONLY:
+		return 0x05;
+	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
+		return 0x06;
+	case PR_WRITE_EXCLUSIVE_ALL_REGS:
+		return 0x07;
+	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
+		return 0x08;
+	default:
+		return 0;
+	}
+};
+
+static inline enum pr_type scsi_pr_type_to_block(char type)
+{
+	switch (type) {
+	case 0x01:
+		return PR_WRITE_EXCLUSIVE;
+	case 0x03:
+		return PR_EXCLUSIVE_ACCESS;
+	case 0x05:
+		return PR_WRITE_EXCLUSIVE_REG_ONLY;
+	case 0x06:
+		return PR_EXCLUSIVE_ACCESS_REG_ONLY;
+	case 0x07:
+		return PR_WRITE_EXCLUSIVE_ALL_REGS;
+	case 0x08:
+		return PR_EXCLUSIVE_ACCESS_ALL_REGS;
+	default:
+		return 0;
+	}
+}
+
+#endif
-- 
2.25.1

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

Re: [PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> +static inline char block_pr_type_to_scsi(enum pr_type type)
> +{
> +	switch (type) {
> +	case PR_WRITE_EXCLUSIVE:
> +		return 0x01;
> +	case PR_EXCLUSIVE_ACCESS:
> +		return 0x03;
> +	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> +		return 0x05;
> +	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> +		return 0x06;
> +	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> +		return 0x07;
> +	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> +		return 0x08;
> +	default:
> +		return 0;
> +	}
> +};
> +
> +static inline enum pr_type scsi_pr_type_to_block(char type)
> +{
> +	switch (type) {
> +	case 0x01:
> +		return PR_WRITE_EXCLUSIVE;
> +	case 0x03:
> +		return PR_EXCLUSIVE_ACCESS;
> +	case 0x05:
> +		return PR_WRITE_EXCLUSIVE_REG_ONLY;
> +	case 0x06:
> +		return PR_EXCLUSIVE_ACCESS_REG_ONLY;
> +	case 0x07:
> +		return PR_WRITE_EXCLUSIVE_ALL_REGS;
> +	case 0x08:
> +		return PR_EXCLUSIVE_ACCESS_ALL_REGS;
> +	default:
> +		return 0;
> +	}
> +}

Since 'char' is used above to represent a single byte, please use u8 or 
uint8_t instead.

Thanks,

Bart.

Re: [dm-devel] [PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> +static inline char block_pr_type_to_scsi(enum pr_type type)
> +{
> +	switch (type) {
> +	case PR_WRITE_EXCLUSIVE:
> +		return 0x01;
> +	case PR_EXCLUSIVE_ACCESS:
> +		return 0x03;
> +	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> +		return 0x05;
> +	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> +		return 0x06;
> +	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> +		return 0x07;
> +	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> +		return 0x08;
> +	default:
> +		return 0;
> +	}
> +};
> +
> +static inline enum pr_type scsi_pr_type_to_block(char type)
> +{
> +	switch (type) {
> +	case 0x01:
> +		return PR_WRITE_EXCLUSIVE;
> +	case 0x03:
> +		return PR_EXCLUSIVE_ACCESS;
> +	case 0x05:
> +		return PR_WRITE_EXCLUSIVE_REG_ONLY;
> +	case 0x06:
> +		return PR_EXCLUSIVE_ACCESS_REG_ONLY;
> +	case 0x07:
> +		return PR_WRITE_EXCLUSIVE_ALL_REGS;
> +	case 0x08:
> +		return PR_EXCLUSIVE_ACCESS_ALL_REGS;
> +	default:
> +		return 0;
> +	}
> +}

Since 'char' is used above to represent a single byte, please use u8 or 
uint8_t instead.

Thanks,

Bart.

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

Re: [PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Christoph Hellwig]

Looks good modulo the u8 cleanup suggestion:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 03/11] scsi: Move sd_pr_type to header to share.

[Christoph Hellwig]

Looks good modulo the u8 cleanup suggestion:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 04/11] block: Add PR callouts for read keys and reservation

[Mike Christie]

Add callouts for reading keys and reservations.

Note: This only initially adds the struct definitions in the kernel as I'm
not sure if we wanted to export the interface to userspace yet. We may
want to refine internally for LIO and when we can enable it for NVMe then
add the finished interface to userspace.

The latter is not done, because NVMe's report reservation command is
similar to SCSI's read full status where they return a remote and local
ID as well as the key/reservation info. However, a lot of SCSI devices
don't do remote/local ID parts correctly. It seems that read full status
might not be used very often so does not get a lot of testing.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 include/linux/pr.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/pr.h b/include/linux/pr.h
index 94ceec713afe..21a8eb8b34b5 100644
--- a/include/linux/pr.h
+++ b/include/linux/pr.h
@@ -4,6 +4,18 @@
 
 #include <uapi/linux/pr.h>
 
+struct pr_keys {
+	u32	generation;
+	u32	num_keys;
+	u64	keys[];
+};
+
+struct pr_held_reservation {
+	u64	key;
+	u32	type;
+	u32	generation;
+};
+
 struct pr_ops {
 	int (*pr_register)(struct block_device *bdev, u64 old_key, u64 new_key,
 			u32 flags);
@@ -14,6 +26,10 @@ struct pr_ops {
 	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);
+	int (*pr_read_keys)(struct block_device *bdev,
+			    struct pr_keys *keys_info, int keys_info_len);
+	int (*pr_read_reservation)(struct block_device *bdev,
+				   struct pr_held_reservation *rsv);
 };
 
 #endif /* LINUX_PR_H */
-- 
2.25.1

[dm-devel] [PATCH 04/11] block: Add PR callouts for read keys and reservation

[Mike Christie]

Add callouts for reading keys and reservations.

Note: This only initially adds the struct definitions in the kernel as I'm
not sure if we wanted to export the interface to userspace yet. We may
want to refine internally for LIO and when we can enable it for NVMe then
add the finished interface to userspace.

The latter is not done, because NVMe's report reservation command is
similar to SCSI's read full status where they return a remote and local
ID as well as the key/reservation info. However, a lot of SCSI devices
don't do remote/local ID parts correctly. It seems that read full status
might not be used very often so does not get a lot of testing.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 include/linux/pr.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/pr.h b/include/linux/pr.h
index 94ceec713afe..21a8eb8b34b5 100644
--- a/include/linux/pr.h
+++ b/include/linux/pr.h
@@ -4,6 +4,18 @@
 
 #include <uapi/linux/pr.h>
 
+struct pr_keys {
+	u32	generation;
+	u32	num_keys;
+	u64	keys[];
+};
+
+struct pr_held_reservation {
+	u64	key;
+	u32	type;
+	u32	generation;
+};
+
 struct pr_ops {
 	int (*pr_register)(struct block_device *bdev, u64 old_key, u64 new_key,
 			u32 flags);
@@ -14,6 +26,10 @@ struct pr_ops {
 	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);
+	int (*pr_read_keys)(struct block_device *bdev,
+			    struct pr_keys *keys_info, int keys_info_len);
+	int (*pr_read_reservation)(struct block_device *bdev,
+				   struct pr_held_reservation *rsv);
 };
 
 #endif /* LINUX_PR_H */
-- 
2.25.1

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

Re: [PATCH 04/11] block: Add PR callouts for read keys and reservation

[Christoph Hellwig]

On Fri, Jun 03, 2022 at 01:55:29AM -0500, Mike Christie wrote:
> Add callouts for reading keys and reservations.
> 
> Note: This only initially adds the struct definitions in the kernel as I'm
> not sure if we wanted to export the interface to userspace yet. We may
> want to refine internally for LIO and when we can enable it for NVMe then
> add the finished interface to userspace.

Yes, this sounds like a good plan.  I can see how they would be useful
for userspace, but I'm also perfectly fine with us not locking ourselves
into an ABI quite yet.

Re: [dm-devel] [PATCH 04/11] block: Add PR callouts for read keys and reservation

[Christoph Hellwig]

On Fri, Jun 03, 2022 at 01:55:29AM -0500, Mike Christie wrote:
> Add callouts for reading keys and reservations.
> 
> Note: This only initially adds the struct definitions in the kernel as I'm
> not sure if we wanted to export the interface to userspace yet. We may
> want to refine internally for LIO and when we can enable it for NVMe then
> add the finished interface to userspace.

Yes, this sounds like a good plan.  I can see how they would be useful
for userspace, but I'm also perfectly fine with us not locking ourselves
into an ABI quite yet.

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

[PATCH 05/11] scsi: Add support for block PR read keys/reservation.

[Mike Christie]

This adds support in sd.c for the block PR read keys and read reservation
callouts.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c         | 88 +++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_proto.h |  5 +++
 2 files changed, 93 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 765fca0f38c7..71283aaf3e82 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1684,6 +1684,92 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
 	return ret;
 }
 
+static int sd_pr_in_command(struct block_device *bdev, u8 sa,
+			    unsigned char *data, int data_len)
+{
+	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
+	struct scsi_device *sdev = sdkp->device;
+	struct scsi_sense_hdr sshdr;
+	u8 cmd[10] = { 0, };
+	int result;
+
+	cmd[0] = PERSISTENT_RESERVE_IN;
+	cmd[1] = sa;
+	put_unaligned_be16(data_len, &cmd[7]);
+
+	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, data, data_len,
+				  &sshdr, SD_TIMEOUT, sdkp->max_retries, NULL);
+	if (scsi_status_is_check_condition(result) &&
+	    scsi_sense_valid(&sshdr)) {
+		sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
+		scsi_print_sense_hdr(sdev, NULL, &sshdr);
+	}
+
+	return result;
+}
+
+static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info,
+			   int keys_info_len)
+{
+	u8 *data = (u8 *)keys_info;
+	int result, i, data_offset;
+
+	if (keys_info_len < 8)
+		return -EINVAL;
+
+	result = sd_pr_in_command(bdev, READ_KEYS, data, keys_info_len);
+	if (result)
+		return result;
+	/*
+	 * The device gives us the info in BE, but users want it in a easier
+	 * to use format so we convert it for them.
+	 */
+	keys_info->generation = get_unaligned_be32(&data[0]);
+	keys_info->num_keys = get_unaligned_be32(&data[4]) / 8;
+
+	data_offset = 8;
+	for (i = 0; i < keys_info->num_keys; i++) {
+		if (data_offset + 8 > keys_info_len - 8)
+			return -EOVERFLOW;
+
+		keys_info->keys[i] = get_unaligned_be64(&data[data_offset]);
+		data_offset += 8;
+	}
+
+	return result;
+}
+
+static int sd_pr_read_reservation(struct block_device *bdev,
+				  struct pr_held_reservation *rsv)
+{
+	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
+	struct scsi_device *sdev = sdkp->device;
+	u8 data[24] = { 0, };
+	int result, len;
+
+	result = sd_pr_in_command(bdev, READ_RESERVATION, data, sizeof(data));
+	if (result)
+		return result;
+
+	memset(rsv, 0, sizeof(*rsv));
+	len = get_unaligned_be32(&data[4]);
+	if (!len)
+		return result;
+
+	/* Make sure we have at least the key and type */
+	if (len < 14) {
+		sdev_printk(KERN_INFO, sdev,
+			    "READ RESERVATION failed due to short return buffer of %d bytes\n",
+			    len);
+		return -EINVAL;
+	}
+
+	rsv->generation = get_unaligned_be32(&data[0]);
+	rsv->key = get_unaligned_be64(&data[8]);
+	rsv->type = scsi_pr_type_to_block(data[21] & 0x0f);
+	return 0;
+}
+
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1758,6 +1844,8 @@ static const struct pr_ops sd_pr_ops = {
 	.pr_release	= sd_pr_release,
 	.pr_preempt	= sd_pr_preempt,
 	.pr_clear	= sd_pr_clear,
+	.pr_read_keys	= sd_pr_read_keys,
+	.pr_read_reservation = sd_pr_read_reservation,
 };
 
 static void scsi_disk_free_disk(struct gendisk *disk)
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index f017843a8124..07ff33ea27e9 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -151,6 +151,11 @@
 #define ZO_FINISH_ZONE	      0x02
 #define ZO_OPEN_ZONE	      0x03
 #define ZO_RESET_WRITE_POINTER 0x04
+/* values for PR in service action */
+#define READ_KEYS             0x00
+#define READ_RESERVATION      0x01
+#define REPORT_CAPABILITES    0x02
+#define READ_FULL_STATUS      0x03
 /* values for variable length command */
 #define XDREAD_32	      0x03
 #define XDWRITE_32	      0x04
-- 
2.25.1

[dm-devel] [PATCH 05/11] scsi: Add support for block PR read keys/reservation.

[Mike Christie]

This adds support in sd.c for the block PR read keys and read reservation
callouts.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c         | 88 +++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_proto.h |  5 +++
 2 files changed, 93 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 765fca0f38c7..71283aaf3e82 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1684,6 +1684,92 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
 	return ret;
 }
 
+static int sd_pr_in_command(struct block_device *bdev, u8 sa,
+			    unsigned char *data, int data_len)
+{
+	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
+	struct scsi_device *sdev = sdkp->device;
+	struct scsi_sense_hdr sshdr;
+	u8 cmd[10] = { 0, };
+	int result;
+
+	cmd[0] = PERSISTENT_RESERVE_IN;
+	cmd[1] = sa;
+	put_unaligned_be16(data_len, &cmd[7]);
+
+	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, data, data_len,
+				  &sshdr, SD_TIMEOUT, sdkp->max_retries, NULL);
+	if (scsi_status_is_check_condition(result) &&
+	    scsi_sense_valid(&sshdr)) {
+		sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
+		scsi_print_sense_hdr(sdev, NULL, &sshdr);
+	}
+
+	return result;
+}
+
+static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info,
+			   int keys_info_len)
+{
+	u8 *data = (u8 *)keys_info;
+	int result, i, data_offset;
+
+	if (keys_info_len < 8)
+		return -EINVAL;
+
+	result = sd_pr_in_command(bdev, READ_KEYS, data, keys_info_len);
+	if (result)
+		return result;
+	/*
+	 * The device gives us the info in BE, but users want it in a easier
+	 * to use format so we convert it for them.
+	 */
+	keys_info->generation = get_unaligned_be32(&data[0]);
+	keys_info->num_keys = get_unaligned_be32(&data[4]) / 8;
+
+	data_offset = 8;
+	for (i = 0; i < keys_info->num_keys; i++) {
+		if (data_offset + 8 > keys_info_len - 8)
+			return -EOVERFLOW;
+
+		keys_info->keys[i] = get_unaligned_be64(&data[data_offset]);
+		data_offset += 8;
+	}
+
+	return result;
+}
+
+static int sd_pr_read_reservation(struct block_device *bdev,
+				  struct pr_held_reservation *rsv)
+{
+	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
+	struct scsi_device *sdev = sdkp->device;
+	u8 data[24] = { 0, };
+	int result, len;
+
+	result = sd_pr_in_command(bdev, READ_RESERVATION, data, sizeof(data));
+	if (result)
+		return result;
+
+	memset(rsv, 0, sizeof(*rsv));
+	len = get_unaligned_be32(&data[4]);
+	if (!len)
+		return result;
+
+	/* Make sure we have at least the key and type */
+	if (len < 14) {
+		sdev_printk(KERN_INFO, sdev,
+			    "READ RESERVATION failed due to short return buffer of %d bytes\n",
+			    len);
+		return -EINVAL;
+	}
+
+	rsv->generation = get_unaligned_be32(&data[0]);
+	rsv->key = get_unaligned_be64(&data[8]);
+	rsv->type = scsi_pr_type_to_block(data[21] & 0x0f);
+	return 0;
+}
+
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1758,6 +1844,8 @@ static const struct pr_ops sd_pr_ops = {
 	.pr_release	= sd_pr_release,
 	.pr_preempt	= sd_pr_preempt,
 	.pr_clear	= sd_pr_clear,
+	.pr_read_keys	= sd_pr_read_keys,
+	.pr_read_reservation = sd_pr_read_reservation,
 };
 
 static void scsi_disk_free_disk(struct gendisk *disk)
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index f017843a8124..07ff33ea27e9 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -151,6 +151,11 @@
 #define ZO_FINISH_ZONE	      0x02
 #define ZO_OPEN_ZONE	      0x03
 #define ZO_RESET_WRITE_POINTER 0x04
+/* values for PR in service action */
+#define READ_KEYS             0x00
+#define READ_RESERVATION      0x01
+#define REPORT_CAPABILITES    0x02
+#define READ_FULL_STATUS      0x03
 /* values for variable length command */
 #define XDREAD_32	      0x03
 #define XDWRITE_32	      0x04
-- 
2.25.1

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

[PATCH 06/11] dm: Add support for block PR read keys/reservation.

[Mike Christie]

This adds support in dm for the block PR read keys and read reservation
callouts.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/md/dm.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 82957bd460e8..ca544dfc3210 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3114,12 +3114,56 @@ static int dm_pr_clear(struct block_device *bdev, u64 key)
 	return r;
 }
 
+static int dm_pr_read_keys(struct block_device *bdev, struct pr_keys *keys,
+			   int keys_data_len)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_read_keys)
+		r = ops->pr_read_keys(bdev, keys, keys_data_len);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
+static int dm_pr_read_reservation(struct block_device *bdev,
+				  struct pr_held_reservation *rsv)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_read_reservation)
+		r = ops->pr_read_reservation(bdev, rsv);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
 static const struct pr_ops dm_pr_ops = {
 	.pr_register	= dm_pr_register,
 	.pr_reserve	= dm_pr_reserve,
 	.pr_release	= dm_pr_release,
 	.pr_preempt	= dm_pr_preempt,
 	.pr_clear	= dm_pr_clear,
+	.pr_read_keys	= dm_pr_read_keys,
+	.pr_read_reservation = dm_pr_read_reservation,
 };
 
 static const struct block_device_operations dm_blk_dops = {
-- 
2.25.1

[dm-devel] [PATCH 06/11] dm: Add support for block PR read keys/reservation.

[Mike Christie]

This adds support in dm for the block PR read keys and read reservation
callouts.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/md/dm.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 82957bd460e8..ca544dfc3210 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3114,12 +3114,56 @@ static int dm_pr_clear(struct block_device *bdev, u64 key)
 	return r;
 }
 
+static int dm_pr_read_keys(struct block_device *bdev, struct pr_keys *keys,
+			   int keys_data_len)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_read_keys)
+		r = ops->pr_read_keys(bdev, keys, keys_data_len);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
+static int dm_pr_read_reservation(struct block_device *bdev,
+				  struct pr_held_reservation *rsv)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_read_reservation)
+		r = ops->pr_read_reservation(bdev, rsv);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
 static const struct pr_ops dm_pr_ops = {
 	.pr_register	= dm_pr_register,
 	.pr_reserve	= dm_pr_reserve,
 	.pr_release	= dm_pr_release,
 	.pr_preempt	= dm_pr_preempt,
 	.pr_clear	= dm_pr_clear,
+	.pr_read_keys	= dm_pr_read_keys,
+	.pr_read_reservation = dm_pr_read_reservation,
 };
 
 static const struct block_device_operations dm_blk_dops = {
-- 
2.25.1

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

[PATCH 07/11] scsi: target: Allow backends to hook into PR handling.

[Mike Christie]

For the cases where you want to export a device to a VM via a single
I_T nexus and want to passthrough the PR handling to the physical/real
device you have to use pscsi or tcmu. Both are good for specific uses
however for the case where you want good performance, and are not using
SCSI devices directly (using DM/MD RAID or multipath devices) then we are
out of luck.

The following patches allow iblock to mimimally hook into the LIO PR code
and then pass the PR handling to the physical device. Note that like with
the tcmu an pscsi cases it's only supported when you export the device via
one I_T nexus.

This patch adds the initial LIO callouts. The next patch will modify
iblock.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_pr.c      | 60 ++++++++++++++++++++++++++++
 include/target/target_core_backend.h |  5 +++
 2 files changed, 65 insertions(+)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 3829b61b56c1..1c11f884e12f 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3531,6 +3531,26 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 	return ret;
 }
 
+static sense_reason_t
+target_try_pr_out_pt(struct se_cmd *cmd, u8 sa, u64 res_key, u64 sa_res_key,
+		     u8 type, bool aptpl, bool all_tg_pt, bool spec_i_pt)
+{
+	struct exec_cmd_ops *ops = cmd->protocol_data;
+
+	if (!cmd->se_sess || !cmd->se_lun) {
+		pr_err("SPC-3 PR: se_sess || struct se_lun is NULL!\n");
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+
+	if (!ops->execute_pr_out) {
+		pr_err("SPC-3 PR: Device has been configured for PR passthrough but it's not supported by the backend.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	return ops->execute_pr_out(cmd, sa, res_key, sa_res_key, type,
+				   aptpl, all_tg_pt, spec_i_pt);
+}
+
 /*
  * See spc4r17 section 6.14 Table 170
  */
@@ -3634,6 +3654,12 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 		return TCM_PARAMETER_LIST_LENGTH_ERROR;
 	}
 
+	if (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR) {
+		ret = target_try_pr_out_pt(cmd, sa, res_key, sa_res_key, type,
+					   aptpl, all_tg_pt, spec_i_pt);
+		goto done;
+	}
+
 	/*
 	 * (core_scsi3_emulate_pro_* function parameters
 	 * are defined by spc4r17 Table 174:
@@ -3675,6 +3701,7 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 		return TCM_INVALID_CDB_FIELD;
 	}
 
+done:
 	if (!ret)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return ret;
@@ -4032,6 +4059,33 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 	return 0;
 }
 
+static sense_reason_t target_try_pr_in_pt(struct se_cmd *cmd)
+{
+	struct exec_cmd_ops *ops = cmd->protocol_data;
+	unsigned char *buf;
+	sense_reason_t ret;
+
+	if (cmd->data_length < 8) {
+		pr_err("PRIN SA SCSI Data Length: %u too small\n",
+		       cmd->data_length);
+		return TCM_INVALID_CDB_FIELD;
+	}
+
+	if (!ops->execute_pr_in) {
+		pr_err("SPC-3 PR: Device has been configured for PR passthrough but it's not supported by the backend.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	buf = transport_kmap_data_sg(cmd);
+	if (!buf)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	ret = ops->execute_pr_in(cmd, cmd->t_task_cdb[1] & 0x1f, buf);
+
+	transport_kunmap_data_sg(cmd);
+	return ret;
+}
+
 sense_reason_t
 target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 {
@@ -4053,6 +4107,11 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 		return TCM_RESERVATION_CONFLICT;
 	}
 
+	if (cmd->se_dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR) {
+		ret = target_try_pr_in_pt(cmd);
+		goto done;
+	}
+
 	switch (cmd->t_task_cdb[1] & 0x1f) {
 	case PRI_READ_KEYS:
 		ret = core_scsi3_pri_read_keys(cmd);
@@ -4072,6 +4131,7 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 		return TCM_INVALID_CDB_FIELD;
 	}
 
+done:
 	if (!ret)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return ret;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 4e6cf1bf3b87..0b7f540be84f 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -68,6 +68,11 @@ struct exec_cmd_ops {
 	sense_reason_t (*execute_write_same)(struct se_cmd *cmd);
 	sense_reason_t (*execute_unmap)(struct se_cmd *cmd,
 				sector_t lba, sector_t nolb);
+	sense_reason_t (*execute_pr_out)(struct se_cmd *cmd, u8 sa, u64 key,
+					 u64 sa_key, u8 type, bool aptpl,
+					 bool all_tg_pt, bool spec_i_pt);
+	sense_reason_t (*execute_pr_in)(struct se_cmd *cmd, u8 sa,
+					unsigned char *param_data);
 };
 
 int	transport_backend_register(const struct target_backend_ops *);
-- 
2.25.1

[dm-devel] [PATCH 07/11] scsi: target: Allow backends to hook into PR handling.

[Mike Christie]

For the cases where you want to export a device to a VM via a single
I_T nexus and want to passthrough the PR handling to the physical/real
device you have to use pscsi or tcmu. Both are good for specific uses
however for the case where you want good performance, and are not using
SCSI devices directly (using DM/MD RAID or multipath devices) then we are
out of luck.

The following patches allow iblock to mimimally hook into the LIO PR code
and then pass the PR handling to the physical device. Note that like with
the tcmu an pscsi cases it's only supported when you export the device via
one I_T nexus.

This patch adds the initial LIO callouts. The next patch will modify
iblock.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_pr.c      | 60 ++++++++++++++++++++++++++++
 include/target/target_core_backend.h |  5 +++
 2 files changed, 65 insertions(+)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 3829b61b56c1..1c11f884e12f 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3531,6 +3531,26 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 	return ret;
 }
 
+static sense_reason_t
+target_try_pr_out_pt(struct se_cmd *cmd, u8 sa, u64 res_key, u64 sa_res_key,
+		     u8 type, bool aptpl, bool all_tg_pt, bool spec_i_pt)
+{
+	struct exec_cmd_ops *ops = cmd->protocol_data;
+
+	if (!cmd->se_sess || !cmd->se_lun) {
+		pr_err("SPC-3 PR: se_sess || struct se_lun is NULL!\n");
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+
+	if (!ops->execute_pr_out) {
+		pr_err("SPC-3 PR: Device has been configured for PR passthrough but it's not supported by the backend.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	return ops->execute_pr_out(cmd, sa, res_key, sa_res_key, type,
+				   aptpl, all_tg_pt, spec_i_pt);
+}
+
 /*
  * See spc4r17 section 6.14 Table 170
  */
@@ -3634,6 +3654,12 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 		return TCM_PARAMETER_LIST_LENGTH_ERROR;
 	}
 
+	if (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR) {
+		ret = target_try_pr_out_pt(cmd, sa, res_key, sa_res_key, type,
+					   aptpl, all_tg_pt, spec_i_pt);
+		goto done;
+	}
+
 	/*
 	 * (core_scsi3_emulate_pro_* function parameters
 	 * are defined by spc4r17 Table 174:
@@ -3675,6 +3701,7 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 		return TCM_INVALID_CDB_FIELD;
 	}
 
+done:
 	if (!ret)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return ret;
@@ -4032,6 +4059,33 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 	return 0;
 }
 
+static sense_reason_t target_try_pr_in_pt(struct se_cmd *cmd)
+{
+	struct exec_cmd_ops *ops = cmd->protocol_data;
+	unsigned char *buf;
+	sense_reason_t ret;
+
+	if (cmd->data_length < 8) {
+		pr_err("PRIN SA SCSI Data Length: %u too small\n",
+		       cmd->data_length);
+		return TCM_INVALID_CDB_FIELD;
+	}
+
+	if (!ops->execute_pr_in) {
+		pr_err("SPC-3 PR: Device has been configured for PR passthrough but it's not supported by the backend.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	buf = transport_kmap_data_sg(cmd);
+	if (!buf)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	ret = ops->execute_pr_in(cmd, cmd->t_task_cdb[1] & 0x1f, buf);
+
+	transport_kunmap_data_sg(cmd);
+	return ret;
+}
+
 sense_reason_t
 target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 {
@@ -4053,6 +4107,11 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 		return TCM_RESERVATION_CONFLICT;
 	}
 
+	if (cmd->se_dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR) {
+		ret = target_try_pr_in_pt(cmd);
+		goto done;
+	}
+
 	switch (cmd->t_task_cdb[1] & 0x1f) {
 	case PRI_READ_KEYS:
 		ret = core_scsi3_pri_read_keys(cmd);
@@ -4072,6 +4131,7 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 		return TCM_INVALID_CDB_FIELD;
 	}
 
+done:
 	if (!ret)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return ret;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 4e6cf1bf3b87..0b7f540be84f 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -68,6 +68,11 @@ struct exec_cmd_ops {
 	sense_reason_t (*execute_write_same)(struct se_cmd *cmd);
 	sense_reason_t (*execute_unmap)(struct se_cmd *cmd,
 				sector_t lba, sector_t nolb);
+	sense_reason_t (*execute_pr_out)(struct se_cmd *cmd, u8 sa, u64 key,
+					 u64 sa_key, u8 type, bool aptpl,
+					 bool all_tg_pt, bool spec_i_pt);
+	sense_reason_t (*execute_pr_in)(struct se_cmd *cmd, u8 sa,
+					unsigned char *param_data);
 };
 
 int	transport_backend_register(const struct target_backend_ops *);
-- 
2.25.1

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

Re: [PATCH 07/11] scsi: target: Allow backends to hook into PR handling.

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 07/11] scsi: target: Allow backends to hook into PR handling.

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 08/11] scsi: target: Add block PR support to iblock.

[Mike Christie]

This adds support for the block PR callouts to target_core_iblock. This
patch doesn't attempt to implement the entire spec because there's no way
to test it all. It only supports what's required for windows clustering
right now.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 312 ++++++++++++++++++++++++++++
 1 file changed, 312 insertions(+)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 9b0d788f6744..e8cf201e59ca 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -23,13 +23,16 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/pr.h>
 #include <scsi/scsi_proto.h>
+#include <scsi/scsi_block_pr.h>
 #include <asm/unaligned.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_backend.h>
 
 #include "target_core_iblock.h"
+#include "target_core_pr.h"
 
 #define IBLOCK_MAX_BIO_PER_TASK	 32	/* max # of bios to submit at a time */
 #define IBLOCK_BIO_POOL_SIZE	128
@@ -822,6 +825,295 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 }
 
+static sense_reason_t iblock_execute_pr_out(struct se_cmd *cmd, u8 sa, u64 key,
+					    u64 sa_key, u8 type, bool aptpl,
+					    bool all_tg_pt, bool spec_i_pt)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	int ret;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	switch (sa) {
+	case PRO_REGISTER_AND_MOVE:
+		pr_err("PRO_REGISTER_AND_MOVE is not supported by iblock PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	case PRO_REPLACE_LOST_RESERVATION:
+		pr_err("PRO_REPLACE_LOST_RESERVATION is not supported by iblock PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	case PRO_REGISTER:
+	case PRO_REGISTER_AND_IGNORE_EXISTING_KEY:
+		if (!ops->pr_register) {
+			pr_err("block device does not support pr_register.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		/*
+		 * We only support one target port. We don't know the target
+		 * port config at this level and the block layer has a
+		 * different view.
+		 */
+		if (spec_i_pt || all_tg_pt) {
+			pr_err("SPC-3 PR: SPEC_I_PT and ALL_TG_PT are not supported by PR passthrough.\n");
+
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		/* The block layer pr ops always enables aptpl */
+		if (!aptpl)
+			pr_info("APTPL not set by initiator, but will be used.\n");
+
+		ret = ops->pr_register(bdev, key, sa_key,
+				sa == PRO_REGISTER ? 0 : PR_FL_IGNORE_KEY);
+		break;
+	case PRO_RESERVE:
+		if (!ops->pr_reserve) {
+			pr_err("block_device does not support pr_reserve.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		switch (type) {
+		case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
+		case PR_TYPE_EXCLUSIVE_ACCESS_REGONLY:
+			break;
+		default:
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_reserve(bdev, key, scsi_pr_type_to_block(type), 0);
+		break;
+	case PRO_CLEAR:
+		if (!ops->pr_clear) {
+			pr_err("block_device does not support pr_clear.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_clear(bdev, key);
+		break;
+	case PRO_PREEMPT:
+	case PRO_PREEMPT_AND_ABORT:
+		if (!ops->pr_clear) {
+			pr_err("block_device does not support pr_preempt.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		switch (type) {
+		case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
+		case PR_TYPE_EXCLUSIVE_ACCESS_REGONLY:
+			break;
+		default:
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_preempt(bdev, key, sa_key,
+				      scsi_pr_type_to_block(type),
+				      sa == PRO_PREEMPT ? false : true);
+		break;
+	default:
+		pr_err("Unknown PERSISTENT_RESERVE_OUT SA: 0x%02x\n", sa);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (ret)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	return TCM_NO_SENSE;
+}
+
+static void iblock_pr_report_caps(unsigned char *param_data)
+{
+	u16 len = 8;
+
+	put_unaligned_be16(len, &param_data[0]);
+	/*
+	 * When using the pr_ops passthrough method we only support exporting
+	 * the device through one target port because from the backend module
+	 * level we can't see the target port config. As a result we only
+	 * support registration directly from the I_T nexus the cmd is sent
+	 * through and do not set ATP_C here.
+	 *
+	 * The block layer pr_ops do not support passing in initiators so
+	 * we don't set SIP_C here.
+	 */
+	/* PTPL_C: Persistence across Target Power Loss bit */
+	param_data[2] |= 0x01;
+	/*
+	 * We are filling in the PERSISTENT RESERVATION TYPE MASK below, so
+	 * set the TMV: Task Mask Valid bit.
+	 */
+	param_data[3] |= 0x80;
+	/*
+	 * Change ALLOW COMMANDs to 0x20 or 0x40 later from Table 166
+	 */
+	param_data[3] |= 0x10; /* ALLOW COMMANDs field 001b */
+	/*
+	 * PTPL_A: Persistence across Target Power Loss Active bit. The block
+	 * layer pr ops always enables this so report it active.
+	 */
+	param_data[3] |= 0x01;
+	/*
+	 * Setup the PERSISTENT RESERVATION TYPE MASK from Table 212 spc4r37.
+	 *
+	 * We are only supporting the following to simplify conversions we
+	 * may need to do between what the initiator sends us and what is
+	 * below us if there is a multipath device.
+	 */
+	param_data[4] |= 0x40; /* PR_TYPE_EXCLUSIVE_ACCESS_REGONLY */
+	param_data[4] |= 0x20; /* PR_TYPE_WRITE_EXCLUSIVE_REGONLY */
+}
+
+static int iblock_pr_read_keys(struct se_cmd *cmd, unsigned char *param_data)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	int i, ret, len, paths, data_offset;
+	struct pr_keys *keys;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (!ops->pr_read_keys) {
+		pr_err("Block device does not support read_keys.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	/*
+	 * We don't know what's under us, but dm-multipath will register every
+	 * path with the same key, so start off with enough space for 16 paths.
+	 */
+	paths = 16;
+retry:
+	len = sizeof(*keys) + (8 * paths);
+
+	keys = kzalloc(len, GFP_KERNEL);
+	if (!keys)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	ret = ops->pr_read_keys(bdev, keys, len);
+	if (ret == -EOVERFLOW) {
+		kfree(keys);
+		paths *= 2;
+		goto retry;
+	} else if (ret) {
+		ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		goto free_keys;
+	}
+
+	ret = TCM_NO_SENSE;
+
+	put_unaligned_be32(keys->generation, &param_data[0]);
+	if (!keys->num_keys) {
+		put_unaligned_be32(0, &param_data[4]);
+		goto free_keys;
+	}
+
+	/*
+	 * We only support a single I_T Nexus so there should only be one
+	 * key. There might be a multipath device under us but the initiator
+	 * does not know this. So, here we make sure the registered keys are
+	 * the same and that we only return the one key to match what the
+	 * initiator had sent us.
+	 */
+	put_unaligned_be32(8, &param_data[4]);
+
+	data_offset = 8;
+	for (i = 0; i < keys->num_keys; i++) {
+		if (data_offset + 8 > cmd->data_length)
+			break;
+
+		if (i == 0) {
+			put_unaligned_be64(keys->keys[i],
+					   &param_data[data_offset]);
+		} else if (keys->keys[i] != keys->keys[i - 1]) {
+			pr_err("Detected mismatched keys 0x%016llx and 0x%016llx\n",
+			       keys->keys[i], keys->keys[i - 1]);
+			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+			goto free_keys;
+		}
+
+		data_offset += 8;
+	}
+
+free_keys:
+	kfree(keys);
+	return ret;
+}
+
+static int iblock_pr_read_reservation(struct se_cmd *cmd,
+				      unsigned char *param_data)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_held_reservation rsv;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (!ops->pr_read_reservation) {
+		pr_err("Block device does not support read_keys.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (ops->pr_read_reservation(bdev, &rsv))
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	put_unaligned_be32(rsv.generation, &param_data[0]);
+	if (!rsv.key) {
+		put_unaligned_be32(0, &param_data[4]);
+		return TCM_NO_SENSE;
+	}
+
+	put_unaligned_be32(16, &param_data[4]);
+
+	if (cmd->data_length < 16)
+		return TCM_NO_SENSE;
+	put_unaligned_be64(rsv.key, &param_data[8]);
+
+	if (cmd->data_length < 22)
+		return TCM_NO_SENSE;
+	param_data[21] = block_pr_type_to_scsi(rsv.type);
+
+	return TCM_NO_SENSE;
+}
+
+static sense_reason_t iblock_execute_pr_in(struct se_cmd *cmd, u8 sa,
+					   unsigned char *param_data)
+{
+	sense_reason_t ret;
+
+	switch (sa) {
+	case PRI_REPORT_CAPABILITIES:
+		iblock_pr_report_caps(param_data);
+		break;
+	case PRI_READ_KEYS:
+		ret = iblock_pr_read_keys(cmd, param_data);
+		break;
+	case PRI_READ_RESERVATION:
+		ret = iblock_pr_read_reservation(cmd, param_data);
+		break;
+	case PRI_READ_FULL_STATUS:
+	default:
+		pr_err("Unknown PERSISTENT_RESERVE_IN SA: 0x%02x\n", sa);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	return ret;
+}
+
 static sector_t iblock_get_blocks(struct se_device *dev)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
@@ -876,11 +1168,30 @@ static struct exec_cmd_ops iblock_exec_cmd_ops = {
 	.execute_sync_cache	= iblock_execute_sync_cache,
 	.execute_write_same	= iblock_execute_write_same,
 	.execute_unmap		= iblock_execute_unmap,
+	.execute_pr_out		= iblock_execute_pr_out,
+	.execute_pr_in		= iblock_execute_pr_in,
 };
 
 static sense_reason_t
 iblock_parse_cdb(struct se_cmd *cmd)
 {
+	unsigned char *cdb = cmd->t_task_cdb;
+	struct se_device *dev = cmd->se_dev;
+
+	switch (cdb[0]) {
+	case RESERVE:
+	case RESERVE_10:
+	case RELEASE:
+	case RELEASE_10:
+		/*
+		 * The block layer pr_ops don't support the old RESERVE/RELEASE
+		 * commands.
+		 */
+		if (dev->dev_attrib.emulate_pr &&
+		    (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	return sbc_parse_cdb(cmd, &iblock_exec_cmd_ops);
 }
 
@@ -896,6 +1207,7 @@ static bool iblock_get_write_cache(struct se_device *dev)
 static const struct target_backend_ops iblock_ops = {
 	.name			= "iblock",
 	.inquiry_prod		= "IBLOCK",
+	.transport_flags_changeable = TRANSPORT_FLAG_PASSTHROUGH_PGR,
 	.inquiry_rev		= IBLOCK_VERSION,
 	.owner			= THIS_MODULE,
 	.attach_hba		= iblock_attach_hba,
-- 
2.25.1

[dm-devel] [PATCH 08/11] scsi: target: Add block PR support to iblock.

[Mike Christie]

This adds support for the block PR callouts to target_core_iblock. This
patch doesn't attempt to implement the entire spec because there's no way
to test it all. It only supports what's required for windows clustering
right now.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 312 ++++++++++++++++++++++++++++
 1 file changed, 312 insertions(+)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 9b0d788f6744..e8cf201e59ca 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -23,13 +23,16 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/pr.h>
 #include <scsi/scsi_proto.h>
+#include <scsi/scsi_block_pr.h>
 #include <asm/unaligned.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_backend.h>
 
 #include "target_core_iblock.h"
+#include "target_core_pr.h"
 
 #define IBLOCK_MAX_BIO_PER_TASK	 32	/* max # of bios to submit at a time */
 #define IBLOCK_BIO_POOL_SIZE	128
@@ -822,6 +825,295 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 }
 
+static sense_reason_t iblock_execute_pr_out(struct se_cmd *cmd, u8 sa, u64 key,
+					    u64 sa_key, u8 type, bool aptpl,
+					    bool all_tg_pt, bool spec_i_pt)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	int ret;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	switch (sa) {
+	case PRO_REGISTER_AND_MOVE:
+		pr_err("PRO_REGISTER_AND_MOVE is not supported by iblock PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	case PRO_REPLACE_LOST_RESERVATION:
+		pr_err("PRO_REPLACE_LOST_RESERVATION is not supported by iblock PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	case PRO_REGISTER:
+	case PRO_REGISTER_AND_IGNORE_EXISTING_KEY:
+		if (!ops->pr_register) {
+			pr_err("block device does not support pr_register.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		/*
+		 * We only support one target port. We don't know the target
+		 * port config at this level and the block layer has a
+		 * different view.
+		 */
+		if (spec_i_pt || all_tg_pt) {
+			pr_err("SPC-3 PR: SPEC_I_PT and ALL_TG_PT are not supported by PR passthrough.\n");
+
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		/* The block layer pr ops always enables aptpl */
+		if (!aptpl)
+			pr_info("APTPL not set by initiator, but will be used.\n");
+
+		ret = ops->pr_register(bdev, key, sa_key,
+				sa == PRO_REGISTER ? 0 : PR_FL_IGNORE_KEY);
+		break;
+	case PRO_RESERVE:
+		if (!ops->pr_reserve) {
+			pr_err("block_device does not support pr_reserve.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		switch (type) {
+		case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
+		case PR_TYPE_EXCLUSIVE_ACCESS_REGONLY:
+			break;
+		default:
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_reserve(bdev, key, scsi_pr_type_to_block(type), 0);
+		break;
+	case PRO_CLEAR:
+		if (!ops->pr_clear) {
+			pr_err("block_device does not support pr_clear.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_clear(bdev, key);
+		break;
+	case PRO_PREEMPT:
+	case PRO_PREEMPT_AND_ABORT:
+		if (!ops->pr_clear) {
+			pr_err("block_device does not support pr_preempt.\n");
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		switch (type) {
+		case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
+		case PR_TYPE_EXCLUSIVE_ACCESS_REGONLY:
+			break;
+		default:
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+		}
+
+		ret = ops->pr_preempt(bdev, key, sa_key,
+				      scsi_pr_type_to_block(type),
+				      sa == PRO_PREEMPT ? false : true);
+		break;
+	default:
+		pr_err("Unknown PERSISTENT_RESERVE_OUT SA: 0x%02x\n", sa);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (ret)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	return TCM_NO_SENSE;
+}
+
+static void iblock_pr_report_caps(unsigned char *param_data)
+{
+	u16 len = 8;
+
+	put_unaligned_be16(len, &param_data[0]);
+	/*
+	 * When using the pr_ops passthrough method we only support exporting
+	 * the device through one target port because from the backend module
+	 * level we can't see the target port config. As a result we only
+	 * support registration directly from the I_T nexus the cmd is sent
+	 * through and do not set ATP_C here.
+	 *
+	 * The block layer pr_ops do not support passing in initiators so
+	 * we don't set SIP_C here.
+	 */
+	/* PTPL_C: Persistence across Target Power Loss bit */
+	param_data[2] |= 0x01;
+	/*
+	 * We are filling in the PERSISTENT RESERVATION TYPE MASK below, so
+	 * set the TMV: Task Mask Valid bit.
+	 */
+	param_data[3] |= 0x80;
+	/*
+	 * Change ALLOW COMMANDs to 0x20 or 0x40 later from Table 166
+	 */
+	param_data[3] |= 0x10; /* ALLOW COMMANDs field 001b */
+	/*
+	 * PTPL_A: Persistence across Target Power Loss Active bit. The block
+	 * layer pr ops always enables this so report it active.
+	 */
+	param_data[3] |= 0x01;
+	/*
+	 * Setup the PERSISTENT RESERVATION TYPE MASK from Table 212 spc4r37.
+	 *
+	 * We are only supporting the following to simplify conversions we
+	 * may need to do between what the initiator sends us and what is
+	 * below us if there is a multipath device.
+	 */
+	param_data[4] |= 0x40; /* PR_TYPE_EXCLUSIVE_ACCESS_REGONLY */
+	param_data[4] |= 0x20; /* PR_TYPE_WRITE_EXCLUSIVE_REGONLY */
+}
+
+static int iblock_pr_read_keys(struct se_cmd *cmd, unsigned char *param_data)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	int i, ret, len, paths, data_offset;
+	struct pr_keys *keys;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (!ops->pr_read_keys) {
+		pr_err("Block device does not support read_keys.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	/*
+	 * We don't know what's under us, but dm-multipath will register every
+	 * path with the same key, so start off with enough space for 16 paths.
+	 */
+	paths = 16;
+retry:
+	len = sizeof(*keys) + (8 * paths);
+
+	keys = kzalloc(len, GFP_KERNEL);
+	if (!keys)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	ret = ops->pr_read_keys(bdev, keys, len);
+	if (ret == -EOVERFLOW) {
+		kfree(keys);
+		paths *= 2;
+		goto retry;
+	} else if (ret) {
+		ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		goto free_keys;
+	}
+
+	ret = TCM_NO_SENSE;
+
+	put_unaligned_be32(keys->generation, &param_data[0]);
+	if (!keys->num_keys) {
+		put_unaligned_be32(0, &param_data[4]);
+		goto free_keys;
+	}
+
+	/*
+	 * We only support a single I_T Nexus so there should only be one
+	 * key. There might be a multipath device under us but the initiator
+	 * does not know this. So, here we make sure the registered keys are
+	 * the same and that we only return the one key to match what the
+	 * initiator had sent us.
+	 */
+	put_unaligned_be32(8, &param_data[4]);
+
+	data_offset = 8;
+	for (i = 0; i < keys->num_keys; i++) {
+		if (data_offset + 8 > cmd->data_length)
+			break;
+
+		if (i == 0) {
+			put_unaligned_be64(keys->keys[i],
+					   &param_data[data_offset]);
+		} else if (keys->keys[i] != keys->keys[i - 1]) {
+			pr_err("Detected mismatched keys 0x%016llx and 0x%016llx\n",
+			       keys->keys[i], keys->keys[i - 1]);
+			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+			goto free_keys;
+		}
+
+		data_offset += 8;
+	}
+
+free_keys:
+	kfree(keys);
+	return ret;
+}
+
+static int iblock_pr_read_reservation(struct se_cmd *cmd,
+				      unsigned char *param_data)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+	struct block_device *bdev = ib_dev->ibd_bd;
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_held_reservation rsv;
+
+	if (!ops) {
+		pr_err("Block device does not support pr_ops but iblock device has been configured for PR passthrough.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (!ops->pr_read_reservation) {
+		pr_err("Block device does not support read_keys.\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (ops->pr_read_reservation(bdev, &rsv))
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	put_unaligned_be32(rsv.generation, &param_data[0]);
+	if (!rsv.key) {
+		put_unaligned_be32(0, &param_data[4]);
+		return TCM_NO_SENSE;
+	}
+
+	put_unaligned_be32(16, &param_data[4]);
+
+	if (cmd->data_length < 16)
+		return TCM_NO_SENSE;
+	put_unaligned_be64(rsv.key, &param_data[8]);
+
+	if (cmd->data_length < 22)
+		return TCM_NO_SENSE;
+	param_data[21] = block_pr_type_to_scsi(rsv.type);
+
+	return TCM_NO_SENSE;
+}
+
+static sense_reason_t iblock_execute_pr_in(struct se_cmd *cmd, u8 sa,
+					   unsigned char *param_data)
+{
+	sense_reason_t ret;
+
+	switch (sa) {
+	case PRI_REPORT_CAPABILITIES:
+		iblock_pr_report_caps(param_data);
+		break;
+	case PRI_READ_KEYS:
+		ret = iblock_pr_read_keys(cmd, param_data);
+		break;
+	case PRI_READ_RESERVATION:
+		ret = iblock_pr_read_reservation(cmd, param_data);
+		break;
+	case PRI_READ_FULL_STATUS:
+	default:
+		pr_err("Unknown PERSISTENT_RESERVE_IN SA: 0x%02x\n", sa);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	return ret;
+}
+
 static sector_t iblock_get_blocks(struct se_device *dev)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
@@ -876,11 +1168,30 @@ static struct exec_cmd_ops iblock_exec_cmd_ops = {
 	.execute_sync_cache	= iblock_execute_sync_cache,
 	.execute_write_same	= iblock_execute_write_same,
 	.execute_unmap		= iblock_execute_unmap,
+	.execute_pr_out		= iblock_execute_pr_out,
+	.execute_pr_in		= iblock_execute_pr_in,
 };
 
 static sense_reason_t
 iblock_parse_cdb(struct se_cmd *cmd)
 {
+	unsigned char *cdb = cmd->t_task_cdb;
+	struct se_device *dev = cmd->se_dev;
+
+	switch (cdb[0]) {
+	case RESERVE:
+	case RESERVE_10:
+	case RELEASE:
+	case RELEASE_10:
+		/*
+		 * The block layer pr_ops don't support the old RESERVE/RELEASE
+		 * commands.
+		 */
+		if (dev->dev_attrib.emulate_pr &&
+		    (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
+			return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	return sbc_parse_cdb(cmd, &iblock_exec_cmd_ops);
 }
 
@@ -896,6 +1207,7 @@ static bool iblock_get_write_cache(struct se_device *dev)
 static const struct target_backend_ops iblock_ops = {
 	.name			= "iblock",
 	.inquiry_prod		= "IBLOCK",
+	.transport_flags_changeable = TRANSPORT_FLAG_PASSTHROUGH_PGR,
 	.inquiry_rev		= IBLOCK_VERSION,
 	.owner			= THIS_MODULE,
 	.attach_hba		= iblock_attach_hba,
-- 
2.25.1

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

Re: [PATCH 08/11] scsi: target: Add block PR support to iblock.

[Christoph Hellwig]

> +static sense_reason_t iblock_execute_pr_in(struct se_cmd *cmd, u8 sa,
> +					   unsigned char *param_data)
> +{
> +	sense_reason_t ret;
> +
> +	switch (sa) {
> +	case PRI_REPORT_CAPABILITIES:
> +		iblock_pr_report_caps(param_data);
> +		break;
> +	case PRI_READ_KEYS:
> +		ret = iblock_pr_read_keys(cmd, param_data);
> +		break;
> +	case PRI_READ_RESERVATION:
> +		ret = iblock_pr_read_reservation(cmd, param_data);
> +		break;
> +	case PRI_READ_FULL_STATUS:
> +	default:
> +		pr_err("Unknown PERSISTENT_RESERVE_IN SA: 0x%02x\n", sa);
> +		return TCM_UNSUPPORTED_SCSI_OPCODE;
> +	}
> +
> +	return ret;


ret is uninitialize in the iblock_pr_report_caps case.

> +	switch (cdb[0]) {
> +	case RESERVE:
> +	case RESERVE_10:
> +	case RELEASE:
> +	case RELEASE_10:
> +		/*
> +		 * The block layer pr_ops don't support the old RESERVE/RELEASE
> +		 * commands.
> +		 */
> +		if (dev->dev_attrib.emulate_pr &&
> +		    (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
> +			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +	}

Can't this check go straight int sbc_parse_cdb?

Re: [dm-devel] [PATCH 08/11] scsi: target: Add block PR support to iblock.

[Christoph Hellwig]

> +static sense_reason_t iblock_execute_pr_in(struct se_cmd *cmd, u8 sa,
> +					   unsigned char *param_data)
> +{
> +	sense_reason_t ret;
> +
> +	switch (sa) {
> +	case PRI_REPORT_CAPABILITIES:
> +		iblock_pr_report_caps(param_data);
> +		break;
> +	case PRI_READ_KEYS:
> +		ret = iblock_pr_read_keys(cmd, param_data);
> +		break;
> +	case PRI_READ_RESERVATION:
> +		ret = iblock_pr_read_reservation(cmd, param_data);
> +		break;
> +	case PRI_READ_FULL_STATUS:
> +	default:
> +		pr_err("Unknown PERSISTENT_RESERVE_IN SA: 0x%02x\n", sa);
> +		return TCM_UNSUPPORTED_SCSI_OPCODE;
> +	}
> +
> +	return ret;


ret is uninitialize in the iblock_pr_report_caps case.

> +	switch (cdb[0]) {
> +	case RESERVE:
> +	case RESERVE_10:
> +	case RELEASE:
> +	case RELEASE_10:
> +		/*
> +		 * The block layer pr_ops don't support the old RESERVE/RELEASE
> +		 * commands.
> +		 */
> +		if (dev->dev_attrib.emulate_pr &&
> +		    (dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
> +			return TCM_UNSUPPORTED_SCSI_OPCODE;
> +	}

Can't this check go straight int sbc_parse_cdb?

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

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

[Mike Christie]

BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also
general nexus failures. For the pr_ops use we want to know if an IO failed
for specifically a reservation conflict so we can report that error upwards
to a VM. This patch adds a new error code for this case and converts nvme.
The next patch converts scsi because it's a little more complicated.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 block/blk-core.c          | 1 +
 drivers/nvme/host/core.c  | 2 +-
 include/linux/blk_types.h | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index bc0506772152..3908ac4a70b6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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" },
 
 	/* everything else not covered above: */
 	[BLK_STS_IOERR]		= { -EIO,	"I/O" },
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e1846d04817f..9b77d8eb480c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -268,7 +268,7 @@ static blk_status_t nvme_error_status(u16 status)
 	case NVME_SC_INVALID_PI:
 		return BLK_STS_PROTECTION;
 	case NVME_SC_RESERVATION_CONFLICT:
-		return BLK_STS_NEXUS;
+		return BLK_STS_RSV_CONFLICT;
 	case NVME_SC_HOST_PATH_ERROR:
 		return BLK_STS_TRANSPORT;
 	case NVME_SC_ZONE_TOO_MANY_ACTIVE:
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 1973ef9bd40f..927d9d30e1df 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -162,6 +162,9 @@ typedef u16 blk_short_t;
  */
 #define BLK_STS_OFFLINE		((__force blk_status_t)17)
 
+/* NVMe/SCSI reservation conflict. */
+#define BLK_STS_RSV_CONFLICT	((__force blk_status_t)18)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
@@ -183,6 +186,7 @@ static inline bool blk_path_error(blk_status_t error)
 	case BLK_STS_NEXUS:
 	case BLK_STS_MEDIUM:
 	case BLK_STS_PROTECTION:
+	case BLK_STS_RSV_CONFLICT:
 		return false;
 	}
 
-- 
2.25.1

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

[Mike Christie]

BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also
general nexus failures. For the pr_ops use we want to know if an IO failed
for specifically a reservation conflict so we can report that error upwards
to a VM. This patch adds a new error code for this case and converts nvme.
The next patch converts scsi because it's a little more complicated.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 block/blk-core.c          | 1 +
 drivers/nvme/host/core.c  | 2 +-
 include/linux/blk_types.h | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index bc0506772152..3908ac4a70b6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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" },
 
 	/* everything else not covered above: */
 	[BLK_STS_IOERR]		= { -EIO,	"I/O" },
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e1846d04817f..9b77d8eb480c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -268,7 +268,7 @@ static blk_status_t nvme_error_status(u16 status)
 	case NVME_SC_INVALID_PI:
 		return BLK_STS_PROTECTION;
 	case NVME_SC_RESERVATION_CONFLICT:
-		return BLK_STS_NEXUS;
+		return BLK_STS_RSV_CONFLICT;
 	case NVME_SC_HOST_PATH_ERROR:
 		return BLK_STS_TRANSPORT;
 	case NVME_SC_ZONE_TOO_MANY_ACTIVE:
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 1973ef9bd40f..927d9d30e1df 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -162,6 +162,9 @@ typedef u16 blk_short_t;
  */
 #define BLK_STS_OFFLINE		((__force blk_status_t)17)
 
+/* NVMe/SCSI reservation conflict. */
+#define BLK_STS_RSV_CONFLICT	((__force blk_status_t)18)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
@@ -183,6 +186,7 @@ static inline bool blk_path_error(blk_status_t error)
 	case BLK_STS_NEXUS:
 	case BLK_STS_MEDIUM:
 	case BLK_STS_PROTECTION:
+	case BLK_STS_RSV_CONFLICT:
 		return false;
 	}
 
-- 
2.25.1

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

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

[Keith Busch]

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.

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

[Keith Busch]

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.

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

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

[Mike Christie]

On 6/3/22 2:45 PM, 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". :)

Will fix.

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

Ah ok I might have misuderstood the reason/usage of the -Exyz error.

The patches in this set use the pr_ops in the kernel so I can see the BLK_STS
value. We do bio based IO so we get that value in the end io callback.

I thought the -Exyx error can get returned to userspace. Because scsi and nvme
currently return -EBADE for reservation conflicts I thought I had to keep doing
that. If that's not the case, then yeah -EACCESS is better and I'll definitely
use it.

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

[Mike Christie]

On 6/3/22 2:45 PM, 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". :)

Will fix.

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

Ah ok I might have misuderstood the reason/usage of the -Exyz error.

The patches in this set use the pr_ops in the kernel so I can see the BLK_STS
value. We do bio based IO so we get that value in the end io callback.

I thought the -Exyx error can get returned to userspace. Because scsi and nvme
currently return -EBADE for reservation conflicts I thought I had to keep doing
that. If that's not the case, then yeah -EACCESS is better and I'll definitely
use it.

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

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

[Hannes Reinecke]

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'd rather modify the existing code to return BLK_STS_RSV_CONFLICT 
instead of BLK_STS_NEXUS, but keep the 'EBADE' mapping.
Userspace ABI and all that.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Hannes Reinecke]

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'd rather modify the existing code to return BLK_STS_RSV_CONFLICT 
instead of BLK_STS_NEXUS, but keep the 'EBADE' mapping.
Userspace ABI and all that.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

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

[michael.christie]

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.

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

[michael.christie]

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.

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

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

[Hannes Reinecke]

On 6/4/22 19:13, michael.christie@oracle.com wrote:
> 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.

Well ... we tried to find a generic error for reservation failure, as we 
thought that reservation failure was too SCSI specific.
And we wanted the error to describe what the resulting handling should 
be, not what the cause was. Hence we ended up with BLK_STS_NEXUS.

But turns out that our initial assumption wasn't valid, and that 
reservations are a general concept. So by all means, rename 
BLK_STS_NEXUS to BLK_STS_RSV_CONFLICT to make it clear what this error 
is about.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Hannes Reinecke]

On 6/4/22 19:13, michael.christie@oracle.com wrote:
> 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.

Well ... we tried to find a generic error for reservation failure, as we 
thought that reservation failure was too SCSI specific.
And we wanted the error to describe what the resulting handling should 
be, not what the cause was. Hence we ended up with BLK_STS_NEXUS.

But turns out that our initial assumption wasn't valid, and that 
reservations are a general concept. So by all means, rename 
BLK_STS_NEXUS to BLK_STS_RSV_CONFLICT to make it clear what this error 
is about.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

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

[Christoph Hellwig]

On Sun, Jun 05, 2022 at 11:42:11AM +0200, Hannes Reinecke wrote:
> Well ... we tried to find a generic error for reservation failure, as we 
> thought that reservation failure was too SCSI specific.
> And we wanted the error to describe what the resulting handling should be, 
> not what the cause was. Hence we ended up with BLK_STS_NEXUS.
>
> But turns out that our initial assumption wasn't valid, and that 
> reservations are a general concept. So by all means, rename BLK_STS_NEXUS 
> to BLK_STS_RSV_CONFLICT to make it clear what this error is about.

I think think this is a good ida, but we'll need to involve the
s390 dasd folks.  Maybe do this as a separate prep patch?

While thinking about DASD I think it would benefit from returning
the blk_status_t from ->free_cp insted of the hand crafted conversion
as well.

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

[Christoph Hellwig]

On Sun, Jun 05, 2022 at 11:42:11AM +0200, Hannes Reinecke wrote:
> Well ... we tried to find a generic error for reservation failure, as we 
> thought that reservation failure was too SCSI specific.
> And we wanted the error to describe what the resulting handling should be, 
> not what the cause was. Hence we ended up with BLK_STS_NEXUS.
>
> But turns out that our initial assumption wasn't valid, and that 
> reservations are a general concept. So by all means, rename BLK_STS_NEXUS 
> to BLK_STS_RSV_CONFLICT to make it clear what this error is about.

I think think this is a good ida, but we'll need to involve the
s390 dasd folks.  Maybe do this as a separate prep patch?

While thinking about DASD I think it would benefit from returning
the blk_status_t from ->free_cp insted of the hand crafted conversion
as well.

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

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

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also
> general nexus failures. For the pr_ops use we want to know if an IO failed
> for specifically a reservation conflict so we can report that error upwards
> to a VM. This patch adds a new error code for this case and converts nvme.
> The next patch converts scsi because it's a little more complicated.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   block/blk-core.c          | 1 +
>   drivers/nvme/host/core.c  | 2 +-
>   include/linux/blk_types.h | 4 ++++
>   3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index bc0506772152..3908ac4a70b6 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -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" },
                                                  ^^^^^^^^^^

Please fix the spelling of "reservation".

Thanks,

Bart.

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

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also
> general nexus failures. For the pr_ops use we want to know if an IO failed
> for specifically a reservation conflict so we can report that error upwards
> to a VM. This patch adds a new error code for this case and converts nvme.
> The next patch converts scsi because it's a little more complicated.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   block/blk-core.c          | 1 +
>   drivers/nvme/host/core.c  | 2 +-
>   include/linux/blk_types.h | 4 ++++
>   3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index bc0506772152..3908ac4a70b6 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -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" },
                                                  ^^^^^^^^^^

Please fix the spelling of "reservation".

Thanks,

Bart.

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

[PATCH 10/11] scsi: Use BLK_STS_RSV_CONFLICT for reservation conflicts

[Mike Christie]

This has scsi use BLK_STS_RSV_CONFLICT for reservation conflicts so upper
layers like lio can distinguish this between a general nexus error and a
reservation conflict. For the latter we can then report that error to VMs.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_error.c |  1 -
 drivers/scsi/scsi_lib.c   | 11 ++++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cdaca13ac1f1..621627486e5c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1985,7 +1985,6 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd)
 	case SAM_STAT_RESERVATION_CONFLICT:
 		sdev_printk(KERN_INFO, scmd->device,
 			    "reservation conflict\n");
-		set_host_byte(scmd, DID_NEXUS_FAILURE);
 		return SUCCESS; /* causes immediate i/o error */
 	}
 	return FAILED;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d18cc7e510e..623dc1cee51e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -597,7 +597,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 	case DID_OK:
 		if (scsi_status_is_good(result))
 			return BLK_STS_OK;
-		return BLK_STS_IOERR;
+		break;
 	case DID_TRANSPORT_FAILFAST:
 	case DID_TRANSPORT_MARGINAL:
 		return BLK_STS_TRANSPORT;
@@ -613,9 +613,14 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 	case DID_MEDIUM_ERROR:
 		set_host_byte(cmd, DID_OK);
 		return BLK_STS_MEDIUM;
-	default:
-		return BLK_STS_IOERR;
 	}
+
+	switch (get_status_byte(cmd)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		return BLK_STS_RSV_CONFLICT;
+	}
+
+	return BLK_STS_IOERR;
 }
 
 /**
-- 
2.25.1

[dm-devel] [PATCH 10/11] scsi: Use BLK_STS_RSV_CONFLICT for reservation conflicts

[Mike Christie]

This has scsi use BLK_STS_RSV_CONFLICT for reservation conflicts so upper
layers like lio can distinguish this between a general nexus error and a
reservation conflict. For the latter we can then report that error to VMs.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_error.c |  1 -
 drivers/scsi/scsi_lib.c   | 11 ++++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cdaca13ac1f1..621627486e5c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1985,7 +1985,6 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd)
 	case SAM_STAT_RESERVATION_CONFLICT:
 		sdev_printk(KERN_INFO, scmd->device,
 			    "reservation conflict\n");
-		set_host_byte(scmd, DID_NEXUS_FAILURE);
 		return SUCCESS; /* causes immediate i/o error */
 	}
 	return FAILED;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d18cc7e510e..623dc1cee51e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -597,7 +597,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 	case DID_OK:
 		if (scsi_status_is_good(result))
 			return BLK_STS_OK;
-		return BLK_STS_IOERR;
+		break;
 	case DID_TRANSPORT_FAILFAST:
 	case DID_TRANSPORT_MARGINAL:
 		return BLK_STS_TRANSPORT;
@@ -613,9 +613,14 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 	case DID_MEDIUM_ERROR:
 		set_host_byte(cmd, DID_OK);
 		return BLK_STS_MEDIUM;
-	default:
-		return BLK_STS_IOERR;
 	}
+
+	switch (get_status_byte(cmd)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		return BLK_STS_RSV_CONFLICT;
+	}
+
+	return BLK_STS_IOERR;
 }
 
 /**
-- 
2.25.1

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

[PATCH 11/11] scsi: target: Handle BLK_STS_RSV_CONFLICT.

[Mike Christie]

If we get a BLK_STS_RSV_CONFLICT report it to the initiator instead of
using the SAM_STAT_CHECK_CONDITION which gets translated to a generic
error that initiators don't know how to handle.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index e8cf201e59ca..2a964b57303a 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -309,7 +309,7 @@ static unsigned long long iblock_emulate_read_cap_with_block_size(
 	return blocks_long;
 }
 
-static void iblock_complete_cmd(struct se_cmd *cmd)
+static void iblock_complete_cmd(struct se_cmd *cmd, blk_status_t blk_status)
 {
 	struct iblock_req *ibr = cmd->priv;
 	u8 status;
@@ -317,7 +317,9 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
 	if (!refcount_dec_and_test(&ibr->pending))
 		return;
 
-	if (atomic_read(&ibr->ib_bio_err_cnt))
+	if (blk_status == BLK_STS_RSV_CONFLICT)
+		status = SAM_STAT_RESERVATION_CONFLICT;
+	else if (atomic_read(&ibr->ib_bio_err_cnt))
 		status = SAM_STAT_CHECK_CONDITION;
 	else
 		status = SAM_STAT_GOOD;
@@ -330,6 +332,7 @@ static void iblock_bio_done(struct bio *bio)
 {
 	struct se_cmd *cmd = bio->bi_private;
 	struct iblock_req *ibr = cmd->priv;
+	blk_status_t blk_status = bio->bi_status;
 
 	if (bio->bi_status) {
 		pr_err("bio error: %p,  err: %d\n", bio, bio->bi_status);
@@ -342,7 +345,7 @@ static void iblock_bio_done(struct bio *bio)
 
 	bio_put(bio);
 
-	iblock_complete_cmd(cmd);
+	iblock_complete_cmd(cmd, blk_status);
 }
 
 static struct bio *iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num,
@@ -755,7 +758,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 
 	if (!sgl_nents) {
 		refcount_set(&ibr->pending, 1);
-		iblock_complete_cmd(cmd);
+		iblock_complete_cmd(cmd, BLK_STS_OK);
 		return 0;
 	}
 
@@ -813,7 +816,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	}
 
 	iblock_submit_bios(&list);
-	iblock_complete_cmd(cmd);
+	iblock_complete_cmd(cmd, BLK_STS_OK);
 	return 0;
 
 fail_put_bios:
-- 
2.25.1

[dm-devel] [PATCH 11/11] scsi: target: Handle BLK_STS_RSV_CONFLICT.

[Mike Christie]

If we get a BLK_STS_RSV_CONFLICT report it to the initiator instead of
using the SAM_STAT_CHECK_CONDITION which gets translated to a generic
error that initiators don't know how to handle.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index e8cf201e59ca..2a964b57303a 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -309,7 +309,7 @@ static unsigned long long iblock_emulate_read_cap_with_block_size(
 	return blocks_long;
 }
 
-static void iblock_complete_cmd(struct se_cmd *cmd)
+static void iblock_complete_cmd(struct se_cmd *cmd, blk_status_t blk_status)
 {
 	struct iblock_req *ibr = cmd->priv;
 	u8 status;
@@ -317,7 +317,9 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
 	if (!refcount_dec_and_test(&ibr->pending))
 		return;
 
-	if (atomic_read(&ibr->ib_bio_err_cnt))
+	if (blk_status == BLK_STS_RSV_CONFLICT)
+		status = SAM_STAT_RESERVATION_CONFLICT;
+	else if (atomic_read(&ibr->ib_bio_err_cnt))
 		status = SAM_STAT_CHECK_CONDITION;
 	else
 		status = SAM_STAT_GOOD;
@@ -330,6 +332,7 @@ static void iblock_bio_done(struct bio *bio)
 {
 	struct se_cmd *cmd = bio->bi_private;
 	struct iblock_req *ibr = cmd->priv;
+	blk_status_t blk_status = bio->bi_status;
 
 	if (bio->bi_status) {
 		pr_err("bio error: %p,  err: %d\n", bio, bio->bi_status);
@@ -342,7 +345,7 @@ static void iblock_bio_done(struct bio *bio)
 
 	bio_put(bio);
 
-	iblock_complete_cmd(cmd);
+	iblock_complete_cmd(cmd, blk_status);
 }
 
 static struct bio *iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num,
@@ -755,7 +758,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 
 	if (!sgl_nents) {
 		refcount_set(&ibr->pending, 1);
-		iblock_complete_cmd(cmd);
+		iblock_complete_cmd(cmd, BLK_STS_OK);
 		return 0;
 	}
 
@@ -813,7 +816,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	}
 
 	iblock_submit_bios(&list);
-	iblock_complete_cmd(cmd);
+	iblock_complete_cmd(cmd, BLK_STS_OK);
 	return 0;
 
 fail_put_bios:
-- 
2.25.1

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

Re: [PATCH 0/8] Use block pr_ops in LIO

[Christoph Hellwig]

From a highlevel POV this looks good.  I'll do a more detail review in
the next days once I find a little time.  Any chance you could also
wire up the new ops for nvme so that we know the abtractions work right
even for exporting nvme as SCSI?  No need to wire up nvmet for now, but
that would be the logical next step.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Christoph Hellwig]

From a highlevel POV this looks good.  I'll do a more detail review in
the next days once I find a little time.  Any chance you could also
wire up the new ops for nvme so that we know the abtractions work right
even for exporting nvme as SCSI?  No need to wire up nvmet for now, but
that would be the logical next step.

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

Re: [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/3/22 6:46 AM, Christoph Hellwig wrote:
> From a highlevel POV this looks good.  I'll do a more detail review in
> the next days once I find a little time.  Any chance you could also
> wire up the new ops for nvme so that we know the abtractions work right
> even for exporting nvme as SCSI?  No need to wire up nvmet for now, but
> that would be the logical next step.

I forgot to cc nvme. Doing that now. For the nvme list developers here
the patchset discussed in this thread:

https://lore.kernel.org/linux-scsi/20220603114645.GA14309@lst.de/T/#t

For the patchset in this thread I only did the simple SCSI commands
READ_KEYS and READ_RESERVATION. For nvme-only people, those commands just
return the registered keys and the reservation info like the key and
type. There is no controller/host ID type of info like in nvme's
report reservation command.

I did the basic commands because the apps I need to support don't use the
more advanced SCSI command READ_FULL_STATUS which returns the equivalent of
nvme's controller and host ID. I also hit issues with SCSI targets not
implementing the command correctly. For example, the linux scsi target just
got fixed last year and it only works by accident in some cases where we have
2 bugs that cancel each other out :)

However, for nvme and for the interface we want to provide to userspace,
do we want to implement an interface like READ_FULL_STATUS and report
reservations where we report the host/controller/port info? If so, below
is a patch I started.

Notes:
1. I hit some issues with SCSI targets not reporting the IDs sometimes or
sometimes they report it incorrectly. For nvme, it seems easier. SCSI has 
to handle a hand full of ways to report the ID where nvme has 2 ways to
do the host ID.

2. I couldn't find a nvme device to test. Qemu and nvmet don't seem to
support reservations.

3. The patch is only compile tested. It's based on a different patch I
did. I'm just posting because you can see the pr_reservations_info data
struct we could use for nvme and scsi if we want to report the ID info
and keys/registrations in one command.


diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ca544dfc3210..161a715ab70a 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3156,6 +3156,28 @@ static int dm_pr_read_reservation(struct block_device *bdev,
 	return r;
 }
 
+static int dm_pr_report_reservation(struct block_device *bdev,
+				    struct pr_reservation_info *rsv_info,
+				    int rsv_regs_len)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_report_reservation)
+		r = ops->pr_report_reservation(bdev, rsv_info, rsv_regs_len);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
 static const struct pr_ops dm_pr_ops = {
 	.pr_register	= dm_pr_register,
 	.pr_reserve	= dm_pr_reserve,
@@ -3163,7 +3185,8 @@ static const struct pr_ops dm_pr_ops = {
 	.pr_preempt	= dm_pr_preempt,
 	.pr_clear	= dm_pr_clear,
 	.pr_read_keys	= dm_pr_read_keys,
-	.pr_read_reservation = dm_pr_read_reservation,
+	.pr_read_reservation	= dm_pr_read_reservation,
+	.pr_report_reservation	= dm_pr_report_reservation,
 };
 
 static const struct block_device_operations dm_blk_dops = {
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 71283aaf3e82..1f251b8f381d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1770,6 +1770,101 @@ static int sd_pr_read_reservation(struct block_device *bdev,
 	return 0;
 }
 
+static int sd_pr_read_full_status(struct block_device *bdev,
+				  struct pr_reservation_info *rsv_info,
+				  int rsv_regs_len)
+{
+	int len, full_data_off, i, result, num_regs;
+	struct pr_registration_info *reg_info;
+	struct pr_keys keys_info;
+	u8 *full_data;
+
+retry:
+	result = sd_pr_read_keys(bdev, &keys_info, 0);
+	if (result)
+		return result;
+
+	if (!keys_info.num_keys)
+		return 0;
+
+	len = keys_info.num_keys * sizeof(*reg_info);
+	if (len >= rsv_regs_len)
+		return -EOVERFLOW;
+	len += 8;
+
+	full_data = kzalloc(len, GFP_KERNEL);
+	if (!full_data)
+		return -ENOMEM;
+
+	result = sd_pr_in_command(bdev, READ_FULL_STATUS, full_data, len);
+	if (result) {
+		/*
+		 * TODO? - If it's not supported do we want to drop down
+		 * to READ_KEYS/RESERVATION and just not fill out the port
+		 * and transport ID info.
+		 */
+		return result;
+	}
+
+	num_regs = get_unaligned_be32(&full_data[4]) / 8;
+	/*
+	 * We can have fewer registrations if the target did the ALL_TG_PT
+	 * format where it does not report every I_T nexus. If we now have
+	 * more registrations then someone is doing PR out commands so try
+	 * to get a bigger buffer.
+	 */
+	if (keys_info.num_keys < num_regs) {
+		kfree(full_data);
+		goto retry;
+	}
+
+	rsv_info->generation = get_unaligned_be32(&full_data[0]);
+	rsv_info->num_registrations = num_regs;
+
+	full_data_off = 8;
+
+	for (i = 0; i < num_regs; i++) {
+		/* every reg should have the same type? */
+		rsv_info->type = scsi_pr_type_to_block(full_data[13] & 0x0f);
+
+		reg_info = &rsv_info->registrations[i];
+		reg_info->key = get_unaligned_be64(&full_data[0]);
+
+		if (full_data[12] & 0x01)
+			reg_info->flags |= PR_REG_INFO_FL_HOLDER;
+		if (full_data[12] & 0x02)
+			reg_info->flags |= PR_REG_INFO_FL_ALL_TG_PT;
+
+		/* We use SCSI rel target port id for remote_id */
+		reg_info->remote_id = get_unaligned_be16(&full_data[18]);
+
+		/* We use SCSI transport ID as the local_id */
+		reg_info->local_id_len = get_unaligned_be32(&full_data[20]);
+		if (!reg_info->local_id_len)
+			continue;
+
+		/*
+		 * TODO. Do we fail or operate like the SCSI spec and return
+		 * what we have and user should know that they messed up
+		 * and need to send a bigger buffer to get the rest of the
+		 * data;
+		 */
+		full_data_off += 24;
+		if (full_data_off + reg_info->local_id_len >= rsv_regs_len)
+			return -EOVERFLOW;
+		/*
+		 * TODO - we should put this in a easier to use format for
+		 * users.
+		 */
+		memcpy(reg_info->local_id, &full_data[full_data_off],
+		       reg_info->local_id_len);
+		full_data_off += reg_info->local_id_len;
+	}
+
+	kfree(full_data);
+	return 0;
+}
+
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1845,7 +1940,8 @@ static const struct pr_ops sd_pr_ops = {
 	.pr_preempt	= sd_pr_preempt,
 	.pr_clear	= sd_pr_clear,
 	.pr_read_keys	= sd_pr_read_keys,
-	.pr_read_reservation = sd_pr_read_reservation,
+	.pr_read_reservation	= sd_pr_read_reservation,
+	.pr_report_reservation	= sd_pr_read_full_status,
 };
 
 static void scsi_disk_free_disk(struct gendisk *disk)
diff --git a/include/linux/pr.h b/include/linux/pr.h
index 21a8eb8b34b5..ec325a0ed33c 100644
--- a/include/linux/pr.h
+++ b/include/linux/pr.h
@@ -30,6 +30,9 @@ struct pr_ops {
 			    struct pr_keys *keys_info, int keys_info_len);
 	int (*pr_read_reservation)(struct block_device *bdev,
 				   struct pr_held_reservation *rsv);
+	int (*pr_report_reservation)(struct block_device *bdev,
+				     struct pr_reservation_info *rsv_info,
+				     int rsv_regs_len);
 };
 
 #endif /* LINUX_PR_H */
diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..66028d37ac5d 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -39,6 +39,35 @@ struct pr_clear {
 	__u32	__pad;
 };
 
+/* Is reservation holder */
+#define PR_REG_INFO_FL_HOLDER		(1 << 0)
+/*
+ * Registration applies to all SCSI target ports accesed through initiator port
+ * at local_id. remote_id is not set in this case.
+ */
+#define PR_REG_INFO_FL_ALL_TG_PT	(1 << 1)
+
+struct pr_registration_info {
+	__u64	key;
+	__u8	flags;
+	__u8	__pad;
+	/* NVMe Controler ID or SCSI relative target port id */
+	__u16	remote_id;
+	__u32	__pad2;
+	/* local_id size in bytes. */
+	__u32	local_id_len;
+	/* NVMe Host ID or SCSI Transport ID */
+	char	local_id[];
+};
+
+struct pr_reservation_info {
+	__u32	generation;
+	__u16	num_registrations;
+	__u8	type;
+	__u8	__pad;
+	struct pr_registration_info registrations[];
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/3/22 6:46 AM, Christoph Hellwig wrote:
> From a highlevel POV this looks good.  I'll do a more detail review in
> the next days once I find a little time.  Any chance you could also
> wire up the new ops for nvme so that we know the abtractions work right
> even for exporting nvme as SCSI?  No need to wire up nvmet for now, but
> that would be the logical next step.

I forgot to cc nvme. Doing that now. For the nvme list developers here
the patchset discussed in this thread:

https://lore.kernel.org/linux-scsi/20220603114645.GA14309@lst.de/T/#t

For the patchset in this thread I only did the simple SCSI commands
READ_KEYS and READ_RESERVATION. For nvme-only people, those commands just
return the registered keys and the reservation info like the key and
type. There is no controller/host ID type of info like in nvme's
report reservation command.

I did the basic commands because the apps I need to support don't use the
more advanced SCSI command READ_FULL_STATUS which returns the equivalent of
nvme's controller and host ID. I also hit issues with SCSI targets not
implementing the command correctly. For example, the linux scsi target just
got fixed last year and it only works by accident in some cases where we have
2 bugs that cancel each other out :)

However, for nvme and for the interface we want to provide to userspace,
do we want to implement an interface like READ_FULL_STATUS and report
reservations where we report the host/controller/port info? If so, below
is a patch I started.

Notes:
1. I hit some issues with SCSI targets not reporting the IDs sometimes or
sometimes they report it incorrectly. For nvme, it seems easier. SCSI has 
to handle a hand full of ways to report the ID where nvme has 2 ways to
do the host ID.

2. I couldn't find a nvme device to test. Qemu and nvmet don't seem to
support reservations.

3. The patch is only compile tested. It's based on a different patch I
did. I'm just posting because you can see the pr_reservations_info data
struct we could use for nvme and scsi if we want to report the ID info
and keys/registrations in one command.


diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ca544dfc3210..161a715ab70a 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3156,6 +3156,28 @@ static int dm_pr_read_reservation(struct block_device *bdev,
 	return r;
 }
 
+static int dm_pr_report_reservation(struct block_device *bdev,
+				    struct pr_reservation_info *rsv_info,
+				    int rsv_regs_len)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	const struct pr_ops *ops;
+	int r, srcu_idx;
+
+	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
+	if (r < 0)
+		goto out;
+
+	ops = bdev->bd_disk->fops->pr_ops;
+	if (ops && ops->pr_report_reservation)
+		r = ops->pr_report_reservation(bdev, rsv_info, rsv_regs_len);
+	else
+		r = -EOPNOTSUPP;
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return r;
+}
+
 static const struct pr_ops dm_pr_ops = {
 	.pr_register	= dm_pr_register,
 	.pr_reserve	= dm_pr_reserve,
@@ -3163,7 +3185,8 @@ static const struct pr_ops dm_pr_ops = {
 	.pr_preempt	= dm_pr_preempt,
 	.pr_clear	= dm_pr_clear,
 	.pr_read_keys	= dm_pr_read_keys,
-	.pr_read_reservation = dm_pr_read_reservation,
+	.pr_read_reservation	= dm_pr_read_reservation,
+	.pr_report_reservation	= dm_pr_report_reservation,
 };
 
 static const struct block_device_operations dm_blk_dops = {
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 71283aaf3e82..1f251b8f381d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1770,6 +1770,101 @@ static int sd_pr_read_reservation(struct block_device *bdev,
 	return 0;
 }
 
+static int sd_pr_read_full_status(struct block_device *bdev,
+				  struct pr_reservation_info *rsv_info,
+				  int rsv_regs_len)
+{
+	int len, full_data_off, i, result, num_regs;
+	struct pr_registration_info *reg_info;
+	struct pr_keys keys_info;
+	u8 *full_data;
+
+retry:
+	result = sd_pr_read_keys(bdev, &keys_info, 0);
+	if (result)
+		return result;
+
+	if (!keys_info.num_keys)
+		return 0;
+
+	len = keys_info.num_keys * sizeof(*reg_info);
+	if (len >= rsv_regs_len)
+		return -EOVERFLOW;
+	len += 8;
+
+	full_data = kzalloc(len, GFP_KERNEL);
+	if (!full_data)
+		return -ENOMEM;
+
+	result = sd_pr_in_command(bdev, READ_FULL_STATUS, full_data, len);
+	if (result) {
+		/*
+		 * TODO? - If it's not supported do we want to drop down
+		 * to READ_KEYS/RESERVATION and just not fill out the port
+		 * and transport ID info.
+		 */
+		return result;
+	}
+
+	num_regs = get_unaligned_be32(&full_data[4]) / 8;
+	/*
+	 * We can have fewer registrations if the target did the ALL_TG_PT
+	 * format where it does not report every I_T nexus. If we now have
+	 * more registrations then someone is doing PR out commands so try
+	 * to get a bigger buffer.
+	 */
+	if (keys_info.num_keys < num_regs) {
+		kfree(full_data);
+		goto retry;
+	}
+
+	rsv_info->generation = get_unaligned_be32(&full_data[0]);
+	rsv_info->num_registrations = num_regs;
+
+	full_data_off = 8;
+
+	for (i = 0; i < num_regs; i++) {
+		/* every reg should have the same type? */
+		rsv_info->type = scsi_pr_type_to_block(full_data[13] & 0x0f);
+
+		reg_info = &rsv_info->registrations[i];
+		reg_info->key = get_unaligned_be64(&full_data[0]);
+
+		if (full_data[12] & 0x01)
+			reg_info->flags |= PR_REG_INFO_FL_HOLDER;
+		if (full_data[12] & 0x02)
+			reg_info->flags |= PR_REG_INFO_FL_ALL_TG_PT;
+
+		/* We use SCSI rel target port id for remote_id */
+		reg_info->remote_id = get_unaligned_be16(&full_data[18]);
+
+		/* We use SCSI transport ID as the local_id */
+		reg_info->local_id_len = get_unaligned_be32(&full_data[20]);
+		if (!reg_info->local_id_len)
+			continue;
+
+		/*
+		 * TODO. Do we fail or operate like the SCSI spec and return
+		 * what we have and user should know that they messed up
+		 * and need to send a bigger buffer to get the rest of the
+		 * data;
+		 */
+		full_data_off += 24;
+		if (full_data_off + reg_info->local_id_len >= rsv_regs_len)
+			return -EOVERFLOW;
+		/*
+		 * TODO - we should put this in a easier to use format for
+		 * users.
+		 */
+		memcpy(reg_info->local_id, &full_data[full_data_off],
+		       reg_info->local_id_len);
+		full_data_off += reg_info->local_id_len;
+	}
+
+	kfree(full_data);
+	return 0;
+}
+
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1845,7 +1940,8 @@ static const struct pr_ops sd_pr_ops = {
 	.pr_preempt	= sd_pr_preempt,
 	.pr_clear	= sd_pr_clear,
 	.pr_read_keys	= sd_pr_read_keys,
-	.pr_read_reservation = sd_pr_read_reservation,
+	.pr_read_reservation	= sd_pr_read_reservation,
+	.pr_report_reservation	= sd_pr_read_full_status,
 };
 
 static void scsi_disk_free_disk(struct gendisk *disk)
diff --git a/include/linux/pr.h b/include/linux/pr.h
index 21a8eb8b34b5..ec325a0ed33c 100644
--- a/include/linux/pr.h
+++ b/include/linux/pr.h
@@ -30,6 +30,9 @@ struct pr_ops {
 			    struct pr_keys *keys_info, int keys_info_len);
 	int (*pr_read_reservation)(struct block_device *bdev,
 				   struct pr_held_reservation *rsv);
+	int (*pr_report_reservation)(struct block_device *bdev,
+				     struct pr_reservation_info *rsv_info,
+				     int rsv_regs_len);
 };
 
 #endif /* LINUX_PR_H */
diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..66028d37ac5d 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -39,6 +39,35 @@ struct pr_clear {
 	__u32	__pad;
 };
 
+/* Is reservation holder */
+#define PR_REG_INFO_FL_HOLDER		(1 << 0)
+/*
+ * Registration applies to all SCSI target ports accesed through initiator port
+ * at local_id. remote_id is not set in this case.
+ */
+#define PR_REG_INFO_FL_ALL_TG_PT	(1 << 1)
+
+struct pr_registration_info {
+	__u64	key;
+	__u8	flags;
+	__u8	__pad;
+	/* NVMe Controler ID or SCSI relative target port id */
+	__u16	remote_id;
+	__u32	__pad2;
+	/* local_id size in bytes. */
+	__u32	local_id_len;
+	/* NVMe Host ID or SCSI Transport ID */
+	char	local_id[];
+};
+
+struct pr_reservation_info {
+	__u32	generation;
+	__u16	num_registrations;
+	__u8	type;
+	__u8	__pad;
+	struct pr_registration_info registrations[];
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)

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

Re: [PATCH 0/8] Use block pr_ops in LIO

[Christoph Hellwig]

On Fri, Jun 03, 2022 at 12:55:33PM -0500, Mike Christie wrote:
> However, for nvme and for the interface we want to provide to userspace,
> do we want to implement an interface like READ_FULL_STATUS and report
> reservations where we report the host/controller/port info? If so, below
> is a patch I started.

If we wire the ops up to the nvme target we'd need that.  But for now
I think the more useful case would be to use nvme as the underlying
devices for the scsi target that already has all the PR infrastructure
and helps to validate the interface.

> Notes:
> 1. I hit some issues with SCSI targets not reporting the IDs sometimes or
> sometimes they report it incorrectly. For nvme, it seems easier. SCSI has 
> to handle a hand full of ways to report the ID where nvme has 2 ways to
> do the host ID.

Yeah.

> 2. I couldn't find a nvme device to test. Qemu and nvmet don't seem to
> support reservations.

Basically any dual ported PCIe SSD should support them, typically those
are the U.2 format factor ones found in servers or enclosures.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Christoph Hellwig]

On Fri, Jun 03, 2022 at 12:55:33PM -0500, Mike Christie wrote:
> However, for nvme and for the interface we want to provide to userspace,
> do we want to implement an interface like READ_FULL_STATUS and report
> reservations where we report the host/controller/port info? If so, below
> is a patch I started.

If we wire the ops up to the nvme target we'd need that.  But for now
I think the more useful case would be to use nvme as the underlying
devices for the scsi target that already has all the PR infrastructure
and helps to validate the interface.

> Notes:
> 1. I hit some issues with SCSI targets not reporting the IDs sometimes or
> sometimes they report it incorrectly. For nvme, it seems easier. SCSI has 
> to handle a hand full of ways to report the ID where nvme has 2 ways to
> do the host ID.

Yeah.

> 2. I couldn't find a nvme device to test. Qemu and nvmet don't seem to
> support reservations.

Basically any dual ported PCIe SSD should support them, typically those
are the U.2 format factor ones found in servers or enclosures.

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

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> The following patches were built over Linus's tree. They allow us to use
> the block pr_ops with LIO's target_core_iblock module to support cluster
> applications in VMs.
> 
> Currently, to use something like windows clustering 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.
> 
> Note that this is patchset does not attempt to support every PR SCSI
> feature in iblock. It has the same limitations as tcmu and pscsi where
> you can have a single I_T nexus per device and only supports what is
> needed for windows clustering right now.

How has this patch series been tested? Does LIO pass the libiscsi 
persistent reservation tests with this patch series applied?

Thanks,

Bart.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Bart Van Assche]

On 6/2/22 23:55, Mike Christie wrote:
> The following patches were built over Linus's tree. They allow us to use
> the block pr_ops with LIO's target_core_iblock module to support cluster
> applications in VMs.
> 
> Currently, to use something like windows clustering 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.
> 
> Note that this is patchset does not attempt to support every PR SCSI
> feature in iblock. It has the same limitations as tcmu and pscsi where
> you can have a single I_T nexus per device and only supports what is
> needed for windows clustering right now.

How has this patch series been tested? Does LIO pass the libiscsi 
persistent reservation tests with this patch series applied?

Thanks,

Bart.

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

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/4/22 11:01 PM, Bart Van Assche wrote:
> On 6/2/22 23:55, Mike Christie wrote:
>> The following patches were built over Linus's tree. They allow us to use
>> the block pr_ops with LIO's target_core_iblock module to support cluster
>> applications in VMs.
>>
>> Currently, to use something like windows clustering 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.
>>
>> Note that this is patchset does not attempt to support every PR SCSI
>> feature in iblock. It has the same limitations as tcmu and pscsi where
>> you can have a single I_T nexus per device and only supports what is
>> needed for windows clustering right now.
> 
> How has this patch series been tested? Does LIO pass the libiscsi persistent reservation tests with this patch series applied?
> 

libiscsi is not suitable for this type of setup. If libiscsi works correctly,
then this patchset should fail. It's probably opposite of what you are
thinking about. We are not supporting a a single instance of LIO/qemu that
handles multiple I_T nexues like what libiscsi can test well. It's more
like multiple LIO/qemu instances each on different systems that each have a
single I_T nexus between the VM's initiator and LIO/qemu. So it's more of
a passthrough between the VM and real device.

For example, right now to use a cluster app in VMs with a backend device that
is itself cluster aware/shared you commonly do:

1. Qemu's userspace block layer which can send SG_IO to your real backend
device to do the PR request. Checks for conflicts are then done by the
backend device as well.

So here you have 2 systems. On system0, Qemu0 exports /dev/sdb to VM0. VM0
only has the one I_T nexus. System1 exports /dev/sdb to  VM1. VM1 only has
the one I_T nexus as well.

2. Qemu vhost-scsi with pscsi or tcmu. In these cases it's similar as 1 where
you have 2 different systems. How you pass the PRs to the real device may
differ for tcmu. pscsi just injects them into the scsi queue. We do not use
the LIO pr code at all (pgr_support=0).

3. This patchset allows you to use Qemu vhost-scsi with iblock. The setup will
be similar as 1 and 2 but we use a different backend driver.

To test this type of thing you would want a cluster aware libiscsi where
you do a pr register and reserve in VM0, then in VM1 you would do the WRITE
to check that your pr_type is honored from that I_T nexus.

And so we are going to run our internal QA type of tests, but we are hoping to
also implement some qemu clustered SCSI type of tests like this. We are still
trying to figure out the framework (looking into Luis's ansible based stuff,
etc) because for general iscsi testing we want to be able to kick off multiple
VMs and bare metal systems and run both open-iscsi + lio tests.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/4/22 11:01 PM, Bart Van Assche wrote:
> On 6/2/22 23:55, Mike Christie wrote:
>> The following patches were built over Linus's tree. They allow us to use
>> the block pr_ops with LIO's target_core_iblock module to support cluster
>> applications in VMs.
>>
>> Currently, to use something like windows clustering 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.
>>
>> Note that this is patchset does not attempt to support every PR SCSI
>> feature in iblock. It has the same limitations as tcmu and pscsi where
>> you can have a single I_T nexus per device and only supports what is
>> needed for windows clustering right now.
> 
> How has this patch series been tested? Does LIO pass the libiscsi persistent reservation tests with this patch series applied?
> 

libiscsi is not suitable for this type of setup. If libiscsi works correctly,
then this patchset should fail. It's probably opposite of what you are
thinking about. We are not supporting a a single instance of LIO/qemu that
handles multiple I_T nexues like what libiscsi can test well. It's more
like multiple LIO/qemu instances each on different systems that each have a
single I_T nexus between the VM's initiator and LIO/qemu. So it's more of
a passthrough between the VM and real device.

For example, right now to use a cluster app in VMs with a backend device that
is itself cluster aware/shared you commonly do:

1. Qemu's userspace block layer which can send SG_IO to your real backend
device to do the PR request. Checks for conflicts are then done by the
backend device as well.

So here you have 2 systems. On system0, Qemu0 exports /dev/sdb to VM0. VM0
only has the one I_T nexus. System1 exports /dev/sdb to  VM1. VM1 only has
the one I_T nexus as well.

2. Qemu vhost-scsi with pscsi or tcmu. In these cases it's similar as 1 where
you have 2 different systems. How you pass the PRs to the real device may
differ for tcmu. pscsi just injects them into the scsi queue. We do not use
the LIO pr code at all (pgr_support=0).

3. This patchset allows you to use Qemu vhost-scsi with iblock. The setup will
be similar as 1 and 2 but we use a different backend driver.

To test this type of thing you would want a cluster aware libiscsi where
you do a pr register and reserve in VM0, then in VM1 you would do the WRITE
to check that your pr_type is honored from that I_T nexus.

And so we are going to run our internal QA type of tests, but we are hoping to
also implement some qemu clustered SCSI type of tests like this. We are still
trying to figure out the framework (looking into Luis's ansible based stuff,
etc) because for general iscsi testing we want to be able to kick off multiple
VMs and bare metal systems and run both open-iscsi + lio tests.

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

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Bart Van Assche]

On 6/5/22 09:55, Mike Christie wrote:
> libiscsi is not suitable for this type of setup.
I think that this setup can be tested as follows with libiscsi:
* Configure the backend storage.
* Configure LIO to use the backend storage on two different servers.
* On a third server, log in with the iSCSI initiator to both servers.
* Run the libiscsi iscsi-test-cu test software on the third server and
   pass the two IQNs that refer to the LIO servers as arguments.

 From the iscsi-test-cu -h output:

     iscsi-test-cu [OPTIONS] <iscsi-url> [multipath-iscsi-url]

Did I perhaps overlook or misunderstand something?

Thanks,

Bart.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Bart Van Assche]

On 6/5/22 09:55, Mike Christie wrote:
> libiscsi is not suitable for this type of setup.
I think that this setup can be tested as follows with libiscsi:
* Configure the backend storage.
* Configure LIO to use the backend storage on two different servers.
* On a third server, log in with the iSCSI initiator to both servers.
* Run the libiscsi iscsi-test-cu test software on the third server and
   pass the two IQNs that refer to the LIO servers as arguments.

 From the iscsi-test-cu -h output:

     iscsi-test-cu [OPTIONS] <iscsi-url> [multipath-iscsi-url]

Did I perhaps overlook or misunderstand something?

Thanks,

Bart.

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

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/5/22 1:15 PM, Bart Van Assche wrote:
> On 6/5/22 09:55, Mike Christie wrote:
>> libiscsi is not suitable for this type of setup.
> I think that this setup can be tested as follows with libiscsi:
> * Configure the backend storage.
> * Configure LIO to use the backend storage on two different servers.
> * On a third server, log in with the iSCSI initiator to both servers.
> * Run the libiscsi iscsi-test-cu test software on the third server and
>   pass the two IQNs that refer to the LIO servers as arguments.
> 
> From the iscsi-test-cu -h output:
> 
>     iscsi-test-cu [OPTIONS] <iscsi-url> [multipath-iscsi-url]
> 

Ah I didn't see that. In the README/man it only describes the multiple
initiator name feature for multiple sessions. They don't have the
multipath arg, so I didn't know you can pass in the second target.

> Did I perhaps overlook or misunderstand something?

It works. Thanks.

Re: [dm-devel] [PATCH 0/8] Use block pr_ops in LIO

[Mike Christie]

On 6/5/22 1:15 PM, Bart Van Assche wrote:
> On 6/5/22 09:55, Mike Christie wrote:
>> libiscsi is not suitable for this type of setup.
> I think that this setup can be tested as follows with libiscsi:
> * Configure the backend storage.
> * Configure LIO to use the backend storage on two different servers.
> * On a third server, log in with the iSCSI initiator to both servers.
> * Run the libiscsi iscsi-test-cu test software on the third server and
>   pass the two IQNs that refer to the LIO servers as arguments.
> 
> From the iscsi-test-cu -h output:
> 
>     iscsi-test-cu [OPTIONS] <iscsi-url> [multipath-iscsi-url]
> 

Ah I didn't see that. In the README/man it only describes the multiple
initiator name feature for multiple sessions. They don't have the
multipath arg, so I didn't know you can pass in the second target.

> Did I perhaps overlook or misunderstand something?

It works. Thanks.

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