target: add emulate_pr backstore attr to toggle PR support

RFC PATCH Superseded David Disseldorp 3 participants 8 messages Jun 1, 2018 – Jun 21, 2018 SCSI target

Share & tools

David Disseldorp

author

2018-06-01 00:05 UTC #0 lore raw

RFC PATCH 1/1 target: add emulate_pr backstore attr to toggle PR support

The new emulate_pr backstore attribute allows for Persistent Reservation and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be useful for scenarios such as:
- Enforcing ATS (Compare & Write) usage on recent VMware ESXi initiators
- Allowing clustered (e.g. tcm-user) backends to block such requests, avoiding the multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive Invalid Command Operation Code response sense data.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
 drivers/target/target_core_device.c   |  1 +
 drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
 include/target/target_core_base.h     |  3 +++
 4 files changed, 42 insertions(+), 5 deletions(-)

drivers/target/target_core_configfs.c +16 -5

index 3f4bf126eed0..17b830e35a08 100644
@@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);

@@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);

@@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);

@@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_emulate_tpws,
 	&attr_emulate_caw,
 	&attr_emulate_3pc,
+	&attr_emulate_pr,
 	&attr_pi_prot_type,
 	&attr_hw_pi_prot_type,
 	&attr_pi_prot_format,

@@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "Passthrough\n");
 
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		ret = target_core_dev_pr_show_spc2_res(dev, page);

@@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)
 
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "SPC_PASSTHROUGH\n");
-	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		return sprintf(page, "SPC2_RESERVATIONS\n");
-	else
-		return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+	return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,

@@ -1578,7 +1587,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)
 		return 0;
 
 	return sprintf(page, "APTPL Bit Status: %s\n",

@@ -1590,7 +1600,8 @@ static ssize_t target_pr_res_aptpl_metadata_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)
 		return 0;
 
 	return sprintf(page, "Ready to process PR APTPL metadata..\n");

drivers/target/target_core_device.c +1 -0

index e27db4d45a9d..96a60072e446 100644
@@ -806,6 +806,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	dev->dev_attrib.emulate_tpws = DA_EMULATE_TPWS;
 	dev->dev_attrib.emulate_caw = DA_EMULATE_CAW;
 	dev->dev_attrib.emulate_3pc = DA_EMULATE_3PC;
+	dev->dev_attrib.emulate_pr = DA_EMULATE_PR;
 	dev->dev_attrib.pi_prot_type = TARGET_DIF_TYPE0_PROT;
 	dev->dev_attrib.enforce_pr_isids = DA_ENFORCE_PR_ISIDS;
 	dev->dev_attrib.force_pr_aptpl = DA_FORCE_PR_APTPL;

drivers/target/target_core_pr.c +22 -0

index 2e865fdaa362..9960a44c3a59 100644
@@ -208,6 +208,11 @@ target_scsi2_reservation_release(struct se_cmd *cmd)
 	struct se_portal_group *tpg;
 	int rc;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing SCSI2 RELEASE with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	if (!sess || !sess->se_tpg)
 		goto out;
 	rc = target_check_scsi2_reservation_conflict(cmd);

