On 06/26/2018 07:35 AM, Bodo Stroesser wrote:
> On 06/25/18 20:56, 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:
>> - Ensuring 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.
>
> Currently I'm working on changes in the same area. But in my case we
> would need an attribute to optionally allow pr and alua CDBs to be
> passed to user space via tcm-user transparently. I already have the
> patches prepared but didn't send them yet as an important detail
> still is missing. (See details below)
> In general my patches change the existing 'pr/alua_support'
> attributes to be writable.
>
> If this patch and the mine would be accepted, we would have three
> different modes for pr handling: the normal in stack handling, the
> reject of pr CDBs and transparent pass through. The former two being
> available for all backstore devices but the latter for devices using
> passthrough_parse_cdb() only.
>
> Obviously it would be great to have only one attribute to control
> emulation mode. That would be easier if we had two different modes
> only for any dev.
>
> Now I'm wondering whether the "reject pr CDBs" mode does make sense
> for e.g. tcm-user. Wouldn't it be enough to have the emulation
> in stack and the transparent mode only? In that case userspace process
> simply could use transparent mode and send a reject for these CDBs.
>
> So I'd like to propose the following:
> 1) Have an attribute to switch between normal mode (pr_support active)
> and new mode (pr_support inactive).
> Whether this is done via the existing pr_support attribute or via
> a new attribute is a question of flavor.
> 2) If pr_support is inactive, spc_parse_cdb() would reject PR CDBs
> while passthrough_parse_cdb() would pass through those CDBs to
> userspace.
> 3) In the "pr_support inactive" mode for devices using
> passthrough_parse_cdb() the transport_flag
> TRANSPORT_FLAG_PASSTHROUGH_PGR would be set. For devices using
> spc_parse_cdb() a new flag like TRANSPORT_FLAG_REJECT_PGR should be
> used. (See details below regarding dev specific transport_flags.)
I am not sure I got this. How do apps like targetcli that manage tcmu
with rtslib tell the tcmu userspace daemon if it should reject PGRs? I
think you need 2 device attributes or 1 with a 3rd state:
1. emulate_pr - Controls whether PGRs are executed or failed. Does not
matter if in kernel or userspace.
2. pgr_support - Controls if PGRs are executed in the LIO PGR layer in
kernel or passed to the backend driver (up to userspace for tcmu or to
the physical device for pscsi).
So with tcmu and {emulate_pr=0 pgr_support=0} would be pass to userspace
and fail PGRs.
> 4) The same could be done for ALUA also.
Do we need all this for ALUA? I'm not sure anyone is going to use it now.
You can already enable/disable ALUA for devices that support it.
For pscsi there does not seem to be any users requesting LIO support, so
there is no one to fix issues like the lu group bug or where the real
device reports different info, like the tpgs value than what the LIO
alua layer got configured for.
For tcmu the primary benefit would be that you can do ALUA commands in
kernel and it does not have the issue where userspace does not know the
target port info. It sounded like you were going to fix that issue.
If there are still people trying to modify tcmu so it can do READ/WRITE
type of commands in userpsace and everything else in kernel then I would
just let them handle ALUA.
>
> What do you think?
>
> --Bodo
>
>
> Details of my patches:
>
> Currently the transport_flags are per transport template, while the
> pr_support and alua_support attributes are per backstore device.
> So my patches add a per se_dev copy of the transport_flags which is
> initialized on device creation and which replaces all current uses
> of the transport_flags.
> Next I make the pr_/alua_support attributes writable, but have an
> additional field in the transport template that allows to reject
> changes depending on the transport. By setting bits in the additional
> field of tcm-user's transport template only, changing the
> pr_/alua_support attributes would be possible for tcm_user, but not
> pscsi for the moment.
>
> While the patches described above are ready, I'm still developing a
> further change:
> In the pr/alua transparent mode userspace processes should not only
> be able to reject PR/ALUA CDBs but also to process them correctly.
> Therefore userspace process optionally needs the I_T_NEXUS info for
> each CDB.
> My plan was to first finish this last patch before sending the entire
> series. But now I could send the first part earlier.
>
> I'm adding Mike Christie on CC, as he already gave me some very
> helpful hints for my patches.
>
>> Signed-off-by: David Disseldorp <ddiss@xxxxxxx>
>> ---
>> drivers/target/target_core_configfs.c | 32
>> ++++++++++++++++++++++++--------
>> drivers/target/target_core_device.c | 13 +++++++++++++
>> drivers/target/target_core_pr.c | 2 ++
>> drivers/target/target_core_spc.c | 8 ++++++++
>> include/target/target_core_base.h | 3 +++
>> 5 files changed, 50 insertions(+), 8 deletions(-)
>>
>> Changes since v2:
>> * handle target_pr_res_aptpl_metadata_store()
>> * use common error path for spc_parse_cdb() and passthrough_parse_cdb()
>> checks
>> * drop erroneous TRANSPORT_FLAG_PASSTHROUGH_PGR ->
>> TRANSPORT_FLAG_PASSTHROUGH changes
>>
>> Changes since v1:
>> * block Reservation request passthrough when emulate_pr=0
>> * fix some style issues
>> * add an emulate_pr check to pgr_support_show()
>>
>> diff --git a/drivers/target/target_core_configfs.c
>> b/drivers/target/target_core_configfs.c
>> index 5ccef7d597fa..f7385a6950e4 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -532,6 +532,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);
>> @@ -592,6 +593,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);
>> @@ -1100,9 +1102,13 @@ static ssize_t pgr_support_show(struct
>> config_item *item, char *page)
>> {
>> struct se_dev_attrib *da = to_attrib(item);
>> u8 flags = da->da_dev->transport->transport_flags;
>> + int pgr_support = 1;
>> - return snprintf(page, PAGE_SIZE, "%d\n",
>> - flags & TRANSPORT_FLAG_PASSTHROUGH_PGR ? 0 : 1);
>> + if (!da->da_dev->dev_attrib.emulate_pr ||
>> + (flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
>> + pgr_support = 0;
>> +
>> + return snprintf(page, PAGE_SIZE, "%d\n", pgr_support);
>> }
>> CONFIGFS_ATTR(, emulate_model_alias);
>> @@ -1116,6 +1122,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);
>> @@ -1156,6 +1163,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,
>> @@ -1427,6 +1435,9 @@ static ssize_t target_pr_res_holder_show(struct
>> config_item *item, char *page)
>> struct se_device *dev = pr_to_dev(item);
>> int ret;
>> + if (!dev->dev_attrib.emulate_pr)
>> + return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
>> +
>> if (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR)
>> return sprintf(page, "Passthrough\n");
>> @@ -1567,12 +1578,14 @@ static ssize_t
>> target_pr_res_type_show(struct config_item *item, char *page)
>> {
>> struct se_device *dev = pr_to_dev(item);
>> + if (!dev->dev_attrib.emulate_pr)
>> + return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
>> 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_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,
>> @@ -1580,7 +1593,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->dev_attrib.emulate_pr ||
>> + (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR))
>> return 0;
>> return sprintf(page, "APTPL Bit Status: %s\n",
>> @@ -1592,7 +1606,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->dev_attrib.emulate_pr ||
>> + (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR))
>> return 0;
>> return sprintf(page, "Ready to process PR APTPL metadata..\n");
>> @@ -1638,7 +1653,8 @@ static ssize_t
>> target_pr_res_aptpl_metadata_store(struct config_item *item,
>> u16 tpgt = 0;
>> u8 type = 0;
>> - if (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR)
>> + if (!dev->dev_attrib.emulate_pr ||
>> + (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR))
>> return count;
>> if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
>> return count;
>> diff --git a/drivers/target/target_core_device.c
>> b/drivers/target/target_core_device.c
>> index e27db4d45a9d..e443ce5bf311 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;
>> @@ -1172,6 +1173,18 @@ passthrough_parse_cdb(struct se_cmd *cmd,
>> }
>> /*
>> + * With emulate_pr disabled, all reservation requests should fail,
>> + * regardless of whether or not TRANSPORT_FLAG_PASSTHROUGH_PGR is
>> set.
>> + */
>> + if (!dev->dev_attrib.emulate_pr &&
>> + ((cdb[0] == PERSISTENT_RESERVE_IN) ||
>> + (cdb[0] == PERSISTENT_RESERVE_OUT) ||
>> + (cdb[0] == RELEASE || cdb[0] == RELEASE_10) ||
>> + (cdb[0] == RESERVE || cdb[0] == RESERVE_10))) {
>> + return TCM_UNSUPPORTED_SCSI_OPCODE;
>> + }
>> +
>> + /*
>> * For PERSISTENT RESERVE IN/OUT, RELEASE, and RESERVE we need to
>> * emulate the response, since tcmu does not have the information
>> * required to process these commands.
>> diff --git a/drivers/target/target_core_pr.c
>> b/drivers/target/target_core_pr.c
>> index 01ac306131c1..4ef516e168b4 100644
>> --- a/drivers/target/target_core_pr.c
>> +++ b/drivers/target/target_core_pr.c
>> @@ -4090,6 +4090,8 @@ target_check_reservation(struct se_cmd *cmd)
>> return 0;
>> if (dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE)
>> return 0;
>> + if (!dev->dev_attrib.emulate_pr)
>> + return 0;
>> if (dev->transport->transport_flags &
>> TRANSPORT_FLAG_PASSTHROUGH_PGR)
>> return 0;
>> diff --git a/drivers/target/target_core_spc.c
>> b/drivers/target/target_core_spc.c
>> index cb0461a10808..4ad05c8415c8 100644
>> --- a/drivers/target/target_core_spc.c
>> +++ b/drivers/target/target_core_spc.c
>> @@ -1281,6 +1281,14 @@ spc_parse_cdb(struct se_cmd *cmd, unsigned int
>> *size)
>> struct se_device *dev = cmd->se_dev;
>> unsigned char *cdb = cmd->t_task_cdb;
>> + if (!dev->dev_attrib.emulate_pr &&
>> + ((cdb[0] == PERSISTENT_RESERVE_IN) ||
>> + (cdb[0] == PERSISTENT_RESERVE_OUT) ||
>> + (cdb[0] == RELEASE || cdb[0] == RELEASE_10) ||
>> + (cdb[0] == RESERVE || cdb[0] == RESERVE_10))) {
>> + return TCM_UNSUPPORTED_SCSI_OPCODE;
>> + }
>> +
>> switch (cdb[0]) {
>> case MODE_SELECT:
>> *size = cdb[4];
>> diff --git a/include/target/target_core_base.h
>> b/include/target/target_core_base.h
>> index 922a39f45abc..bca3713e0658 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 */
>> @@ -666,6 +668,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;
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
