On 4/10/2015 9:59 PM, Nicholas A. Bellinger wrote:
On Thu, 2015-04-09 at 17:45 -0400, Martin K. Petersen wrote:
"nab" == Nicholas A Bellinger <nab@xxxxxxxxxxxxxxx> writes:
How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
persistent?
nab> AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
cmd-> prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
nab> sbc_set_prot_op_checks() code.
nab> Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer
nab> is present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was
nab> cleared..? Or should the command be rejected when a protection
nab> buffer is present + RDPROTECT/WRPROTECT is non-zero if fabric_prot
nab> was cleared..?
Depends how compliant you want to be.
You can synthesize PI with RDPROTECT/WRPROTECT=1 as long as the
initiator doesn't rely on app tag escapes (we don't). Most non-HDD/SSD
targets work this way.
<nod>
I would suggest that you return invalid field in CDB for
RDPROTECT/WRPROTECT=3 unless the PI can be made persistent, however.
Ok, after thinking about this some more, here's what I've come up with..
The following incremental patch saves the current sess_prot_type into
se_node_acl, and will always reset sess_prot_type if a previous saved
value exists. So the PI setting for the fabric's session with backend
devices not supporting PI is persistent across session restart.
I also noticed the internal DIF emulation was not honoring
se_cmd->prot_checks for the WRPROTECT/RDPROTECT == 0x3 case, so
sbc_dif_v1_verify() has been updated to follow which checks have been
calculated based on WRPROTECT/RDPROTECT in sbc_set_prot_op_checks().
Finally in sbc_check_prot(), if PROTECT is non-zero for a backend device
with DIF disabled, and sess_prot_type is not set go ahead and return
INVALID_CDB_FIELD.
WDYT..?
--nab
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 315ff64..a75512f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -697,9 +697,13 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
pi_prot_type = cmd->se_sess->sess_prot_type;
break;
}
+ if (!protect)
+ return TCM_NO_SENSE;
/* Fallthrough */
default:
- return TCM_NO_SENSE;
+ pr_err("Unable to determine pi_prot_type for CDB: 0x%02x "
+ "PROTECT: 0x%02x\n", cdb[0], protect);
+ return TCM_INVALID_CDB_FIELD;
}
if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
@@ -1221,6 +1227,9 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
int block_size = dev->dev_attrib.block_size;
__be16 csum;
+ if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD))
+ goto check_ref;
+
csum = cpu_to_be16(crc_t10dif(p, block_size));
if (sdt->guard_tag != csum) {
@@ -1230,6 +1239,10 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
}
+check_ref:
+ if (!(cmd->prot_checks & TARGET_DIF_CHECK_REFTAG))
+ return 0;
+
if (cmd->prot_type == TARGET_DIF_TYPE1_PROT &&
be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
pr_err("DIFv1 Type 1 reference failed on sector: %llu tag: 0x%08x"
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f884198..3ff38b2 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -329,11 +329,17 @@ void __transport_register_session(
se_sess->fabric_sess_ptr = fabric_sess_ptr;
/*
* Determine if fabric allows for T10-PI feature bits to be exposed
- * to initiators for device backends with !dev->dev_attrib.pi_prot_type
+ * to initiators for device backends with !dev->dev_attrib.pi_prot_type.
+ *
+ * If so, then always save prot_type on a per se_node_acl node basis
+ * and re-instate the previous sess_prot_type to avoid disabling PI
+ * from below any previously initiator side registered LUNs.
*/
- if (tfo->tpg_check_prot_fabric_only)
- se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
-
+ if (se_nacl->saved_prot_type)
+ se_sess->sess_prot_type = se_nacl->saved_prot_type;
+ else if (tfo->tpg_check_prot_fabric_only)
+ se_sess->sess_prot_type = se_nacl->saved_prot_type =
+ tfo->tpg_check_prot_fabric_only(se_tpg);
/*
* Used by struct se_node_acl's under ConfigFS to locate active se_session-t
*
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 383110d..51dcf2b 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -590,6 +590,7 @@ struct se_node_acl {
bool acl_stop:1;
u32 queue_depth;
u32 acl_index;
+ enum target_prot_type saved_prot_type;
#define MAX_ACL_TAG_SIZE 64
char acl_tag[MAX_ACL_TAG_SIZE];
/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
This looks fine to me.
Acked-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