@@ -255,6 +260,11 @@ target_scsi2_reservation_reserve(struct se_cmd *cmd)
 	sense_reason_t ret = 0;
 	int rc;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing SCSI2 RESERVE with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	if ((cmd->t_task_cdb[1] & 0x01) &&
 	    (cmd->t_task_cdb[1] & 0x02)) {
 		pr_err("LongIO and Obsolete Bits set, returning ILLEGAL_REQUEST\n");

@@ -3560,6 +3570,11 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 	int spec_i_pt = 0, all_tg_pt = 0, unreg = 0;
 	sense_reason_t ret;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing PROUT with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	/*
 	 * Following spc2r20 5.5.1 Reservations overview:
 	 *

@@ -4045,6 +4060,11 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 {
 	sense_reason_t ret;
 
+	if (!cmd->se_dev->dev_attrib.emulate_pr) {
+		pr_err("failing PRIN with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	/*
 	 * Following spc2r20 5.5.1 Reservations overview:
 	 *

@@ -4097,6 +4117,8 @@ target_check_reservation(struct se_cmd *cmd)
 		return 0;
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return 0;
+	if (!dev->dev_attrib.emulate_pr)
+		return 0;
 
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)

include/target/target_core_base.h +3 -0

index 9f9f5902af38..06d88645938b 100644
@@ -87,6 +87,8 @@
 #define DA_EMULATE_3PC				1
 /* No Emulation for PSCSI by default */
 #define DA_EMULATE_ALUA				0
+/* Emulate SCSI2 RESERVE/RELEASE and Persistent Reservations by default */
+#define DA_EMULATE_PR				1
 /* Enforce SCSI Initiator Port TransportID with 'ISID' for PR */
 #define DA_ENFORCE_PR_ISIDS			1
 /* Force SPC-3 PR Activate Persistence across Target Power Loss */

@@ -665,6 +667,7 @@ struct se_dev_attrib {
 	int		emulate_tpws;
 	int		emulate_caw;
 	int		emulate_3pc;
+	int		emulate_pr;
 	int		pi_prot_format;
 	enum target_prot_type pi_prot_type;
 	enum target_prot_type hw_pi_prot_type;

Signature · 1 line

2.13.6

Bryant G. Ly

2018-06-01 13:20 UTC #1 lore raw

Quoted 187 lines · David Disseldorp

When explicitly disabled, PR and RESERVE/RELEASE requests receive Invalid Command Operation Code response sense data.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
 drivers/target/target_core_device.c   |  1 +
 drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
 include/target/target_core_base.h     |  3 +++
 4 files changed, 42 insertions(+), 5 deletions(-)

drivers/target/target_core_configfs.c +16 -5

index 3f4bf126eed0..17b830e35a08 100644
@@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);

@@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);

@@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);

@@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_emulate_tpws,
 	&attr_emulate_caw,
 	&attr_emulate_3pc,
+	&attr_emulate_pr,
 	&attr_pi_prot_type,
 	&attr_hw_pi_prot_type,
 	&attr_pi_prot_format,

@@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "Passthrough\n");
 
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		ret = target_core_dev_pr_show_spc2_res(dev, page);

@@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)
 
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "SPC_PASSTHROUGH\n");
-	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		return sprintf(page, "SPC2_RESERVATIONS\n");
-	else
-		return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+	return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,

@@ -1578,7 +1587,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)
 		return 0;
 
 	return sprintf(page, "APTPL Bit Status: %s\n",

@@ -1590,7 +1600,8 @@ static ssize_t target_pr_res_aptpl_metadata_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)
 		return 0;
 
 	return sprintf(page, "Ready to process PR APTPL metadata..\n");

drivers/target/target_core_device.c +1 -0

index e27db4d45a9d..96a60072e446 100644
@@ -806,6 +806,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	dev->dev_attrib.emulate_tpws = DA_EMULATE_TPWS;
 	dev->dev_attrib.emulate_caw = DA_EMULATE_CAW;
 	dev->dev_attrib.emulate_3pc = DA_EMULATE_3PC;
+	dev->dev_attrib.emulate_pr = DA_EMULATE_PR;
 	dev->dev_attrib.pi_prot_type = TARGET_DIF_TYPE0_PROT;
 	dev->dev_attrib.enforce_pr_isids = DA_ENFORCE_PR_ISIDS;
 	dev->dev_attrib.force_pr_aptpl = DA_FORCE_PR_APTPL;

drivers/target/target_core_pr.c +22 -0

index 2e865fdaa362..9960a44c3a59 100644
@@ -208,6 +208,11 @@ target_scsi2_reservation_release(struct se_cmd *cmd)
 	struct se_portal_group *tpg;
 	int rc;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing SCSI2 RELEASE with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	if (!sess || !sess->se_tpg)
 		goto out;
 	rc = target_check_scsi2_reservation_conflict(cmd);

