[PATCH-v2,02/15] target: Add protected fabric + unprotected device support

Commit Message

Nicholas A. Bellinger March 30, 2015, 3:28 a.m. UTC

From: Nicholas Bellinger <[e-mail]>

This patch adds a new target_core_fabric_ops callback for allowing fabric
drivers to expose a TPG attribute for signaling when a T10-PI protected
fabric wants to function with an un-protected device without T10-PI.

This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
operations when functioning with non T10-PI enabled devices, seperate
from any available hw offloads the fabric supports.

This is done using a new se_sess->sess_prot_type that is set at fabric
session creation time based upon the TPG attribute.  It currently cannot
be changed for individual sessions after initial creation.

Also, update existing target_core_sbc.c code to honor sess_prot_type when
setting up cmd->prot_op + cmd->prot_type assignments.

Cc: Martin Petersen <[e-mail]>
Cc: Sagi Grimberg <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: Doug Gilbert <[e-mail]>
Signed-off-by: Nicholas Bellinger <[e-mail]>
---
 drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
 drivers/target/target_core_transport.c |  8 +++++++
 include/target/target_core_base.h      |  1 +
 include/target/target_core_fabric.h    |  8 +++++++
 4 files changed, 50 insertions(+), 11 deletions(-)

Comments

Sagi Grimberg March 30, 2015, 7:51 a.m. UTC | #1

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[e-mail]>
>
> This patch adds a new target_core_fabric_ops callback for allowing fabric
> drivers to expose a TPG attribute for signaling when a T10-PI protected
> fabric wants to function with an un-protected device without T10-PI.
>
> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> operations when functioning with non T10-PI enabled devices, seperate
> from any available hw offloads the fabric supports.
>
> This is done using a new se_sess->sess_prot_type that is set at fabric
> session creation time based upon the TPG attribute.  It currently cannot
> be changed for individual sessions after initial creation.
>
> Also, update existing target_core_sbc.c code to honor sess_prot_type when
> setting up cmd->prot_op + cmd->prot_type assignments.
>
> Cc: Martin Petersen <[e-mail]>
> Cc: Sagi Grimberg <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Doug Gilbert <[e-mail]>
> Signed-off-by: Nicholas Bellinger <[e-mail]>
> ---
>   drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
>   drivers/target/target_core_transport.c |  8 +++++++
>   include/target/target_core_base.h      |  1 +
>   include/target/target_core_fabric.h    |  8 +++++++
>   4 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 95a7a74..5b3564a 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
>   }
>
>   static int
> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
>   		       bool is_write, struct se_cmd *cmd)
>   {
>   	if (is_write) {
> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> -					 TARGET_PROT_DOUT_INSERT;
> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> +			       protect ? TARGET_PROT_DOUT_PASS :
> +			       TARGET_PROT_DOUT_INSERT;

In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
I think that the protect condition should come first.

>   		switch (protect) {
>   		case 0x0:
>   		case 0x3:
> @@ -610,8 +611,9 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>   			return -EINVAL;
>   		}
>   	} else {
> -		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
> -					 TARGET_PROT_DIN_STRIP;
> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
> +			       protect ? TARGET_PROT_DIN_PASS :
> +			       TARGET_PROT_DIN_STRIP;

Same here.

>   		switch (protect) {
>   		case 0x0:
>   		case 0x1:
> @@ -644,11 +646,15 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   	       u32 sectors, bool is_write)
>   {
>   	u8 protect = cdb[1] >> 5;
> +	int sp_ops = cmd->se_sess->sup_prot_ops;
> +	int pi_prot_type = dev->dev_attrib.pi_prot_type;
> +	bool fabric_prot = false;
>
>   	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
> -		if (protect && !dev->dev_attrib.pi_prot_type) {
> -			pr_err("CDB contains protect bit, but device does not"
> -			       " advertise PROTECT=1 feature bit\n");
> +		if (protect &&
> +		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
> +			pr_err("CDB contains protect bit, but device + fabric does"
> +			       " not advertise PROTECT=1 feature bit\n");

Can you place unlikely on these conditions?

>   			return TCM_INVALID_CDB_FIELD;
>   		}
>   		if (cmd->prot_pto)
> @@ -669,15 +675,28 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   		cmd->reftag_seed = cmd->t_task_lba;
>   		break;
>   	case TARGET_DIF_TYPE0_PROT:
> +		/*
> +		 * See if the fabric supports T10-PI, and the session has been
> +		 * configured to allow export PROTECT=1 feature bit with backend
> +		 * devices that don't support T10-PI.
> +		 */
> +		fabric_prot = is_write ?
> +			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :

Shouldn't this be converted to bool using !!()?


> +			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
> +
> +		if (fabric_prot && cmd->se_sess->sess_prot_type) {
> +			pi_prot_type = cmd->se_sess->sess_prot_type;
> +			break;
> +		}
> +		/* Fallthrough */
>   	default:
>   		return TCM_NO_SENSE;
>   	}
>
> -	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
> -				   is_write, cmd))
> +	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
>   		return TCM_INVALID_CDB_FIELD;
>
> -	cmd->prot_type = dev->dev_attrib.pi_prot_type;
> +	cmd->prot_type = pi_prot_type;
>   	cmd->prot_length = dev->prot_length * sectors;
>
>   	/**
> @@ -1231,6 +1250,9 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
>   	unsigned int i, len, left;
>   	unsigned int offset = sg_off;
>
> +	if (!sg)
> +		return;
> +
>   	left = sectors * dev->prot_length;
>
>   	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 4a00ed5..aef989e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -322,11 +322,19 @@ void __transport_register_session(
>   	struct se_session *se_sess,
>   	void *fabric_sess_ptr)
>   {
> +	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
>   	unsigned char buf[PR_REG_ISID_LEN];
>
>   	se_sess->se_tpg = se_tpg;
>   	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
> +	 */
> +	if (tfo->tpg_check_prot_fabric_only)
> +		se_sess->sess_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
>   	 *
>   	 * Only set for struct se_session's that will actually be moving I/O.
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 672150b..fe25a78 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -616,6 +616,7 @@ struct se_session {
>   	unsigned		sess_tearing_down:1;
>   	u64			sess_bin_isid;
>   	enum target_prot_op	sup_prot_ops;
> +	enum target_prot_type	sess_prot_type;
>   	struct se_node_acl	*se_node_acl;
>   	struct se_portal_group *se_tpg;
>   	void			*fabric_sess_ptr;
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 2f4a250..c93cfdf 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -27,6 +27,14 @@ struct target_core_fabric_ops {
>   	 * inquiry response
>   	 */
>   	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
> +	/*
> +	 * Optionally used as a configfs tunable to determine when
> +	 * target-core should signal the PROTECT=1 feature bit for
> +	 * backends that don't support T10-PI, so that either fabric
> +	 * HW offload or target-core emulation performs the associated
> +	 * WRITE_STRIP and READ_INSERT operations.
> +	 */
> +	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
>   	struct se_node_acl *(*tpg_alloc_fabric_acl)(
>   					struct se_portal_group *);
>   	void (*tpg_release_fabric_acl)(struct se_portal_group *,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nicholas A. Bellinger April 1, 2015, 5:49 a.m. UTC | #2

On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[e-mail]>
> >
> > This patch adds a new target_core_fabric_ops callback for allowing fabric
> > drivers to expose a TPG attribute for signaling when a T10-PI protected
> > fabric wants to function with an un-protected device without T10-PI.
> >
> > This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> > operations when functioning with non T10-PI enabled devices, seperate
> > from any available hw offloads the fabric supports.
> >
> > This is done using a new se_sess->sess_prot_type that is set at fabric
> > session creation time based upon the TPG attribute.  It currently cannot
> > be changed for individual sessions after initial creation.
> >
> > Also, update existing target_core_sbc.c code to honor sess_prot_type when
> > setting up cmd->prot_op + cmd->prot_type assignments.
> >
> > Cc: Martin Petersen <[e-mail]>
> > Cc: Sagi Grimberg <[e-mail]>
> > Cc: Christoph Hellwig <[e-mail]>
> > Cc: Doug Gilbert <[e-mail]>
> > Signed-off-by: Nicholas Bellinger <[e-mail]>
> > ---
> >   drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
> >   drivers/target/target_core_transport.c |  8 +++++++
> >   include/target/target_core_base.h      |  1 +
> >   include/target/target_core_fabric.h    |  8 +++++++
> >   4 files changed, 50 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 95a7a74..5b3564a 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
> >   }
> >
> >   static int
> > -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> > +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
> >   		       bool is_write, struct se_cmd *cmd)
> >   {
> >   	if (is_write) {
> > -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> > -					 TARGET_PROT_DOUT_INSERT;
> > +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> > +			       protect ? TARGET_PROT_DOUT_PASS :
> > +			       TARGET_PROT_DOUT_INSERT;
> 
> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
> I think that the protect condition should come first.
> 

Mmm, not sure I follow..

sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
SGLs are present and se_dev does not accept PI, regardless of protect.

For the protect=1 and fabric_prot=1 case, prot_op is set to
TARGET_PROT_DOUT_STRIP requesting the WRITE_STRIP operation by either HW
fabric offload or target DIX software emulation because the backend
device cannot accept PI.

> >   		switch (protect) {
> >   		case 0x0:
> >   		case 0x3:
> > @@ -610,8 +611,9 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> >   			return -EINVAL;
> >   		}
> >   	} else {
> > -		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
> > -					 TARGET_PROT_DIN_STRIP;
> > +		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
> > +			       protect ? TARGET_PROT_DIN_PASS :
> > +			       TARGET_PROT_DIN_STRIP;
> 
> Same here.
> 
> >   		switch (protect) {
> >   		case 0x0:
> >   		case 0x1:
> > @@ -644,11 +646,15 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
> >   	       u32 sectors, bool is_write)
> >   {
> >   	u8 protect = cdb[1] >> 5;
> > +	int sp_ops = cmd->se_sess->sup_prot_ops;
> > +	int pi_prot_type = dev->dev_attrib.pi_prot_type;
> > +	bool fabric_prot = false;
> >
> >   	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
> > -		if (protect && !dev->dev_attrib.pi_prot_type) {
> > -			pr_err("CDB contains protect bit, but device does not"
> > -			       " advertise PROTECT=1 feature bit\n");
> > +		if (protect &&
> > +		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
> > +			pr_err("CDB contains protect bit, but device + fabric does"
> > +			       " not advertise PROTECT=1 feature bit\n");
> 
> Can you place unlikely on these conditions?
> 

Done.

> >   			return TCM_INVALID_CDB_FIELD;
> >   		}
> >   		if (cmd->prot_pto)
> > @@ -669,15 +675,28 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
> >   		cmd->reftag_seed = cmd->t_task_lba;
> >   		break;
> >   	case TARGET_DIF_TYPE0_PROT:
> > +		/*
> > +		 * See if the fabric supports T10-PI, and the session has been
> > +		 * configured to allow export PROTECT=1 feature bit with backend
> > +		 * devices that don't support T10-PI.
> > +		 */
> > +		fabric_prot = is_write ?
> > +			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :
> 
> Shouldn't this be converted to bool using !!()?
> 
> 

Fixed.

> > +			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
> > +
> > +		if (fabric_prot && cmd->se_sess->sess_prot_type) {
> > +			pi_prot_type = cmd->se_sess->sess_prot_type;
> > +			break;
> > +		}
> > +		/* Fallthrough */
> >   	default:
> >   		return TCM_NO_SENSE;
> >   	}
> >
> > -	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
> > -				   is_write, cmd))
> > +	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
> >   		return TCM_INVALID_CDB_FIELD;
> >
> > -	cmd->prot_type = dev->dev_attrib.pi_prot_type;
> > +	cmd->prot_type = pi_prot_type;
> >   	cmd->prot_length = dev->prot_length * sectors;
> >
> >   	/**
> > @@ -1231,6 +1250,9 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
> >   	unsigned int i, len, left;
> >   	unsigned int offset = sg_off;
> >
> > +	if (!sg)
> > +		return;
> > +
> >   	left = sectors * dev->prot_length;
> >
> >   	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 4a00ed5..aef989e 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -322,11 +322,19 @@ void __transport_register_session(
> >   	struct se_session *se_sess,
> >   	void *fabric_sess_ptr)
> >   {
> > +	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
> >   	unsigned char buf[PR_REG_ISID_LEN];
> >
> >   	se_sess->se_tpg = se_tpg;
> >   	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
> > +	 */
> > +	if (tfo->tpg_check_prot_fabric_only)
> > +		se_sess->sess_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
> >   	 *
> >   	 * Only set for struct se_session's that will actually be moving I/O.
> > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> > index 672150b..fe25a78 100644
> > --- a/include/target/target_core_base.h
> > +++ b/include/target/target_core_base.h
> > @@ -616,6 +616,7 @@ struct se_session {
> >   	unsigned		sess_tearing_down:1;
> >   	u64			sess_bin_isid;
> >   	enum target_prot_op	sup_prot_ops;
> > +	enum target_prot_type	sess_prot_type;
> >   	struct se_node_acl	*se_node_acl;
> >   	struct se_portal_group *se_tpg;
> >   	void			*fabric_sess_ptr;
> > diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> > index 2f4a250..c93cfdf 100644
> > --- a/include/target/target_core_fabric.h
> > +++ b/include/target/target_core_fabric.h
> > @@ -27,6 +27,14 @@ struct target_core_fabric_ops {
> >   	 * inquiry response
> >   	 */
> >   	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
> > +	/*
> > +	 * Optionally used as a configfs tunable to determine when
> > +	 * target-core should signal the PROTECT=1 feature bit for
> > +	 * backends that don't support T10-PI, so that either fabric
> > +	 * HW offload or target-core emulation performs the associated
> > +	 * WRITE_STRIP and READ_INSERT operations.
> > +	 */
> > +	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
> >   	struct se_node_acl *(*tpg_alloc_fabric_acl)(
> >   					struct se_portal_group *);
> >   	void (*tpg_release_fabric_acl)(struct se_portal_group *,
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sagi Grimberg April 1, 2015, 9:04 a.m. UTC | #3

On 4/1/2015 8:49 AM, Nicholas A. Bellinger wrote:
> On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
>> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <[e-mail]>
>>>
>>> This patch adds a new target_core_fabric_ops callback for allowing fabric
>>> drivers to expose a TPG attribute for signaling when a T10-PI protected
>>> fabric wants to function with an un-protected device without T10-PI.
>>>
>>> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
>>> operations when functioning with non T10-PI enabled devices, seperate
>>> from any available hw offloads the fabric supports.
>>>
>>> This is done using a new se_sess->sess_prot_type that is set at fabric
>>> session creation time based upon the TPG attribute.  It currently cannot
>>> be changed for individual sessions after initial creation.
>>>
>>> Also, update existing target_core_sbc.c code to honor sess_prot_type when
>>> setting up cmd->prot_op + cmd->prot_type assignments.
>>>
>>> Cc: Martin Petersen <[e-mail]>
>>> Cc: Sagi Grimberg <[e-mail]>
>>> Cc: Christoph Hellwig <[e-mail]>
>>> Cc: Doug Gilbert <[e-mail]>
>>> Signed-off-by: Nicholas Bellinger <[e-mail]>
>>> ---
>>>    drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
>>>    drivers/target/target_core_transport.c |  8 +++++++
>>>    include/target/target_core_base.h      |  1 +
>>>    include/target/target_core_fabric.h    |  8 +++++++
>>>    4 files changed, 50 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
>>> index 95a7a74..5b3564a 100644
>>> --- a/drivers/target/target_core_sbc.c
>>> +++ b/drivers/target/target_core_sbc.c
>>> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
>>>    }
>>>
>>>    static int
>>> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>>> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
>>>    		       bool is_write, struct se_cmd *cmd)
>>>    {
>>>    	if (is_write) {
>>> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
>>> -					 TARGET_PROT_DOUT_INSERT;
>>> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
>>> +			       protect ? TARGET_PROT_DOUT_PASS :
>>> +			       TARGET_PROT_DOUT_INSERT;
>>
>> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
>> I think that the protect condition should come first.
>>
>
> Mmm, not sure I follow..
>
> sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
> SGLs are present and se_dev does not accept PI, regardless of protect.
>

It's a little confusing that fabric_prot is set if the backend device
does not support PI.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nicholas A. Bellinger April 2, 2015, 4:40 a.m. UTC | #4

On Wed, 2015-04-01 at 12:04 +0300, Sagi Grimberg wrote:
> On 4/1/2015 8:49 AM, Nicholas A. Bellinger wrote:
> > On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
> >> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger <[e-mail]>
> >>>
> >>> This patch adds a new target_core_fabric_ops callback for allowing fabric
> >>> drivers to expose a TPG attribute for signaling when a T10-PI protected
> >>> fabric wants to function with an un-protected device without T10-PI.
> >>>
> >>> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> >>> operations when functioning with non T10-PI enabled devices, seperate
> >>> from any available hw offloads the fabric supports.
> >>>
> >>> This is done using a new se_sess->sess_prot_type that is set at fabric
> >>> session creation time based upon the TPG attribute.  It currently cannot
> >>> be changed for individual sessions after initial creation.
> >>>
> >>> Also, update existing target_core_sbc.c code to honor sess_prot_type when
> >>> setting up cmd->prot_op + cmd->prot_type assignments.
> >>>
> >>> Cc: Martin Petersen <[e-mail]>
> >>> Cc: Sagi Grimberg <[e-mail]>
> >>> Cc: Christoph Hellwig <[e-mail]>
> >>> Cc: Doug Gilbert <[e-mail]>
> >>> Signed-off-by: Nicholas Bellinger <[e-mail]>
> >>> ---
> >>>    drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
> >>>    drivers/target/target_core_transport.c |  8 +++++++
> >>>    include/target/target_core_base.h      |  1 +
> >>>    include/target/target_core_fabric.h    |  8 +++++++
> >>>    4 files changed, 50 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> >>> index 95a7a74..5b3564a 100644
> >>> --- a/drivers/target/target_core_sbc.c
> >>> +++ b/drivers/target/target_core_sbc.c
> >>> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
> >>>    }
> >>>
> >>>    static int
> >>> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> >>> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
> >>>    		       bool is_write, struct se_cmd *cmd)
> >>>    {
> >>>    	if (is_write) {
> >>> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> >>> -					 TARGET_PROT_DOUT_INSERT;
> >>> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> >>> +			       protect ? TARGET_PROT_DOUT_PASS :
> >>> +			       TARGET_PROT_DOUT_INSERT;
> >>
> >> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
> >> I think that the protect condition should come first.
> >>
> >
> > Mmm, not sure I follow..
> >
> > sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
> > SGLs are present and se_dev does not accept PI, regardless of protect.
> >
> 
> It's a little confusing that fabric_prot is set if the backend device
> does not support PI.

Well, it's supposed to signal that fabric supports PI, but the backend
device does not.

Seems like a reasonable name to me..  Any ideas for a better one..?  ;)

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen April 7, 2015, 11:27 p.m. UTC | #5

>>>>> "nab" == Nicholas A Bellinger <[e-mail]> writes:

nab> This specifically is to allow LIO to perform WRITE_STRIP +
nab> READ_INSERT operations when functioning with non T10-PI enabled
nab> devices, seperate from any available hw offloads the fabric
nab> supports.

How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
persistent?

Nicholas A. Bellinger April 8, 2015, 7:40 a.m. UTC | #6

On Tue, 2015-04-07 at 19:27 -0400, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <[e-mail]> writes:
> 
> nab> This specifically is to allow LIO to perform WRITE_STRIP +
> nab> READ_INSERT operations when functioning with non T10-PI enabled
> nab> devices, seperate from any available hw offloads the fabric
> nab> supports.
> 
> How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
> persistent?
> 

AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
cmd->prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
sbc_set_prot_op_checks() code.

Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer is
present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was cleared..?  
Or should the command be rejected when a protection buffer is present +
RDPROTECT/WRPROTECT is non-zero if fabric_prot was cleared..?

Also wrt to PI persistence across session restart, one way it can work
is to store the last se_sess->sess_prot_type in se_node_acl, and
reinstate the previous setting across session restart.  This would allow
new sessions to pickup the latest fabric_prot_type endpoint attribute
value, but make existing ones with PI enabled keep their previous
sess_prot_type settings.

WDYT..? 

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen April 9, 2015, 9:45 p.m. UTC | #7

>>>>> "nab" == Nicholas A Bellinger <[e-mail]> 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.

I would suggest that you return invalid field in CDB for
RDPROTECT/WRPROTECT=3 unless the PI can be made persistent, however.

Patch

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 95a7a74..5b3564a 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -581,12 +581,13 @@  sbc_compare_and_write(struct se_cmd *cmd)
 }
 
 static int
-sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
+sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
 		       bool is_write, struct se_cmd *cmd)
 {
 	if (is_write) {
-		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
-					 TARGET_PROT_DOUT_INSERT;
+		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
+			       protect ? TARGET_PROT_DOUT_PASS :
+			       TARGET_PROT_DOUT_INSERT;
 		switch (protect) {
 		case 0x0:
 		case 0x3:
@@ -610,8 +611,9 @@  sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
 			return -EINVAL;
 		}
 	} else {
-		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
-					 TARGET_PROT_DIN_STRIP;
+		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
+			       protect ? TARGET_PROT_DIN_PASS :
+			       TARGET_PROT_DIN_STRIP;
 		switch (protect) {
 		case 0x0:
 		case 0x1:
@@ -644,11 +646,15 @@  sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 	       u32 sectors, bool is_write)
 {
 	u8 protect = cdb[1] >> 5;
+	int sp_ops = cmd->se_sess->sup_prot_ops;
+	int pi_prot_type = dev->dev_attrib.pi_prot_type;
+	bool fabric_prot = false;
 
 	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
-		if (protect && !dev->dev_attrib.pi_prot_type) {
-			pr_err("CDB contains protect bit, but device does not"
-			       " advertise PROTECT=1 feature bit\n");
+		if (protect &&
+		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
+			pr_err("CDB contains protect bit, but device + fabric does"
+			       " not advertise PROTECT=1 feature bit\n");
 			return TCM_INVALID_CDB_FIELD;
 		}
 		if (cmd->prot_pto)
@@ -669,15 +675,28 @@  sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 		cmd->reftag_seed = cmd->t_task_lba;
 		break;
 	case TARGET_DIF_TYPE0_PROT:
+		/*
+		 * See if the fabric supports T10-PI, and the session has been
+		 * configured to allow export PROTECT=1 feature bit with backend
+		 * devices that don't support T10-PI.
+		 */
+		fabric_prot = is_write ?
+			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :
+			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
+
+		if (fabric_prot && cmd->se_sess->sess_prot_type) {
+			pi_prot_type = cmd->se_sess->sess_prot_type;
+			break;
+		}
+		/* Fallthrough */
 	default:
 		return TCM_NO_SENSE;
 	}
 
-	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
-				   is_write, cmd))
+	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
 		return TCM_INVALID_CDB_FIELD;
 
-	cmd->prot_type = dev->dev_attrib.pi_prot_type;
+	cmd->prot_type = pi_prot_type;
 	cmd->prot_length = dev->prot_length * sectors;
 
 	/**
@@ -1231,6 +1250,9 @@  sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 	unsigned int i, len, left;
 	unsigned int offset = sg_off;
 
+	if (!sg)
+		return;
+
 	left = sectors * dev->prot_length;
 
 	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4a00ed5..aef989e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -322,11 +322,19 @@  void __transport_register_session(
 	struct se_session *se_sess,
 	void *fabric_sess_ptr)
 {
+	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
 	unsigned char buf[PR_REG_ISID_LEN];
 
 	se_sess->se_tpg = se_tpg;
 	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
+	 */
+	if (tfo->tpg_check_prot_fabric_only)
+		se_sess->sess_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
 	 *
 	 * Only set for struct se_session's that will actually be moving I/O.
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 672150b..fe25a78 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -616,6 +616,7 @@  struct se_session {
 	unsigned		sess_tearing_down:1;
 	u64			sess_bin_isid;
 	enum target_prot_op	sup_prot_ops;
+	enum target_prot_type	sess_prot_type;
 	struct se_node_acl	*se_node_acl;
 	struct se_portal_group *se_tpg;
 	void			*fabric_sess_ptr;
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 2f4a250..c93cfdf 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -27,6 +27,14 @@  struct target_core_fabric_ops {
 	 * inquiry response
 	 */
 	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
+	/*
+	 * Optionally used as a configfs tunable to determine when
+	 * target-core should signal the PROTECT=1 feature bit for
+	 * backends that don't support T10-PI, so that either fabric
+	 * HW offload or target-core emulation performs the associated
+	 * WRITE_STRIP and READ_INSERT operations.
+	 */
+	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
 	struct se_node_acl *(*tpg_alloc_fabric_acl)(
 					struct se_portal_group *);
 	void (*tpg_release_fabric_acl)(struct se_portal_group *,