[RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[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
- 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 <ddiss@suse.de>
---
 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(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 3f4bf126eed0..17b830e35a08 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -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");
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index e27db4d45a9d..96a60072e446 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -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;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 2e865fdaa362..9960a44c3a59 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -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)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 9f9f5902af38..06d88645938b 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -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;
-- 
2.13.6

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[Bryant G. Ly]

On 5/31/18 7:05 PM, David Disseldorp wrote:

> 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 <ddiss@suse.de>
> ---
>  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(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 3f4bf126eed0..17b830e35a08 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -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");
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index e27db4d45a9d..96a60072e446 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -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;
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 2e865fdaa362..9960a44c3a59 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -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)
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 9f9f5902af38..06d88645938b 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -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;

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

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[Mike Christie]

On 05/31/2018 07:05 PM, David Disseldorp wrote:
> 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)?


> - 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 <ddiss@suse.de>
> ---
>  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(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 3f4bf126eed0..17b830e35a08 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -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");

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?

> +	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)

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.

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

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

On Fri, 1 Jun 2018 11:22:50 -0500, Mike Christie wrote:

> On 05/31/2018 07:05 PM, David Disseldorp wrote:
> > 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.

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

> > - 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 <ddiss@suse.de>
> > ---
> >  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(-)
> > 
> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> > index 3f4bf126eed0..17b830e35a08 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -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");  
> 
> 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.

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

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

Cheers, David

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[Mike Christie]

On 06/03/2018 07:57 AM, David Disseldorp wrote:
> On Fri, 1 Jun 2018 11:22:50 -0500, Mike Christie wrote:
> 
>> On 05/31/2018 07:05 PM, David Disseldorp wrote:
>>> 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.
> 
>> 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.

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.


> 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.
> 
>>> - 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 <ddiss@suse.de>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>>> index 3f4bf126eed0..17b830e35a08 100644
>>> --- a/drivers/target/target_core_configfs.c
>>> +++ b/drivers/target/target_core_configfs.c
>>> @@ -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");  
>>
>> 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.
> 

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

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

Hi Mike,

On Mon, 4 Jun 2018 12:13:22 -0500, Mike Christie wrote:

> On 06/03/2018 07:57 AM, David Disseldorp wrote:
...
> > 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".

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

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

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

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

On Mon, 4 Jun 2018 12:13:22 -0500, Mike Christie wrote:

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

Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[Mike Christie]

On 06/21/2018 10:26 AM, David Disseldorp wrote:
> On Mon, 4 Jun 2018 12:13:22 -0500, Mike Christie wrote:
> 
>>> 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.
> 

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.