@@ -255,6 +260,11 @@ target_scsi2_reservation_reserve(struct se_cmd *cmd)
 	sense_reason_t ret = 0;
 	int rc;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing SCSI2 RESERVE with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	if ((cmd->t_task_cdb[1] & 0x01) &&
 	    (cmd->t_task_cdb[1] & 0x02)) {
 		pr_err("LongIO and Obsolete Bits set, returning ILLEGAL_REQUEST\n");

@@ -3560,6 +3570,11 @@ target_scsi3_emulate_pr_out(struct se_cmd *cmd)
 	int spec_i_pt = 0, all_tg_pt = 0, unreg = 0;
 	sense_reason_t ret;
 
+	if (!dev->dev_attrib.emulate_pr) {
+		pr_err("failing PROUT with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	/*
 	 * Following spc2r20 5.5.1 Reservations overview:
 	 *

@@ -4045,6 +4060,11 @@ target_scsi3_emulate_pr_in(struct se_cmd *cmd)
 {
 	sense_reason_t ret;
 
+	if (!cmd->se_dev->dev_attrib.emulate_pr) {
+		pr_err("failing PRIN with emulate_pr disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
 	/*
 	 * Following spc2r20 5.5.1 Reservations overview:
 	 *

@@ -4097,6 +4117,8 @@ target_check_reservation(struct se_cmd *cmd)
 		return 0;
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return 0;
+	if (!dev->dev_attrib.emulate_pr)
+		return 0;
 
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)

include/target/target_core_base.h +2 -0

index 9f9f5902af38..06d88645938b 100644
@@ -87,6 +87,8 @@
 #define DA_EMULATE_3PC				1
 /* No Emulation for PSCSI by default */
 #define DA_EMULATE_ALUA				0
+/* Emulate SCSI2 RESERVE/RELEASE and Persistent Reservations by default */
+#define DA_EMULATE_PR				1
 /* Enforce SCSI Initiator Port TransportID with 'ISID' for PR */
 #define DA_ENFORCE_PR_ISIDS			1
 /* Force SPC-3 PR Activate Persistence across Target Power Loss */

include/target/target_core_base.h struct se_dev_attrib { · line 673

Quoted 7 lines

include/target/target_core_base.h +1 -0

@@ -665,6 +667,7 @@ struct se_dev_attrib {
 	int		emulate_tpws;
 	int		emulate_caw;
 	int		emulate_3pc;
+	int		emulate_pr;
 	int		pi_prot_format;
 	enum target_prot_type pi_prot_type;
 	enum target_prot_type hw_pi_prot_type;

You need to handle the passthrough case also, so you'll need to look at:

https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/target/target_core_device.c#L1179

In those cases you'll notice it calls some of the functions you changed to address new attribute, but the return code is overwritten.

-Bryant

Mike Christie

2018-06-01 16:22 UTC #2 lore raw

In reply to David Disseldorp

Quoted 4 lines · David Disseldorp

The new emulate_pr backstore attribute allows for Persistent Reservation and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be useful for scenarios such as:
- Enforcing ATS (Compare & Write) usage on recent VMware ESXi initiators

I was wondering why didn't you want your patch to be able to override the passthrough case like for pscsi when TRANSPORT_FLAG_PASSTHROUGH_PGR is set? It seems like for this ATS case you would want to do that for all backends.

For the ATS case would you ever need to separate the SCSI 2 RESERVE/RELEASE case from the modern PR case (a emulate_scsi2_pr and emulate_pr)? For example would you ever be using a device for ESX, want to do ATS only, but have datastores on the same device that are exported to VMs with apps that are going to use PRs so you would need (emulate_scsi2_prúlse, emulate_pr=true)?

Quoted 61 lines

- Allowing clustered (e.g. tcm-user) backends to block such requests, avoiding the multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive Invalid Command Operation Code response sense data.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
 drivers/target/target_core_device.c   |  1 +
 drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
 include/target/target_core_base.h     |  3 +++
 4 files changed, 42 insertions(+), 5 deletions(-)

drivers/target/target_core_configfs.c +7 -0

index 3f4bf126eed0..17b830e35a08 100644
@@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);

@@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);

@@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);

@@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_emulate_tpws,
 	&attr_emulate_caw,
 	&attr_emulate_3pc,
+	&attr_emulate_pr,
 	&attr_pi_prot_type,
 	&attr_hw_pi_prot_type,
 	&attr_pi_prot_format,

@@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "Passthrough\n");
 
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		ret = target_core_dev_pr_show_spc2_res(dev, page);

drivers/target/target_core_configfs.c static ssize_t target_pr_res_type_show(struct config_item *item, char *page) · line 1578

Quoted 6 lines

drivers/target/target_core_configfs.c +2 -1

@@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)
 
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "SPC_PASSTHROUGH\n");
-	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");

Do we want this formatted like the other strings here in caps with no spaces between words incase parsers are expecting that format for this file?

Quoted 10 lines

drivers/target/target_core_configfs.c +3 -2

+	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		return sprintf(page, "SPC2_RESERVATIONS\n");
-	else
-		return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+	return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,

drivers/target/target_core_configfs.c static ssize_t target_pr_res_aptpl_active_show(struct config_item *item, · line 1591

Quoted 6 lines

drivers/target/target_core_configfs.c +2 -1

@@ -1578,7 +1587,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)

We normally put the "||" on the line above it and the next line starts at the column after the opening "(" above it. I guess we sometimes tab over too, but we never tab way way over like above.

Same below.

Quoted 12 lines

drivers/target/target_core_configfs.c +2 -1

 		return 0;
 
 	return sprintf(page, "APTPL Bit Status: %s\n",

@@ -1590,7 +1600,8 @@ static ssize_t target_pr_res_aptpl_metadata_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)
 		return 0;

David Disseldorp

author

2018-06-03 12:57 UTC #3 lore raw

In reply to David Disseldorp

Quoted 10 lines · Mike Christie

Quoted 4 lines · David Disseldorp
The new emulate_pr backstore attribute allows for Persistent Reservation and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be useful for scenarios such as:
- Enforcing ATS (Compare & Write) usage on recent VMware ESXi initiators
I was wondering why didn't you want your patch to be able to override the passthrough case like for pscsi when TRANSPORT_FLAG_PASSTHROUGH_PGR is set? It seems like for this ATS case you would want to do that for all backends.

Agreed, I'll fix the passthrough case in the next revision.

Quoted 6 lines

For the ATS case would you ever need to separate the SCSI 2 RESERVE/RELEASE case from the modern PR case (a emulate_scsi2_pr and emulate_pr)? For example would you ever be using a device for ESX, want to do ATS only, but have datastores on the same device that are exported to VMs with apps that are going to use PRs so you would need (emulate_scsi2_prúlse, emulate_pr=true)?

I considered more granular toggles, but decided against it due to the added configuration/implementation complexity. Happy to revisit this if others would prefer it though.
Perhaps emulate_reservations would make more sense as a single toggle, given that RESERVE/RELEASE aren't "persistent", although the existing configfs paths seem to use "pr" for both types.

Quoted 62 lines

- Allowing clustered (e.g. tcm-user) backends to block such requests, avoiding the multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive Invalid Command Operation Code response sense data.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
 drivers/target/target_core_device.c   |  1 +
 drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
 include/target/target_core_base.h     |  3 +++
 4 files changed, 42 insertions(+), 5 deletions(-)

drivers/target/target_core_configfs.c +7 -0

index 3f4bf126eed0..17b830e35a08 100644
@@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);

@@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);

@@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);

@@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_emulate_tpws,
 	&attr_emulate_caw,
 	&attr_emulate_3pc,
+	&attr_emulate_pr,
 	&attr_pi_prot_type,
 	&attr_hw_pi_prot_type,
 	&attr_pi_prot_format,

@@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "Passthrough\n");
 
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		ret = target_core_dev_pr_show_spc2_res(dev, page);

@@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)

Quoted 8 lines

drivers/target/target_core_configfs.c static ssize_t target_pr_res_type_show(struct config_item *item, char *page) · line 1578
Quoted 5 lines
drivers/target/target_core_configfs.c +2 -1
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "SPC_PASSTHROUGH\n");
-	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");  
Do we want this formatted like the other strings here in caps with no spaces between words incase parsers are expecting that format for this file?

AFAICT, targetcli (via rtslib-fb) only makes use of the res_aptpl_metadata path. I'm not aware of any other consumers, so happy to go with whatever the community preference is here.

Quoted 21 lines

Quoted 10 lines
drivers/target/target_core_configfs.c +3 -2
+	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		return sprintf(page, "SPC2_RESERVATIONS\n");
-	else
-		return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+	return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
drivers/target/target_core_configfs.c static ssize_t target_pr_res_aptpl_active_show(struct config_item *item, · line 1591
Quoted 6 lines
drivers/target/target_core_configfs.c +2 -1
@@ -1578,7 +1587,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
 {
 	struct se_device *dev = pr_to_dev(item);
 
-	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
+						|| !dev->dev_attrib.emulate_pr)  
We normally put the "||" on the line above it and the next line starts at the column after the opening "(" above it. I guess we sometimes tab over too, but we never tab way way over like above.

Quoted 1 line

Same below.

Will fix these in the next round. Thanks for the feedback, Mike.

Cheers, David

Mike Christie

2018-06-04 17:13 UTC #4 lore raw

In reply to David Disseldorp

Quoted 22 lines · David Disseldorp

Quoted 10 lines · Mike Christie
Quoted 4 lines · David Disseldorp
The new emulate_pr backstore attribute allows for Persistent Reservation and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be useful for scenarios such as:
- Enforcing ATS (Compare & Write) usage on recent VMware ESXi initiators
I was wondering why didn't you want your patch to be able to override the passthrough case like for pscsi when TRANSPORT_FLAG_PASSTHROUGH_PGR is set? It seems like for this ATS case you would want to do that for all backends.
Agreed, I'll fix the passthrough case in the next revision.
Quoted 6 lines
For the ATS case would you ever need to separate the SCSI 2 RESERVE/RELEASE case from the modern PR case (a emulate_scsi2_pr and emulate_pr)? For example would you ever be using a device for ESX, want to do ATS only, but have datastores on the same device that are exported to VMs with apps that are going to use PRs so you would need (emulate_scsi2_prúlse, emulate_pr=true)?

Quoted 3 lines

I considered more granular toggles, but decided against it due to the added configuration/implementation complexity. Happy to revisit this if others would prefer it though.

I might not understand your original "enforce" part of the patch description, because it seems like you must do this.

I think ATS only is the default for the newer versions of ESX when the device reports it supports ATS, and it seems like running windows clustering in VMs on ESX is common. So, you will have that combo of ATS only with PRs a lot.

When you wrote "enforce" earlier I thought you meant the patch is supposed to make sure that ATS only is really on and if ESX messes up (or maybe the user messed up the settings) and it sends a RESERVE/RELEASE then it is failed. I do not see how that is possible with your patch with something like windows clustering VMs.

If the patch was more for protecting against the case where the backend does not support reservations, and ATS only was used to make sure they are never used due to that, and we assume backends always implement older reservations if they implement newer PRs then I think the patch is fine.

So I do not really care :) I think it depends on what you were trying to support.

Quoted 75 lines

Perhaps emulate_reservations would make more sense as a single toggle, given that RESERVE/RELEASE aren't "persistent", although the existing configfs paths seem to use "pr" for both types.

Quoted 62 lines

- Allowing clustered (e.g. tcm-user) backends to block such requests, avoiding the multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive Invalid Command Operation Code response sense data.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
 drivers/target/target_core_device.c   |  1 +
 drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
 include/target/target_core_base.h     |  3 +++
 4 files changed, 42 insertions(+), 5 deletions(-)

drivers/target/target_core_configfs.c +7 -0

index 3f4bf126eed0..17b830e35a08 100644
@@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);

@@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);

@@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);

@@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_emulate_tpws,
 	&attr_emulate_caw,
 	&attr_emulate_3pc,
+	&attr_emulate_pr,
 	&attr_pi_prot_type,
 	&attr_hw_pi_prot_type,
 	&attr_pi_prot_format,

@@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "Passthrough\n");
 
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");
+
 	spin_lock(&dev->dev_reservation_lock);
 	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
 		ret = target_core_dev_pr_show_spc2_res(dev, page);

@@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)

Quoted 8 lines

drivers/target/target_core_configfs.c static ssize_t target_pr_res_type_show(struct config_item *item, char *page) · line 1578
Quoted 5 lines
drivers/target/target_core_configfs.c +2 -1
 	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
 		return sprintf(page, "SPC_PASSTHROUGH\n");
-	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+	if (!dev->dev_attrib.emulate_pr)
+		return sprintf(page, "Reservations Disabled\n");  
Do we want this formatted like the other strings here in caps with no spaces between words incase parsers are expecting that format for this file?

Quoted 4 lines

AFAICT, targetcli (via rtslib-fb) only makes use of the res_aptpl_metadata path. I'm not aware of any other consumers, so happy to go with whatever the community preference is here.

I just seems odd to have a file use 2 different formats for no reason except there were 2 different people coding it.

David Disseldorp

author

2018-06-21 15:03 UTC #5 lore raw

In reply to David Disseldorp

Hi Mike,

Quoted 1 line · Mike Christie

On 06/03/2018 07:57 AM, David Disseldorp wrote:

...

Quoted 6 lines

Quoted 3 lines
I considered more granular toggles, but decided against it due to the added configuration/implementation complexity. Happy to revisit this if others would prefer it though.
I might not understand your original "enforce" part of the patch description, because it seems like you must do this.

I've changed "enforce" to "ensure".

Quoted 4 lines

I think ATS only is the default for the newer versions of ESX when the device reports it supports ATS, and it seems like running windows clustering in VMs on ESX is common. So, you will have that combo of ATS only with PRs a lot.

I don't think a combination of SCSI2 and persistent reservations is common, as VMware recommends Raw Device Mapping (via a separate LUN) for most Windows Failover Cluster use use cases[1]. See:
https://docs.vmware.com/en/VMware-vSphere/6.5/vsphere-esxi-vcenter-server-651-setup-mscs.pdf

IMO a single toggle to disable both RESERVE / RELEASE and Persistent Reservation handling for a given backstore is still the simplest and most user friendly option here.

Quoted 6 lines

When you wrote "enforce" earlier I thought you meant the patch is supposed to make sure that ATS only is really on and if ESX messes up (or maybe the user messed up the settings) and it sends a RESERVE/RELEASE then it is failed. I do not see how that is possible with your patch with something like windows clustering VMs.

Quoted 8 lines

If the patch was more for protecting against the case where the backend does not support reservations, and ATS only was used to make sure they are never used due to that, and we assume backends always implement older reservations if they implement newer PRs then I think the patch is fine.
So I do not really care :) I think it depends on what you were trying to support.

Some older versions of ESX/VMFS can fall back to using SCSI2 reservations instead of ATS[2]. Disabling LIO SCSI2+PR support restricts this fallback behaviour. That's what I was trying to get across in the commit message :)

New version to follow...

Cheers, David

1) Windows Failover Cluster on VMware
https://docs.vmware.com/en/VMware-vSphere/6.5/vsphere-esxi-vcenter-server-651-setup-mscs.pdf
2) vSphere 5 SCSI reservations vs ATS locking
https://pubs.vmware.com/vsphere-50/topic/com.vmware.vsphere.storage.doc_50/GUID-DE30AAE3-72ED-43BF-95B3-A2B885A713DB.html

David Disseldorp

author

2018-06-21 15:26 UTC #6 lore raw

In reply to David Disseldorp

Quoted 7 lines · Mike Christie

Quoted 4 lines
AFAICT, targetcli (via rtslib-fb) only makes use of the res_aptpl_metadata path. I'm not aware of any other consumers, so happy to go with whatever the community preference is here.
I just seems odd to have a file use 2 different formats for no reason except there were 2 different people coding it.

It's already a bit inconsistent, in that res_holder_show() returns human readable strings and res_type_show() opts for enum type strings. I've changed both to use "SPC_RESERVATIONS_DISABLED" for the disabled case for now.

Cheers, David

Mike Christie

2018-06-21 20:04 UTC #7 lore raw

In reply to David Disseldorp

Quoted 7 lines · David Disseldorp

Quoted 5 lines · Mike Christie
Quoted 4 lines
AFAICT, targetcli (via rtslib-fb) only makes use of the res_aptpl_metadata path. I'm not aware of any other consumers, so happy to go with whatever the community preference is here.

Quoted 8 lines

Quoted 2 lines
I just seems odd to have a file use 2 different formats for no reason except there were 2 different people coding it.
It's already a bit inconsistent, in that res_holder_show() returns human readable strings and res_type_show() opts for enum type strings. I've changed both to use "SPC_RESERVATIONS_DISABLED" for the disabled case for now.

I was only asking that we at least have the same format within a file, so we do not have messes like the backend info file.