[PATCH RFC 0/4] add T10-Dif registration for tcm_qla2xxx

[PATCH RFC 0/4] add T10-Dif registration for tcm_qla2xxx

[Quinn Tran]

Nicholas,

Per our conversation at LSF, the following are the patch set
of bug fix/tweak found during T10-Dif testing and add ability
for QLogic FC driver to register with TCM its T10-PI capabilities.  

As for the rest of the other patches that does the actual
T10-Dif data movement, I will submit them to target-devel and
stable kernel.

Quinn
-----
Quinn Tran (4):
  target/core: T10-Dif: check HW support capabilities
  tcm_qla2xxx: T10-Dif set harware capability
  target/rd: T10-Dif: Add init/format support
  target/rd: T10-Dif: RAM disk is allocating more space than required.

 drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 23 +++++++++
 drivers/scsi/qla2xxx/tcm_qla2xxx.h           |  1 +
 drivers/target/target_core_fabric_configfs.c | 20 ++++++++
 drivers/target/target_core_rd.c              | 70 +++++++++++++++++++++++++++-
 drivers/target/target_core_tpg.c             |  9 ++++
 include/target/target_core_base.h            | 14 ++++++
 include/target/target_core_fabric.h          |  1 +
 7 files changed, 136 insertions(+), 2 deletions(-)

-- 
1.8.4.GIT

[PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Quinn Tran]

Check lower layer/HW support of T10-dif capability.
When the LUN is linked between the underlining fabric
and back end device, the Protection Type(1,2,3) is
check against each other to make sure both side are
capable of supporting the same protection setting.

ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
/sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
---
 drivers/target/target_core_fabric_configfs.c | 20 ++++++++++++++++++++
 drivers/target/target_core_tpg.c             |  9 +++++++++
 include/target/target_core_base.h            | 14 ++++++++++++++
 include/target/target_core_fabric.h          |  1 +
 4 files changed, 44 insertions(+)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f04..3ce07ec 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -777,6 +777,26 @@ static int target_fabric_port_link(
 				struct se_portal_group, tpg_group);
 	tf = se_tpg->se_tpg_wwn->wwn_tf;
 
+
+	if (dev->dev_attrib.pi_prot_type) {
+		uint32_t cap[] = { 0,
+				   TARGET_DIF_TYPE1_PROTECTION,
+				   TARGET_DIF_TYPE2_PROTECTION,
+				   TARGET_DIF_TYPE3_PROTECTION};
+		uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
+		pt_bits &= se_tpg->fabric_sup_prot_type;
+
+		/* cross check with fabric to see if t10dif is supported */
+		if (!pt_bits) {
+			//dev->dev_attrib.pi_prot_type = 0;
+			pr_err("dev[%p]: DIF protection mismatch with fabric "
+				"(%s). Transport capability bits[0x%x]\n",
+				dev, se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
+				se_tpg->fabric_sup_prot_type);
+			return -EFAULT;
+		}
+	}
+
 	if (lun->lun_se_dev !=  NULL) {
 		pr_err("Port Symlink already exists\n");
 		return -EEXIST;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index c036595..9279971 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
 }
 EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
 
+void core_tpg_set_fabric_t10dif(
+	struct se_portal_group *tpg,
+	uint32_t fabric_t10dif_force_on)
+{
+	tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
+}
+EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
+
+
 static void core_tpg_lun_ref_release(struct percpu_ref *ref)
 {
 	struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index ec3e3a3..fc2c0ef 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
 	TMR_FUNCTION_REJECTED		= 5,
 };
 
+enum target_guard_type_cap {
+	TARGET_GUARD_CRC = 1 << 0,
+	TARGET_GUARD_IP  = 1 << 1,
+};
+
+enum target_prot_type_cap {
+	TARGET_DIF_TYPE1_PROTECTION = 1 << 0, /* T10 DIF Type 1 */
+	TARGET_DIF_TYPE2_PROTECTION = 1 << 1, /* T10 DIF Type 2 */
+	TARGET_DIF_TYPE3_PROTECTION = 1 << 2, /* T10 DIF Type 3 */
+};
+
 /*
  * Used for target SCSI statistics
  */
@@ -862,6 +873,9 @@ struct se_portal_group {
 	enum transport_tpg_type_table se_tpg_type;
 	/* Number of ACLed Initiator Nodes for this TPG */
 	u32			num_node_acls;
+	u32			fabric_t10dif_force_on;
+	u32			fabric_sup_guard_type;
+	u32			fabric_sup_prot_type;
 	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
 	atomic_t		tpg_pr_ref_count;
 	/* Spinlock for adding/removing ACLed Nodes */
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 1d10436..4c3dab5 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ int	core_tpg_set_initiator_node_queue_depth(struct se_portal_group *,
 		unsigned char *, u32, int);
 int	core_tpg_set_initiator_node_tag(struct se_portal_group *,
 		struct se_node_acl *, const char *);
+void	core_tpg_set_fabric_t10dif(struct se_portal_group *, uint32_t);
 int	core_tpg_register(struct target_core_fabric_ops *, struct se_wwn *,
 		struct se_portal_group *, void *, int);
 int	core_tpg_deregister(struct se_portal_group *);
-- 
1.8.4.GIT

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[sagi grimberg]

On 3/29/2014 2:05 AM, Quinn Tran wrote:
> Check lower layer/HW support of T10-dif capability.
> When the LUN is linked between the underlining fabric
> and back end device, the Protection Type(1,2,3) is
> check against each other to make sure both side are
> capable of supporting the same protection setting.
>
> ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
> /sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123
>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
> ---
>   drivers/target/target_core_fabric_configfs.c | 20 ++++++++++++++++++++
>   drivers/target/target_core_tpg.c             |  9 +++++++++
>   include/target/target_core_base.h            | 14 ++++++++++++++
>   include/target/target_core_fabric.h          |  1 +
>   4 files changed, 44 insertions(+)
>
> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> index 7de9f04..3ce07ec 100644
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -777,6 +777,26 @@ static int target_fabric_port_link(
>   				struct se_portal_group, tpg_group);
>   	tf = se_tpg->se_tpg_wwn->wwn_tf;
>   
> +
> +	if (dev->dev_attrib.pi_prot_type) {
> +		uint32_t cap[] = { 0,
> +				   TARGET_DIF_TYPE1_PROTECTION,
> +				   TARGET_DIF_TYPE2_PROTECTION,
> +				   TARGET_DIF_TYPE3_PROTECTION};
> +		uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
> +		pt_bits &= se_tpg->fabric_sup_prot_type;

At what point should the fabric fill that? it can vary between portals 
right?
I would prefer to do that as a function pointer to explicitly ask the 
fabric it's support.

> +
> +		/* cross check with fabric to see if t10dif is supported */
> +		if (!pt_bits) {
> +			//dev->dev_attrib.pi_prot_type = 0;

Probably should lose the comment.

> +			pr_err("dev[%p]: DIF protection mismatch with fabric "
> +				"(%s). Transport capability bits[0x%x]\n",
> +				dev, se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
> +				se_tpg->fabric_sup_prot_type);
> +			return -EFAULT;

Didn't we agree that this is allowed and the target core should 
compensate on the lack fabric support?

> +		}
> +	}
> +
>   	if (lun->lun_se_dev !=  NULL) {
>   		pr_err("Port Symlink already exists\n");
>   		return -EEXIST;
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index c036595..9279971 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
>   }
>   EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
>   
> +void core_tpg_set_fabric_t10dif(
> +	struct se_portal_group *tpg,
> +	uint32_t fabric_t10dif_force_on)
> +{
> +	tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
> +}
> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
> +

Is there a user for this function in this patch?

> +
>   static void core_tpg_lun_ref_release(struct percpu_ref *ref)
>   {
>   	struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index ec3e3a3..fc2c0ef 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
>   	TMR_FUNCTION_REJECTED		= 5,
>   };
>   
> +enum target_guard_type_cap {
> +	TARGET_GUARD_CRC = 1 << 0,
> +	TARGET_GUARD_IP  = 1 << 1,
> +};
> +

So this was dropped in previous rounds, this is more of an initiator 
thing, the target will always
receive CRC from the wire and will pass it to the backing device so no 
need to convert to IP_CSUM.

> +enum target_prot_type_cap {
> +	TARGET_DIF_TYPE1_PROTECTION = 1 << 0, /* T10 DIF Type 1 */
> +	TARGET_DIF_TYPE2_PROTECTION = 1 << 1, /* T10 DIF Type 2 */
> +	TARGET_DIF_TYPE3_PROTECTION = 1 << 2, /* T10 DIF Type 3 */
> +};
> +
>   /*
>    * Used for target SCSI statistics
>    */
> @@ -862,6 +873,9 @@ struct se_portal_group {
>   	enum transport_tpg_type_table se_tpg_type;
>   	/* Number of ACLed Initiator Nodes for this TPG */
>   	u32			num_node_acls;
> +	u32			fabric_t10dif_force_on;
> +	u32			fabric_sup_guard_type;
> +	u32			fabric_sup_prot_type;
>   	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
>   	atomic_t		tpg_pr_ref_count;
>   	/* Spinlock for adding/removing ACLed Nodes */
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 1d10436..4c3dab5 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -152,6 +152,7 @@ int	core_tpg_set_initiator_node_queue_depth(struct se_portal_group *,
>   		unsigned char *, u32, int);
>   int	core_tpg_set_initiator_node_tag(struct se_portal_group *,
>   		struct se_node_acl *, const char *);
> +void	core_tpg_set_fabric_t10dif(struct se_portal_group *, uint32_t);
>   int	core_tpg_register(struct target_core_fabric_ops *, struct se_wwn *,
>   		struct se_portal_group *, void *, int);
>   int	core_tpg_deregister(struct se_portal_group *);

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Quinn Tran]

Answer in line.

Regards,
Quinn Tran




On 3/28/14 5:05 PM, "sagi grimberg" <sagig@mellanox.com> wrote:

>On 3/29/2014 2:05 AM, Quinn Tran wrote:
>> Check lower layer/HW support of T10-dif capability.
>> When the LUN is linked between the underlining fabric
>> and back end device, the Protection Type(1,2,3) is
>> check against each other to make sure both side are
>> capable of supporting the same protection setting.
>>
>> ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
>> /sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123
>>
>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
>> ---
>>   drivers/target/target_core_fabric_configfs.c | 20 ++++++++++++++++++++
>>   drivers/target/target_core_tpg.c             |  9 +++++++++
>>   include/target/target_core_base.h            | 14 ++++++++++++++
>>   include/target/target_core_fabric.h          |  1 +
>>   4 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/target/target_core_fabric_configfs.c
>>b/drivers/target/target_core_fabric_configfs.c
>> index 7de9f04..3ce07ec 100644
>> --- a/drivers/target/target_core_fabric_configfs.c
>> +++ b/drivers/target/target_core_fabric_configfs.c
>> @@ -777,6 +777,26 @@ static int target_fabric_port_link(
>>                              struct se_portal_group, tpg_group);
>>      tf = se_tpg->se_tpg_wwn->wwn_tf;
>>
>> +
>> +    if (dev->dev_attrib.pi_prot_type) {
>> +            uint32_t cap[] = { 0,
>> +                               TARGET_DIF_TYPE1_PROTECTION,
>> +                               TARGET_DIF_TYPE2_PROTECTION,
>> +                               TARGET_DIF_TYPE3_PROTECTION};
>> +            uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
>> +            pt_bits &= se_tpg->fabric_sup_prot_type;
>
>At what point should the fabric fill that? it can vary between portals
>right?

QT> Yes, protection mode can vary between portals. When user select the
physical function (via fabric_make_tpg), you know the specific portal
based on user input and its capability. This is where Qlogic register its
capabilities based on specific hardware.


>I would prefer to do that as a function pointer to explicitly ask the
>fabric it's support.

QT> is it still require with previous answer ?


>
>> +
>> +            /* cross check with fabric to see if t10dif is supported */
>> +            if (!pt_bits) {
>> +                    //dev->dev_attrib.pi_prot_type = 0;
>
>Probably should lose the comment.

<nod>

>
>> +                    pr_err("dev[%p]: DIF protection mismatch with fabric "
>> +                            "(%s). Transport capability bits[0x%x]\n",
>> +                            dev, se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
>> +                            se_tpg->fabric_sup_prot_type);
>> +                    return -EFAULT;
>
>Didn't we agree that this is allowed and the target core should
>compensate on the lack fabric support?

<QT> My recollection was that it's not recommended to have different
portals with different supported feature. Meaning a SCSI Write without Dif
down a none-T10PI portal update the data.  The Guard on the disk is now
mismatch with the data. A SCSI Read down a T10PI path will run into a
Guard failure.

By introducing this option, Disk vendor require additional logic to
compensate for this. I think it's cheaper to have supported hardware
rather than support nightmare.

>
>> +            }
>> +    }
>> +
>>      if (lun->lun_se_dev !=  NULL) {
>>              pr_err("Port Symlink already exists\n");
>>              return -EEXIST;
>> diff --git a/drivers/target/target_core_tpg.c
>>b/drivers/target/target_core_tpg.c
>> index c036595..9279971 100644
>> --- a/drivers/target/target_core_tpg.c
>> +++ b/drivers/target/target_core_tpg.c
>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
>>   }
>>   EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
>>
>> +void core_tpg_set_fabric_t10dif(
>> +    struct se_portal_group *tpg,
>> +    uint32_t fabric_t10dif_force_on)
>> +{
>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
>> +}
>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
>> +
>
>Is there a user for this function in this patch?

QT> I'm on the fence with this piece.  Just thinking of a case where DIX
is not available for initiator side, but user wants to turn on protection
at the link layer.  Our test folks would like to cover this case in the
future.

>
>> +
>>   static void core_tpg_lun_ref_release(struct percpu_ref *ref)
>>   {
>>      struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
>> diff --git a/include/target/target_core_base.h
>>b/include/target/target_core_base.h
>> index ec3e3a3..fc2c0ef 100644
>> --- a/include/target/target_core_base.h
>> +++ b/include/target/target_core_base.h
>> @@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
>>      TMR_FUNCTION_REJECTED           = 5,
>>   };
>>
>> +enum target_guard_type_cap {
>> +    TARGET_GUARD_CRC = 1 << 0,
>> +    TARGET_GUARD_IP  = 1 << 1,
>> +};
>> +
>
>So this was dropped in previous rounds, this is more of an initiator
>thing, the target will always
>receive CRC from the wire and will pass it to the backing device so no
>need to convert to IP_CSUM.


QT> Will drop this.


>
>> +enum target_prot_type_cap {
>> +    TARGET_DIF_TYPE1_PROTECTION = 1 << 0, /* T10 DIF Type 1 */
>> +    TARGET_DIF_TYPE2_PROTECTION = 1 << 1, /* T10 DIF Type 2 */
>> +    TARGET_DIF_TYPE3_PROTECTION = 1 << 2, /* T10 DIF Type 3 */
>> +};
>> +
>>   /*
>>    * Used for target SCSI statistics
>>    */
>> @@ -862,6 +873,9 @@ struct se_portal_group {
>>      enum transport_tpg_type_table se_tpg_type;
>>      /* Number of ACLed Initiator Nodes for this TPG */
>>      u32                     num_node_acls;
>> +    u32                     fabric_t10dif_force_on;
>> +    u32                     fabric_sup_guard_type;
>> +    u32                     fabric_sup_prot_type;
>>      /* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
>>      atomic_t                tpg_pr_ref_count;
>>      /* Spinlock for adding/removing ACLed Nodes */
>> diff --git a/include/target/target_core_fabric.h
>>b/include/target/target_core_fabric.h
>> index 1d10436..4c3dab5 100644
>> --- a/include/target/target_core_fabric.h
>> +++ b/include/target/target_core_fabric.h
>> @@ -152,6 +152,7 @@ int      core_tpg_set_initiator_node_queue_depth(struct
>>se_portal_group *,
>>              unsigned char *, u32, int);
>>   int        core_tpg_set_initiator_node_tag(struct se_portal_group *,
>>              struct se_node_acl *, const char *);
>> +void        core_tpg_set_fabric_t10dif(struct se_portal_group *, uint32_t);
>>   int        core_tpg_register(struct target_core_fabric_ops *, struct se_wwn
>>*,
>>              struct se_portal_group *, void *, int);
>>   int        core_tpg_deregister(struct se_portal_group *);
>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[sagi grimberg]

On 3/29/2014 3:53 AM, Quinn Tran wrote:
> +
> +    if (dev->dev_attrib.pi_prot_type) {
> +            uint32_t cap[] = { 0,
> +                               TARGET_DIF_TYPE1_PROTECTION,
> +                               TARGET_DIF_TYPE2_PROTECTION,
> +                               TARGET_DIF_TYPE3_PROTECTION};
> +            uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
> +            pt_bits &= se_tpg->fabric_sup_prot_type;
>> At what point should the fabric fill that? it can vary between portals
>> right?
> QT> Yes, protection mode can vary between portals. When user select the
> physical function (via fabric_make_tpg), you know the specific portal
> based on user input and its capability. This is where Qlogic register its
> capabilities based on specific hardware.
>
>
>> I would prefer to do that as a function pointer to explicitly ask the
>> fabric it's support.
> QT> is it still require with previous answer ?
>

Well, I think it is nicer to explicitly ask the fabric at that point 
instead of checking what it previously set.

By the way, I think this patch breaks existing iSER support as iSER 
doesn't set these bits.
Thats why I think it would be a good idea to *explicitly* ask.

>
>>> +                    pr_err("dev[%p]: DIF protection mismatch with fabric "
>>> +                            "(%s). Transport capability bits[0x%x]\n",
>>> +                            dev, se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
>>> +                            se_tpg->fabric_sup_prot_type);
>>> +                    return -EFAULT;
>> Didn't we agree that this is allowed and the target core should
>> compensate on the lack fabric support?
> <QT> My recollection was that it's not recommended to have different
> portals with different supported feature.

Well we seem to remember different things...
Anyway I think it is better to compensate that in backstore/target-core 
level, that would be better
than silently turn off protection. Martin? Nic? your takes?

Also I don't know what rats are hiding here if the backstore is handling 
IOs in this time...
What about cleaning up all the protection resources the backstore driver 
might be managing?

>   Meaning a SCSI Write without Dif
> down a none-T10PI portal update the data.  The Guard on the disk is now
> mismatch with the data. A SCSI Read down a T10PI path will run into a
> Guard failure.
>
> By introducing this option, Disk vendor require additional logic to
> compensate for this. I think it's cheaper to have supported hardware
> rather than support nightmare.
>
>>> +            }
>>> +    }
>>> +
>>>       if (lun->lun_se_dev !=  NULL) {
>>>               pr_err("Port Symlink already exists\n");
>>>               return -EEXIST;
>>> diff --git a/drivers/target/target_core_tpg.c
>>> b/drivers/target/target_core_tpg.c
>>> index c036595..9279971 100644
>>> --- a/drivers/target/target_core_tpg.c
>>> +++ b/drivers/target/target_core_tpg.c
>>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
>>>    }
>>>    EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
>>>
>>> +void core_tpg_set_fabric_t10dif(
>>> +    struct se_portal_group *tpg,
>>> +    uint32_t fabric_t10dif_force_on)
>>> +{
>>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
>>> +}
>>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
>>> +
>> Is there a user for this function in this patch?
> QT> I'm on the fence with this piece.  Just thinking of a case where DIX
> is not available for initiator side, but user wants to turn on protection
> at the link layer.  Our test folks would like to cover this case in the
> future.

Not sure I understand. Initiator will send the target data+protection 
(DIX disabled HBA does INSERT), why does this help?
Why should the target fabric care who generated the protection (OS or HBA)?

Sagi.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 5273 bytes --]


Regards,
Quinn Tran




On 3/28/14 6:24 PM, "sagi grimberg" <sagig@mellanox.com> wrote:

>On 3/29/2014 3:53 AM, Quinn Tran wrote:
>> +
>> +    if (dev->dev_attrib.pi_prot_type) {
>> +            uint32_t cap[] = { 0,
>> +                               TARGET_DIF_TYPE1_PROTECTION,
>> +                               TARGET_DIF_TYPE2_PROTECTION,
>> +                               TARGET_DIF_TYPE3_PROTECTION};
>> +            uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
>> +            pt_bits &= se_tpg->fabric_sup_prot_type;
>>> At what point should the fabric fill that? it can vary between portals
>>> right?
>> QT> Yes, protection mode can vary between portals. When user select the
>> physical function (via fabric_make_tpg), you know the specific portal
>> based on user input and its capability. This is where Qlogic register
>>its
>> capabilities based on specific hardware.
>>
>>
>>> I would prefer to do that as a function pointer to explicitly ask the
>>> fabric it's support.
>> QT> is it still require with previous answer ?
>>
>
>Well, I think it is nicer to explicitly ask the fabric at that point
>instead of checking what it previously set.
>
>By the way, I think this patch breaks existing iSER support as iSER
>doesn't set these bits.
>Thats why I think it would be a good idea to *explicitly* ask.

QT> I see.  No issue with converting to a callback.

>
>>
>>>> +                    pr_err("dev[%p]: DIF protection mismatch with
>>>>fabric "
>>>> +                            "(%s). Transport capability
>>>>bits[0x%x]\n",
>>>> +                            dev,
>>>>se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
>>>> +                            se_tpg->fabric_sup_prot_type);
>>>> +                    return -EFAULT;
>>> Didn't we agree that this is allowed and the target core should
>>> compensate on the lack fabric support?
>> <QT> My recollection was that it's not recommended to have different
>> portals with different supported feature.
>
>Well we seem to remember different things...
>Anyway I think it is better to compensate that in backstore/target-core
>level, that would be better
>than silently turn off protection. Martin? Nic? your takes?

QT> the error return above fail the binding (ln -sf <backend disk> <fabric
LUN>) between the back disk and the frontend/fabric LUN representation.
The failure happens during configuration time.  The commented out code
imply a silent turn off. Sorry should have clean it out.


>
>Also I don't know what rats are hiding here if the backstore is handling
>IOs in this time...
>What about cleaning up all the protection resources the backstore driver
>might be managing?

QT> hmm.  It's a big hammer.  I'll let the other folks chime in on this
because it affect user's interaction.  Nicholas ? Martin?

>
>>   Meaning a SCSI Write without Dif
>> down a none-T10PI portal update the data.  The Guard on the disk is now
>> mismatch with the data. A SCSI Read down a T10PI path will run into a
>> Guard failure.
>>
>> By introducing this option, Disk vendor require additional logic to
>> compensate for this. I think it's cheaper to have supported hardware
>> rather than support nightmare.
>>
>>>> +            }
>>>> +    }
>>>> +
>>>>       if (lun->lun_se_dev !=  NULL) {
>>>>               pr_err("Port Symlink already exists\n");
>>>>               return -EEXIST;
>>>> diff --git a/drivers/target/target_core_tpg.c
>>>> b/drivers/target/target_core_tpg.c
>>>> index c036595..9279971 100644
>>>> --- a/drivers/target/target_core_tpg.c
>>>> +++ b/drivers/target/target_core_tpg.c
>>>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
>>>>    }
>>>>    EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
>>>>
>>>> +void core_tpg_set_fabric_t10dif(
>>>> +    struct se_portal_group *tpg,
>>>> +    uint32_t fabric_t10dif_force_on)
>>>> +{
>>>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
>>>> +}
>>>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
>>>> +
>>> Is there a user for this function in this patch?
>> QT> I'm on the fence with this piece.  Just thinking of a case where DIX
>> is not available for initiator side, but user wants to turn on
>>protection
>> at the link layer.  Our test folks would like to cover this case in the
>> future.
>
>Not sure I understand. Initiator will send the target data+protection
>(DIX disabled HBA does INSERT), why does this help?
>Why should the target fabric care who generated the protection (OS or
>HBA)?

QT> Sorry for the confusion.  The case I'm trying to get at is "Data In
Insert" and "Data out strip".    In this case, the protection starts from
front end target adapter to the back end storage.  In revisit your
previous patch, this case is not covered.


>
>Sagi.


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6780 bytes --]

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Nicholas A. Bellinger]

On Mon, 2014-03-31 at 17:53 +0000, Quinn Tran wrote:
> On 3/28/14 6:24 PM, "sagi grimberg" <sagig@mellanox.com> wrote:
> 
> >On 3/29/2014 3:53 AM, Quinn Tran wrote:

<SNIP>

> >>>> +            }
> >>>> +    }
> >>>> +
> >>>>       if (lun->lun_se_dev !=  NULL) {
> >>>>               pr_err("Port Symlink already exists\n");
> >>>>               return -EEXIST;
> >>>> diff --git a/drivers/target/target_core_tpg.c
> >>>> b/drivers/target/target_core_tpg.c
> >>>> index c036595..9279971 100644
> >>>> --- a/drivers/target/target_core_tpg.c
> >>>> +++ b/drivers/target/target_core_tpg.c
> >>>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
> >>>>    }
> >>>>    EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
> >>>>
> >>>> +void core_tpg_set_fabric_t10dif(
> >>>> +    struct se_portal_group *tpg,
> >>>> +    uint32_t fabric_t10dif_force_on)
> >>>> +{
> >>>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
> >>>> +}
> >>>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
> >>>> +
> >>> Is there a user for this function in this patch?
> >> QT> I'm on the fence with this piece.  Just thinking of a case where DIX
> >> is not available for initiator side, but user wants to turn on
> >>protection
> >> at the link layer.  Our test folks would like to cover this case in the
> >> future.
> >
> >Not sure I understand. Initiator will send the target data+protection
> >(DIX disabled HBA does INSERT), why does this help?
> >Why should the target fabric care who generated the protection (OS or
> >HBA)?
> 
> QT> Sorry for the confusion.  The case I'm trying to get at is "Data In
> Insert" and "Data out strip".    In this case, the protection starts from
> front end target adapter to the back end storage.  In revisit your
> previous patch, this case is not covered.
> 
> 

<nod>

So for the TARGET_PROT_DIN_INSERT + TARGET_PROT_DOUT_STRIP cases, the
target will need to expose INQUIRY PROTECT=1 + other PI related control
bits when the fabric supports these modes, regardless of what PI is
supported on the backend device.

Keeping this configuration in mind as well while coding up the
aforementioned series.  ;)

--nab

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Sagi Grimberg]

On 4/1/2014 4:19 AM, Nicholas A. Bellinger wrote:
> On Mon, 2014-03-31 at 17:53 +0000, Quinn Tran wrote:
>> On 3/28/14 6:24 PM, "sagi grimberg" <sagig@mellanox.com> wrote:
>>
>>> On 3/29/2014 3:53 AM, Quinn Tran wrote:
> <SNIP>
>
>>>>>> +            }
>>>>>> +    }
>>>>>> +
>>>>>>        if (lun->lun_se_dev !=  NULL) {
>>>>>>                pr_err("Port Symlink already exists\n");
>>>>>>                return -EEXIST;
>>>>>> diff --git a/drivers/target/target_core_tpg.c
>>>>>> b/drivers/target/target_core_tpg.c
>>>>>> index c036595..9279971 100644
>>>>>> --- a/drivers/target/target_core_tpg.c
>>>>>> +++ b/drivers/target/target_core_tpg.c
>>>>>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
>>>>>>
>>>>>> +void core_tpg_set_fabric_t10dif(
>>>>>> +    struct se_portal_group *tpg,
>>>>>> +    uint32_t fabric_t10dif_force_on)
>>>>>> +{
>>>>>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
>>>>>> +
>>>>> Is there a user for this function in this patch?
>>>> QT> I'm on the fence with this piece.  Just thinking of a case where DIX
>>>> is not available for initiator side, but user wants to turn on
>>>> protection
>>>> at the link layer.  Our test folks would like to cover this case in the
>>>> future.
>>> Not sure I understand. Initiator will send the target data+protection
>>> (DIX disabled HBA does INSERT), why does this help?
>>> Why should the target fabric care who generated the protection (OS or
>>> HBA)?
>> QT> Sorry for the confusion.  The case I'm trying to get at is "Data In
>> Insert" and "Data out strip".    In this case, the protection starts from
>> front end target adapter to the back end storage.  In revisit your
>> previous patch, this case is not covered.
>>
>>
> <nod>
>
> So for the TARGET_PROT_DIN_INSERT + TARGET_PROT_DOUT_STRIP cases, the
> target will need to expose INQUIRY PROTECT=1 + other PI related control
> bits when the fabric supports these modes, regardless of what PI is
> supported on the backend device.
>
> Keeping this configuration in mind as well while coding up the
> aforementioned series.  ;)

Well, I don't know if adding this support just so we can test it is good 
enough of a justification...

I originally wrote the code to support that. But it got left behind 
since I figured it is not an interesting use-case.
If your beckend doesn't support T10-PI why should the target publish it 
support it and ask the device to strip/insert it?
I suppose it is to allow the initiator to protect half-way, but I don't 
know how interesting it is if the data is not stored with protection...

The only interesting case I thought of was to allow (initiator side) 
performance tests for NULL devices. In this case you probably need
the DOUT_STRIP/DIN_INSERT. But seems to me it is debug code rather then 
production.

Sagi.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:

Sagi> I originally wrote the code to support that. But it got left
Sagi> behind since I figured it is not an interesting use-case.  If your
Sagi> beckend doesn't support T10-PI why should the target publish it
Sagi> support it and ask the device to strip/insert it?  I suppose it is
Sagi> to allow the initiator to protect half-way, but I don't know how
Sagi> interesting it is if the data is not stored with protection...

That depends what you do on the backend. There are several devices out
there that expose PI to the host but use a different protection scheme
internally. And then synthesize PI on the host-facing side. Some even do
T10 PI to an internal protection scheme and then back to T10 PI when
talking to the disk drives in the back end.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[sagi grimberg]

On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
> Sagi> I originally wrote the code to support that. But it got left
> Sagi> behind since I figured it is not an interesting use-case.  If your
> Sagi> beckend doesn't support T10-PI why should the target publish it
> Sagi> support it and ask the device to strip/insert it?  I suppose it is
> Sagi> to allow the initiator to protect half-way, but I don't know how
> Sagi> interesting it is if the data is not stored with protection...
>
> That depends what you do on the backend. There are several devices out
> there that expose PI to the host but use a different protection scheme
> internally. And then synthesize PI on the host-facing side. Some even do
> T10 PI to an internal protection scheme and then back to T10 PI when
> talking to the disk drives in the back end.
>

Hey Martin,

I understand, but even for internal different T10-PI schemes, is 
stripping protection from incoming data
at the fabric level (and then do whatever with it in the backend level) 
the right thing to do here?
I mean we basically lose protection across the PCI with this scheme 
aren't we?

Sagi.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Nicholas A. Bellinger]

On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
> >>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
> > Sagi> I originally wrote the code to support that. But it got left
> > Sagi> behind since I figured it is not an interesting use-case.  If your
> > Sagi> beckend doesn't support T10-PI why should the target publish it
> > Sagi> support it and ask the device to strip/insert it?  I suppose it is
> > Sagi> to allow the initiator to protect half-way, but I don't know how
> > Sagi> interesting it is if the data is not stored with protection...
> >
> > That depends what you do on the backend. There are several devices out
> > there that expose PI to the host but use a different protection scheme
> > internally. And then synthesize PI on the host-facing side. Some even do
> > T10 PI to an internal protection scheme and then back to T10 PI when
> > talking to the disk drives in the back end.
> >
> 
> Hey Martin,
> 
> I understand, but even for internal different T10-PI schemes, is 
> stripping protection from incoming data
> at the fabric level (and then do whatever with it in the backend level) 
> the right thing to do here?
> I mean we basically lose protection across the PCI with this scheme 
> aren't we?
> 

The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
backends that don't support real hw PI, so that at least the protection
can be in place for data movement between physical machines.

Also, I think the amount of changes required to support this type of
configuration in target-core are quite small.

--nab

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Sagi Grimberg]

On 4/1/2014 8:45 PM, Nicholas A. Bellinger wrote:
> On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
>> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
>>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>>> Sagi> I originally wrote the code to support that. But it got left
>>> Sagi> behind since I figured it is not an interesting use-case.  If your
>>> Sagi> beckend doesn't support T10-PI why should the target publish it
>>> Sagi> support it and ask the device to strip/insert it?  I suppose it is
>>> Sagi> to allow the initiator to protect half-way, but I don't know how
>>> Sagi> interesting it is if the data is not stored with protection...
>>>
>>> That depends what you do on the backend. There are several devices out
>>> there that expose PI to the host but use a different protection scheme
>>> internally. And then synthesize PI on the host-facing side. Some even do
>>> T10 PI to an internal protection scheme and then back to T10 PI when
>>> talking to the disk drives in the back end.
>>>
>> Hey Martin,
>>
>> I understand, but even for internal different T10-PI schemes, is
>> stripping protection from incoming data
>> at the fabric level (and then do whatever with it in the backend level)
>> the right thing to do here?
>> I mean we basically lose protection across the PCI with this scheme
>> aren't we?
>>
> The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
> backends that don't support real hw PI, so that at least the protection
> can be in place for data movement between physical machines.
>
> Also, I think the amount of changes required to support this type of
> configuration in target-core are quite small.

So trying to understand how this will come to use.
Target will publish the fabric T10-PI support based only on the 
transport configuration (not accounting the backing devices configuration).
Then upon each cmd the target will look on {backstore configuration, 
PROTECT bit, transport configuration} - then will decide on protection
operation (STRIP/INSERT/PASS).

Looks right?

Sagi.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Nicholas A. Bellinger]

On Wed, 2014-04-02 at 09:51 +0300, Sagi Grimberg wrote:
> On 4/1/2014 8:45 PM, Nicholas A. Bellinger wrote:
> > On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
> >> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
> >>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
> >>> Sagi> I originally wrote the code to support that. But it got left
> >>> Sagi> behind since I figured it is not an interesting use-case.  If your
> >>> Sagi> beckend doesn't support T10-PI why should the target publish it
> >>> Sagi> support it and ask the device to strip/insert it?  I suppose it is
> >>> Sagi> to allow the initiator to protect half-way, but I don't know how
> >>> Sagi> interesting it is if the data is not stored with protection...
> >>>
> >>> That depends what you do on the backend. There are several devices out
> >>> there that expose PI to the host but use a different protection scheme
> >>> internally. And then synthesize PI on the host-facing side. Some even do
> >>> T10 PI to an internal protection scheme and then back to T10 PI when
> >>> talking to the disk drives in the back end.
> >>>
> >> Hey Martin,
> >>
> >> I understand, but even for internal different T10-PI schemes, is
> >> stripping protection from incoming data
> >> at the fabric level (and then do whatever with it in the backend level)
> >> the right thing to do here?
> >> I mean we basically lose protection across the PCI with this scheme
> >> aren't we?
> >>
> > The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
> > backends that don't support real hw PI, so that at least the protection
> > can be in place for data movement between physical machines.
> >
> > Also, I think the amount of changes required to support this type of
> > configuration in target-core are quite small.
> 
> So trying to understand how this will come to use.
> Target will publish the fabric T10-PI support based only on the 
> transport configuration (not accounting the backing devices configuration).

Yes, passing in the transport configuration for PI at
transport_init_session() time seems to make the most sense here in order
to address all fabric types.

> Then upon each cmd the target will look on {backstore configuration, 
> PROTECT bit, transport configuration} - then will decide on protection
> operation (STRIP/INSERT/PASS).
> 
> Looks right?
> 

Correct.

I'm thinking it makes sense for target-core to perform the WRITE_INSERT
+ READ_STRIP (in software) when the transport does not directly support
PI, but the backend has PI enabled.

--nab

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]


Regards,
Quinn Tran




On 4/2/14 11:20 AM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>On Wed, 2014-04-02 at 09:51 +0300, Sagi Grimberg wrote:
>> On 4/1/2014 8:45 PM, Nicholas A. Bellinger wrote:
>> > On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
>> >> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
>> >>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>> >>> Sagi> I originally wrote the code to support that. But it got left
>> >>> Sagi> behind since I figured it is not an interesting use-case.  If
>>your
>> >>> Sagi> beckend doesn't support T10-PI why should the target publish
>>it
>> >>> Sagi> support it and ask the device to strip/insert it?  I suppose
>>it is
>> >>> Sagi> to allow the initiator to protect half-way, but I don't know
>>how
>> >>> Sagi> interesting it is if the data is not stored with protection...
>> >>>
>> >>> That depends what you do on the backend. There are several devices
>>out
>> >>> there that expose PI to the host but use a different protection
>>scheme
>> >>> internally. And then synthesize PI on the host-facing side. Some
>>even do
>> >>> T10 PI to an internal protection scheme and then back to T10 PI when
>> >>> talking to the disk drives in the back end.
>> >>>
>> >> Hey Martin,
>> >>
>> >> I understand, but even for internal different T10-PI schemes, is
>> >> stripping protection from incoming data
>> >> at the fabric level (and then do whatever with it in the backend
>>level)
>> >> the right thing to do here?
>> >> I mean we basically lose protection across the PCI with this scheme
>> >> aren't we?
>> >>
>> > The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
>> > backends that don't support real hw PI, so that at least the
>>protection
>> > can be in place for data movement between physical machines.
>> >
>> > Also, I think the amount of changes required to support this type of
>> > configuration in target-core are quite small.
>>
>> So trying to understand how this will come to use.
>> Target will publish the fabric T10-PI support based only on the
>> transport configuration (not accounting the backing devices
>>configuration).
>
>Yes, passing in the transport configuration for PI at
>transport_init_session() time seems to make the most sense here in order
>to address all fabric types.

QT> Ack.  Registering PI cap per IT nexus would fit all fabrics.

>
>> Then upon each cmd the target will look on {backstore configuration,
>> PROTECT bit, transport configuration} - then will decide on protection
>> operation (STRIP/INSERT/PASS).
>>
>> Looks right?
>>
>
>Correct.
>
>I'm thinking it makes sense for target-core to perform the WRITE_INSERT
>+ READ_STRIP (in software) when the transport does not directly support
>PI, but the backend has PI enabled.
>
>--nab

QT> I see, this cover the case of a transport within a Portal does support
PI.

>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6700 bytes --]

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[sagi grimberg]

On 4/1/2014 8:45 PM, Nicholas A. Bellinger wrote:
> On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
>> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
>>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>>> Sagi> I originally wrote the code to support that. But it got left
>>> Sagi> behind since I figured it is not an interesting use-case.  If your
>>> Sagi> beckend doesn't support T10-PI why should the target publish it
>>> Sagi> support it and ask the device to strip/insert it?  I suppose it is
>>> Sagi> to allow the initiator to protect half-way, but I don't know how
>>> Sagi> interesting it is if the data is not stored with protection...
>>>
>>> That depends what you do on the backend. There are several devices out
>>> there that expose PI to the host but use a different protection scheme
>>> internally. And then synthesize PI on the host-facing side. Some even do
>>> T10 PI to an internal protection scheme and then back to T10 PI when
>>> talking to the disk drives in the back end.
>>>
>> Hey Martin,
>>
>> I understand, but even for internal different T10-PI schemes, is
>> stripping protection from incoming data
>> at the fabric level (and then do whatever with it in the backend level)
>> the right thing to do here?
>> I mean we basically lose protection across the PCI with this scheme
>> aren't we?
>>
> The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
> backends that don't support real hw PI, so that at least the protection
> can be in place for data movement between physical machines.
>
> Also, I think the amount of changes required to support this type of
> configuration in target-core are quite small.

Not sure I understood your intention here.
Do you mean that the backstore doesn't support T10-PI, the transport 
support T10-PI and target publish to fabric that it support T10-PI?
This will lead to the DIN_INSERT/DOUT_STRIP you are referring to right?
Is there any value to publish the fabric PI support if your backing 
device doesn't support it?

The opposite case (backstore support, transport no support) it makes 
sense to NOT publish support and do half-way protection in SW
(the backstore is formatted with PI).

Sagi.

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Nicholas A. Bellinger]

On Wed, 2014-04-02 at 14:43 +0300, sagi grimberg wrote:
> On 4/1/2014 8:45 PM, Nicholas A. Bellinger wrote:
> > On Tue, 2014-04-01 at 20:27 +0300, sagi grimberg wrote:
> >> On 4/1/2014 8:09 PM, Martin K. Petersen wrote:
> >>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
> >>> Sagi> I originally wrote the code to support that. But it got left
> >>> Sagi> behind since I figured it is not an interesting use-case.  If your
> >>> Sagi> beckend doesn't support T10-PI why should the target publish it
> >>> Sagi> support it and ask the device to strip/insert it?  I suppose it is
> >>> Sagi> to allow the initiator to protect half-way, but I don't know how
> >>> Sagi> interesting it is if the data is not stored with protection...
> >>>
> >>> That depends what you do on the backend. There are several devices out
> >>> there that expose PI to the host but use a different protection scheme
> >>> internally. And then synthesize PI on the host-facing side. Some even do
> >>> T10 PI to an internal protection scheme and then back to T10 PI when
> >>> talking to the disk drives in the back end.
> >>>
> >> Hey Martin,
> >>
> >> I understand, but even for internal different T10-PI schemes, is
> >> stripping protection from incoming data
> >> at the fabric level (and then do whatever with it in the backend level)
> >> the right thing to do here?
> >> I mean we basically lose protection across the PCI with this scheme
> >> aren't we?
> >>
> > The WRITE_STRIP + READ_INSERT case would be still be useful for IBLOCK
> > backends that don't support real hw PI, so that at least the protection
> > can be in place for data movement between physical machines.
> >
> > Also, I think the amount of changes required to support this type of
> > configuration in target-core are quite small.
> 
> Not sure I understood your intention here.
> Do you mean that the backstore doesn't support T10-PI, the transport 
> support T10-PI and target publish to fabric that it support T10-PI?
> This will lead to the DIN_INSERT/DOUT_STRIP you are referring to right?

Correct.

> Is there any value to publish the fabric PI support if your backing 
> device doesn't support it?

So yes, I think there is value is allowing a transport to publish PI
support for a device, even when the backend does not support it.

In terms of the possibilities for data corruption, moving payloads
between physical hosts would certainly have a higher potential given the
complexity and number of overall components involved in the transaction.

If all unprotected devices should report PI (by default) when the
transport supports it is a separate question..  ;)

> 
> The opposite case (backstore support, transport no support) it makes 
> sense to NOT publish support and do half-way protection in SW
> (the backstore is formatted with PI).
> 

Correct.

--nab

Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities

[Nicholas A. Bellinger]

On Sat, 2014-03-29 at 04:24 +0300, sagi grimberg wrote:
> On 3/29/2014 3:53 AM, Quinn Tran wrote:
> > +
> > +    if (dev->dev_attrib.pi_prot_type) {
> > +            uint32_t cap[] = { 0,
> > +                               TARGET_DIF_TYPE1_PROTECTION,
> > +                               TARGET_DIF_TYPE2_PROTECTION,
> > +                               TARGET_DIF_TYPE3_PROTECTION};
> > +            uint32_t pt_bits = cap[dev->dev_attrib.pi_prot_type];
> > +            pt_bits &= se_tpg->fabric_sup_prot_type;
> >> At what point should the fabric fill that? it can vary between portals
> >> right?
> > QT> Yes, protection mode can vary between portals. When user select the
> > physical function (via fabric_make_tpg), you know the specific portal
> > based on user input and its capability. This is where Qlogic register its
> > capabilities based on specific hardware.
> >
> >
> >> I would prefer to do that as a function pointer to explicitly ask the
> >> fabric it's support.
> > QT> is it still require with previous answer ?
> >
> 
> Well, I think it is nicer to explicitly ask the fabric at that point 
> instead of checking what it previously set.
> 

I'm currently working on a series that explicitly queries the fabric for
what PI modes are supported, as per our LSF discussion.

> By the way, I think this patch breaks existing iSER support as iSER 
> doesn't set these bits.
> Thats why I think it would be a good idea to *explicitly* ask.

<nod>

> 
> >
> >>> +                    pr_err("dev[%p]: DIF protection mismatch with fabric "
> >>> +                            "(%s). Transport capability bits[0x%x]\n",
> >>> +                            dev, se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name,
> >>> +                            se_tpg->fabric_sup_prot_type);
> >>> +                    return -EFAULT;
> >> Didn't we agree that this is allowed and the target core should
> >> compensate on the lack fabric support?
> > <QT> My recollection was that it's not recommended to have different
> > portals with different supported feature.
> 
> Well we seem to remember different things...
> Anyway I think it is better to compensate that in backstore/target-core 
> level, that would be better
> than silently turn off protection. Martin? Nic? your takes?
> 

At the BoF we concluded that when a backend has PI enabled, but the
fabric does not support PI, that target-core should strip off the
INQUIRY + other control bits that normally indicate protection, but only
on that particular path (eg: session).

This would allow different iSCSI network portals to set a se_session bit
at login time in order to indicate if/when protection is supported at
the fabric nexus level.

If it wasn't for iscsi-target / iser-target sharing the same endpoint
across different fabrics, the PI information could simply be queried on
a per /sys/kernel/config/target/$FABRIC/$WWPN/$TPGT endpoint basis, but
alas it's not that simple..

> Also I don't know what rats are hiding here if the backstore is handling 
> IOs in this time...
> What about cleaning up all the protection resources the backstore driver 
> might be managing?
> 
> >   Meaning a SCSI Write without Dif
> > down a none-T10PI portal update the data.  The Guard on the disk is now
> > mismatch with the data. A SCSI Read down a T10PI path will run into a
> > Guard failure.
> >
> > By introducing this option, Disk vendor require additional logic to
> > compensate for this. I think it's cheaper to have supported hardware
> > rather than support nightmare.
> >
> >>> +            }
> >>> +    }
> >>> +
> >>>       if (lun->lun_se_dev !=  NULL) {
> >>>               pr_err("Port Symlink already exists\n");
> >>>               return -EEXIST;
> >>> diff --git a/drivers/target/target_core_tpg.c
> >>> b/drivers/target/target_core_tpg.c
> >>> index c036595..9279971 100644
> >>> --- a/drivers/target/target_core_tpg.c
> >>> +++ b/drivers/target/target_core_tpg.c
> >>> @@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
> >>>    }
> >>>    EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
> >>>
> >>> +void core_tpg_set_fabric_t10dif(
> >>> +    struct se_portal_group *tpg,
> >>> +    uint32_t fabric_t10dif_force_on)
> >>> +{
> >>> +    tpg->fabric_t10dif_force_on = fabric_t10dif_force_on;
> >>> +}
> >>> +EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
> >>> +
> >> Is there a user for this function in this patch?
> > QT> I'm on the fence with this piece.  Just thinking of a case where DIX
> > is not available for initiator side, but user wants to turn on protection
> > at the link layer.  Our test folks would like to cover this case in the
> > future.
> 
> Not sure I understand. Initiator will send the target data+protection 
> (DIX disabled HBA does INSERT), why does this help?
> Why should the target fabric care who generated the protection (OS or HBA)?
> 

So this is the case where the fabric is responsible for doing the WRITE
INSERT + READ_STRIP (which could be in hardware or software), but the
INQUIRY PROTECT bit + friends still need to be masked (on that
particular session) so the initiator does not generate PI information
the host side LLD cannot handle.

--nab

[PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[Quinn Tran]

Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
capabilities bits to let TCM core knows of HW/fabric capabilities.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++++++++++++++++++++++
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b23a0ff..4d93081 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -910,12 +910,20 @@ DEF_QLA_TPG_ATTR_BOOL(demo_mode_login_only);
 DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
 QLA_TPG_ATTR(demo_mode_login_only, S_IRUGO | S_IWUSR);
 
+/*
+ * Define tcm_qla2xxx_tpg_attrib_s_t10dif_force_on
+ */
+DEF_QLA_TPG_ATTR_BOOL(t10dif_force_on);
+DEF_QLA_TPG_ATTRIB(t10dif_force_on);
+QLA_TPG_ATTR(t10dif_force_on, S_IRUGO | S_IWUSR);
+
 static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
 	&tcm_qla2xxx_tpg_attrib_generate_node_acls.attr,
 	&tcm_qla2xxx_tpg_attrib_cache_dynamic_acls.attr,
 	&tcm_qla2xxx_tpg_attrib_demo_mode_write_protect.attr,
 	&tcm_qla2xxx_tpg_attrib_prod_mode_write_protect.attr,
 	&tcm_qla2xxx_tpg_attrib_demo_mode_login_only.attr,
+	&tcm_qla2xxx_tpg_attrib_t10dif_force_on.attr,
 	NULL,
 };
 
@@ -1049,6 +1057,18 @@ static struct se_portal_group *tcm_qla2xxx_make_tpg(
 	tpg->tpg_attrib.demo_mode_write_protect = 1;
 	tpg->tpg_attrib.cache_dynamic_acls = 1;
 	tpg->tpg_attrib.demo_mode_login_only = 1;
+	tpg->tpg_attrib.t10dif_force_on = 0;
+	tpg->se_tpg.fabric_sup_prot_type = 0;
+	tpg->se_tpg.fabric_sup_guard_type = 0;
+
+	if (scsi_host_get_prot(lport->qla_vha->host)) {
+		tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
+			TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
+			TARGET_DIF_TYPE3_PROT);
+
+		tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
+			TARGET_GUARD_IP;
+	}
 
 	ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
 				&tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
@@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
 		qlt_stop_phase1(vha->vha_tgt.qla_tgt);
 	}
 
+	core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
+
 	return count;
 }
 
@@ -1169,6 +1191,7 @@ static struct se_portal_group *tcm_qla2xxx_npiv_make_tpg(
 	tpg->tpg_attrib.demo_mode_write_protect = 1;
 	tpg->tpg_attrib.cache_dynamic_acls = 1;
 	tpg->tpg_attrib.demo_mode_login_only = 1;
+	tpg->tpg_attrib.t10dif_force_on = 0;
 
 	ret = core_tpg_register(&tcm_qla2xxx_npiv_fabric_configfs->tf_ops, wwn,
 				&tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 33aaac8..62fdce3 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -28,6 +28,7 @@ struct tcm_qla2xxx_tpg_attrib {
 	int demo_mode_write_protect;
 	int prod_mode_write_protect;
 	int demo_mode_login_only;
+	int t10dif_force_on;
 };
 
 struct tcm_qla2xxx_tpg {
-- 
1.8.4.GIT

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[sagi grimberg]

On 3/29/2014 2:05 AM, Quinn Tran wrote:
> Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
> capabilities bits to let TCM core knows of HW/fabric capabilities.
>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
> ---
>   drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++++++++++++++++++++++
>   drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 +
>   2 files changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index b23a0ff..4d93081 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -910,12 +910,20 @@ DEF_QLA_TPG_ATTR_BOOL(demo_mode_login_only);
>   DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
>   QLA_TPG_ATTR(demo_mode_login_only, S_IRUGO | S_IWUSR);
>   
> +/*
> + * Define tcm_qla2xxx_tpg_attrib_s_t10dif_force_on
> + */
> +DEF_QLA_TPG_ATTR_BOOL(t10dif_force_on);
> +DEF_QLA_TPG_ATTRIB(t10dif_force_on);
> +QLA_TPG_ATTR(t10dif_force_on, S_IRUGO | S_IWUSR);
> +
>   static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
>   	&tcm_qla2xxx_tpg_attrib_generate_node_acls.attr,
>   	&tcm_qla2xxx_tpg_attrib_cache_dynamic_acls.attr,
>   	&tcm_qla2xxx_tpg_attrib_demo_mode_write_protect.attr,
>   	&tcm_qla2xxx_tpg_attrib_prod_mode_write_protect.attr,
>   	&tcm_qla2xxx_tpg_attrib_demo_mode_login_only.attr,
> +	&tcm_qla2xxx_tpg_attrib_t10dif_force_on.attr,
>   	NULL,
>   };
>   
> @@ -1049,6 +1057,18 @@ static struct se_portal_group *tcm_qla2xxx_make_tpg(
>   	tpg->tpg_attrib.demo_mode_write_protect = 1;
>   	tpg->tpg_attrib.cache_dynamic_acls = 1;
>   	tpg->tpg_attrib.demo_mode_login_only = 1;
> +	tpg->tpg_attrib.t10dif_force_on = 0;
> +	tpg->se_tpg.fabric_sup_prot_type = 0;
> +	tpg->se_tpg.fabric_sup_guard_type = 0;

You can lose guard_type - this is more relevant to the initiator side.

> +
> +	if (scsi_host_get_prot(lport->qla_vha->host)) {
> +		tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
> +			TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
> +			TARGET_DIF_TYPE3_PROT);
> +
> +		tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
> +			TARGET_GUARD_IP;
> +	}
>   
>   	ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
>   				&tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> @@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
>   		qlt_stop_phase1(vha->vha_tgt.qla_tgt);
>   	}
>   
> +	core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
> +

Any way we can get this logic to be shared also with iscsi, srp, etc... 
all fabrics should
be set with t10dif right? so I would imagine it would be better to 
centralize it right?

>   	return count;
>   }
>   
> @@ -1169,6 +1191,7 @@ static struct se_portal_group *tcm_qla2xxx_npiv_make_tpg(
>   	tpg->tpg_attrib.demo_mode_write_protect = 1;
>   	tpg->tpg_attrib.cache_dynamic_acls = 1;
>   	tpg->tpg_attrib.demo_mode_login_only = 1;
> +	tpg->tpg_attrib.t10dif_force_on = 0;
>   
>   	ret = core_tpg_register(&tcm_qla2xxx_npiv_fabric_configfs->tf_ops, wwn,
>   				&tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
> index 33aaac8..62fdce3 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
> @@ -28,6 +28,7 @@ struct tcm_qla2xxx_tpg_attrib {
>   	int demo_mode_write_protect;
>   	int prod_mode_write_protect;
>   	int demo_mode_login_only;
> +	int t10dif_force_on;
>   };
>   
>   struct tcm_qla2xxx_tpg {

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 4597 bytes --]


Regards,
Quinn Tran




On 3/28/14 5:12 PM, "sagi grimberg" <sagig@mellanox.com> wrote:

>On 3/29/2014 2:05 AM, Quinn Tran wrote:
>> Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
>> capabilities bits to let TCM core knows of HW/fabric capabilities.
>>
>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
>> ---
>>   drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++++++++++++++++++++++
>>   drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 +
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>>b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> index b23a0ff..4d93081 100644
>> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> @@ -910,12 +910,20 @@ DEF_QLA_TPG_ATTR_BOOL(demo_mode_login_only);
>>   DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
>>   QLA_TPG_ATTR(demo_mode_login_only, S_IRUGO | S_IWUSR);
>>
>> +/*
>> + * Define tcm_qla2xxx_tpg_attrib_s_t10dif_force_on
>> + */
>> +DEF_QLA_TPG_ATTR_BOOL(t10dif_force_on);
>> +DEF_QLA_TPG_ATTRIB(t10dif_force_on);
>> +QLA_TPG_ATTR(t10dif_force_on, S_IRUGO | S_IWUSR);
>> +
>>   static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
>>      &tcm_qla2xxx_tpg_attrib_generate_node_acls.attr,
>>      &tcm_qla2xxx_tpg_attrib_cache_dynamic_acls.attr,
>>      &tcm_qla2xxx_tpg_attrib_demo_mode_write_protect.attr,
>>      &tcm_qla2xxx_tpg_attrib_prod_mode_write_protect.attr,
>>      &tcm_qla2xxx_tpg_attrib_demo_mode_login_only.attr,
>> +    &tcm_qla2xxx_tpg_attrib_t10dif_force_on.attr,
>>      NULL,
>>   };
>>
>> @@ -1049,6 +1057,18 @@ static struct se_portal_group
>>*tcm_qla2xxx_make_tpg(
>>      tpg->tpg_attrib.demo_mode_write_protect = 1;
>>      tpg->tpg_attrib.cache_dynamic_acls = 1;
>>      tpg->tpg_attrib.demo_mode_login_only = 1;
>> +    tpg->tpg_attrib.t10dif_force_on = 0;
>> +    tpg->se_tpg.fabric_sup_prot_type = 0;
>> +    tpg->se_tpg.fabric_sup_guard_type = 0;
>
>You can lose guard_type - this is more relevant to the initiator side.

QT> OK

>
>> +
>> +    if (scsi_host_get_prot(lport->qla_vha->host)) {
>> +            tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
>> +                    TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
>> +                    TARGET_DIF_TYPE3_PROT);
>> +
>> +            tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
>> +                    TARGET_GUARD_IP;
>> +    }
>>
>>      ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
>>                              &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
>> @@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
>>              qlt_stop_phase1(vha->vha_tgt.qla_tgt);
>>      }
>>
>> +    core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
>> +
>
>Any way we can get this logic to be shared also with iscsi, srp, etc...
>all fabrics should
>be set with t10dif right? so I would imagine it would be better to
>centralize it right?

QT> Not sure how you want this logic to be shared.  This patch is specific
to Qlogic driver registering its capabilities.


>
>>      return count;
>>   }
>>
>> @@ -1169,6 +1191,7 @@ static struct se_portal_group
>>*tcm_qla2xxx_npiv_make_tpg(
>>      tpg->tpg_attrib.demo_mode_write_protect = 1;
>>      tpg->tpg_attrib.cache_dynamic_acls = 1;
>>      tpg->tpg_attrib.demo_mode_login_only = 1;
>> +    tpg->tpg_attrib.t10dif_force_on = 0;
>>
>>      ret = core_tpg_register(&tcm_qla2xxx_npiv_fabric_configfs->tf_ops,
>>wwn,
>>                              &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
>> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
>>b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
>> index 33aaac8..62fdce3 100644
>> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
>> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
>> @@ -28,6 +28,7 @@ struct tcm_qla2xxx_tpg_attrib {
>>      int demo_mode_write_protect;
>>      int prod_mode_write_protect;
>>      int demo_mode_login_only;
>> +    int t10dif_force_on;
>>   };
>>
>>   struct tcm_qla2xxx_tpg {


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 5692 bytes --]

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[Nicholas A. Bellinger]

 On Mon, 2014-03-31 at 15:38 +0000, Quinn Tran wrote:
> On 3/28/14 5:12 PM, "sagi grimberg" <sagig@mellanox.com> wrote:
> 
> >On 3/29/2014 2:05 AM, Quinn Tran wrote:
> >> Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
> >> capabilities bits to let TCM core knows of HW/fabric capabilities.
> >>
> >> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
> >> ---
> >>   drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++++++++++++++++++++++
> >>   drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 +
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> >>b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> >> index b23a0ff..4d93081 100644
> >> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> >> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c

<SNIP>

> >> +
> >> +    if (scsi_host_get_prot(lport->qla_vha->host)) {
> >> +            tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
> >> +                    TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
> >> +                    TARGET_DIF_TYPE3_PROT);
> >> +
> >> +            tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
> >> +                    TARGET_GUARD_IP;
> >> +    }
> >>
> >>      ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
> >>                              &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> >> @@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
> >>              qlt_stop_phase1(vha->vha_tgt.qla_tgt);
> >>      }
> >>
> >> +    core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
> >> +
> >
> >Any way we can get this logic to be shared also with iscsi, srp, etc...
> >all fabrics should
> >be set with t10dif right? so I would imagine it would be better to
> >centralize it right?
> 
> QT> Not sure how you want this logic to be shared.  This patch is specific
> to Qlogic driver registering its capabilities.
> 

I think that Sagi was referring to a target_core_fabric_ops callback to
query protection information from the fabric..

As mentioned in the last response, this would work just fine on
a /sys/kernel/config/target/$FABRIC/$WWPN/$TPGT context basis, if it
wasn't for iscsi-target / iser-target sharing the same endpoint while
still allowing different protection modes.

--nab

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[Sagi Grimberg]

On 4/1/2014 4:11 AM, Nicholas A. Bellinger wrote:
<SNIP>
>>>> +
>>>> +    if (scsi_host_get_prot(lport->qla_vha->host)) {
>>>> +            tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
>>>> +                    TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
>>>> +                    TARGET_DIF_TYPE3_PROT);
>>>> +
>>>> +            tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
>>>> +                    TARGET_GUARD_IP;
>>>> +    }
>>>>
>>>>       ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
>>>>                               &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
>>>> @@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
>>>>               qlt_stop_phase1(vha->vha_tgt.qla_tgt);
>>>>       }
>>>>
>>>> +    core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
>>>> +
>>> Any way we can get this logic to be shared also with iscsi, srp, etc...
>>> all fabrics should
>>> be set with t10dif right? so I would imagine it would be better to
>>> centralize it right?
>> QT> Not sure how you want this logic to be shared.  This patch is specific
>> to Qlogic driver registering its capabilities.
>>
> I think that Sagi was referring to a target_core_fabric_ops callback to
> query protection information from the fabric..
>
> As mentioned in the last response, this would work just fine on
> a /sys/kernel/config/target/$FABRIC/$WWPN/$TPGT context basis, if it
> wasn't for iscsi-target / iser-target sharing the same endpoint while
> still allowing different protection modes.

So no way to get it centralized? I still don't understand the iscsi/iser 
constraint.

Sagi.

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[Nicholas A. Bellinger]

On Tue, 2014-04-01 at 11:04 +0300, Sagi Grimberg wrote:
> On 4/1/2014 4:11 AM, Nicholas A. Bellinger wrote:
> <SNIP>
> >>>> +
> >>>> +    if (scsi_host_get_prot(lport->qla_vha->host)) {
> >>>> +            tpg->se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
> >>>> +                    TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
> >>>> +                    TARGET_DIF_TYPE3_PROT);
> >>>> +
> >>>> +            tpg->se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
> >>>> +                    TARGET_GUARD_IP;
> >>>> +    }
> >>>>
> >>>>       ret = core_tpg_register(&tcm_qla2xxx_fabric_configfs->tf_ops, wwn,
> >>>>                               &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> >>>> @@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
> >>>>               qlt_stop_phase1(vha->vha_tgt.qla_tgt);
> >>>>       }
> >>>>
> >>>> +    core_tpg_set_fabric_t10dif(se_tpg, tpg->tpg_attrib.t10dif_force_on);
> >>>> +
> >>> Any way we can get this logic to be shared also with iscsi, srp, etc...
> >>> all fabrics should
> >>> be set with t10dif right? so I would imagine it would be better to
> >>> centralize it right?
> >> QT> Not sure how you want this logic to be shared.  This patch is specific
> >> to Qlogic driver registering its capabilities.
> >>
> > I think that Sagi was referring to a target_core_fabric_ops callback to
> > query protection information from the fabric..
> >
> > As mentioned in the last response, this would work just fine on
> > a /sys/kernel/config/target/$FABRIC/$WWPN/$TPGT context basis, if it
> > wasn't for iscsi-target / iser-target sharing the same endpoint while
> > still allowing different protection modes.
> 
> So no way to get it centralized?

Yes, still working on how that might look..

>  I still don't understand the iscsi/iser constraint.

Every other fabric aside from iscsi/iser could simply provide a
TFO->get_fabric_prot(se_tpg) to query for supported PI.

The reason why iscsi/iser is unique is because we can have different
network portals (eg: iser protected + traditional iscsi unprotected) on
the same se_tpg endpoint.

--nab

Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability

[sagi grimberg]

On 4/1/2014 8:40 PM, Nicholas A. Bellinger wrote:
> <SNIP>
>> So no way to get it centralized?
> Yes, still working on how that might look..
>
>>   I still don't understand the iscsi/iser constraint.
> Every other fabric aside from iscsi/iser could simply provide a
> TFO->get_fabric_prot(se_tpg) to query for supported PI.
>
> The reason why iscsi/iser is unique is because we can have different
> network portals (eg: iser protected + traditional iscsi unprotected) on
> the same se_tpg endpoint.

I see. That seems solvable to me... perhaps the easiest thing is to just 
go ahead and support T10-PI
in iscsi-tcp (SW)...

Sagi.

[PATCH 3/4] target/rd: T10-Dif: Add init/format support

[Quinn Tran]

This patch is borrow code from

commit 0f5e2ec46dd64579c0770f3822764f94db4fa465
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Sat Jan 18 09:32:56 2014 +0000

    target/file: Add DIF protection init/format support

    This patch adds support for DIF protection init/format support into
    the FILEIO backend.

    It involves using a seperate $FILE.protection for storing PI that is
    opened via fd_init_prot() using the common pi_prot_type attribute.
    The actual formatting of the protection is done via fd_format_prot()
    using the common pi_prot_format attribute, that will populate the
    initial PI data based upon the currently configured pi_prot_type.

    Based on original FILEIO code from Sagi.

    v1 changes:
      - Fix sparse warnings in fd_init_format_buf (Fengguang)

    Cc: Martin K. Petersen <martin.petersen@oracle.com>
    Cc: Christoph Hellwig <hch@lst.de>
    Cc: Hannes Reinecke <hare@suse.de>
    Cc: Sagi Grimberg <sagig@mellanox.com>
    Cc: Or Gerlitz <ogerlitz@mellanox.com>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_rd.c | 64 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 66a5aba..01dda0b 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -603,7 +603,7 @@ static int rd_init_prot(struct se_device *dev)
 {
 	struct rd_dev *rd_dev = RD_DEV(dev);
 
-        if (!dev->dev_attrib.pi_prot_type)
+	if (!dev->dev_attrib.pi_prot_type)
 		return 0;
 
 	return rd_build_prot_space(rd_dev, dev->prot_length);
@@ -616,6 +616,67 @@ static void rd_free_prot(struct se_device *dev)
 	rd_release_prot_space(rd_dev);
 }
 
+static void rd_init_format_buf(struct se_device *dev, unsigned char *buf,
+			       u32 unit_size, u32 *ref_tag, u16 app_tag,
+			       bool inc_reftag)
+{
+	unsigned char *p = buf;
+	int i;
+
+	for (i = 0; i < unit_size; i += dev->prot_length) {
+		*((u16 *)&p[0]) = 0xffff;
+		*((__be16 *)&p[2]) = cpu_to_be16(app_tag);
+		*((__be32 *)&p[4]) = cpu_to_be32(*ref_tag);
+
+		if (inc_reftag)
+			(*ref_tag)++;
+
+		p += dev->prot_length;
+	}
+}
+
+static int rd_format_prot(struct se_device *dev)
+{
+	struct rd_dev *rd_dev = RD_DEV(dev);
+	u32 ref_tag = 0;
+	int i,j;
+	bool inc_reftag = false;
+	struct rd_dev_sg_table *pt;
+	struct scatterlist *sg;
+	void *paddr;
+
+	if (!dev->dev_attrib.pi_prot_type) {
+		pr_err("Unable to format_prot while pi_prot_type == 0\n");
+		return -ENODEV;
+	}
+
+	switch (dev->dev_attrib.pi_prot_type) {
+	case TARGET_DIF_TYPE3_PROT:
+		ref_tag = 0xffffffff;
+		break;
+	case TARGET_DIF_TYPE2_PROT:
+	case TARGET_DIF_TYPE1_PROT:
+		inc_reftag = true;
+		break;
+	default:
+		break;
+	}
+
+	for (i=0; i < rd_dev->sg_prot_count; i++) {
+		pt= &rd_dev->sg_prot_array[i];
+		for_each_sg(pt->sg_table, sg, pt->rd_sg_count, j) {
+			paddr = kmap(sg_page(sg)) + sg->offset;
+
+			rd_init_format_buf(dev, paddr, sg->length, &ref_tag,
+					0xffff, inc_reftag);
+			kunmap(paddr);
+		}
+	}
+
+	return 0;
+}
+
+
 static struct sbc_ops rd_sbc_ops = {
 	.execute_rw		= rd_execute_rw,
 };
@@ -642,6 +703,7 @@ static struct se_subsystem_api rd_mcp_template = {
 	.get_device_type	= sbc_get_device_type,
 	.get_blocks		= rd_get_blocks,
 	.init_prot		= rd_init_prot,
+	.format_prot            = rd_format_prot,
 	.free_prot		= rd_free_prot,
 };
 
-- 
1.8.4.GIT

Re: [PATCH 3/4] target/rd: T10-Dif: Add init/format support

[sagi grimberg]

On 3/29/2014 2:05 AM, Quinn Tran wrote:
> This patch is borrow code from
>
> commit 0f5e2ec46dd64579c0770f3822764f94db4fa465
> Author: Nicholas Bellinger <nab@linux-iscsi.org>
> Date:   Sat Jan 18 09:32:56 2014 +0000
>
>      target/file: Add DIF protection init/format support
>
>      This patch adds support for DIF protection init/format support into
>      the FILEIO backend.
>
>      It involves using a seperate $FILE.protection for storing PI that is
>      opened via fd_init_prot() using the common pi_prot_type attribute.
>      The actual formatting of the protection is done via fd_format_prot()
>      using the common pi_prot_format attribute, that will populate the
>      initial PI data based upon the currently configured pi_prot_type.
>
>      Based on original FILEIO code from Sagi.
>
>      v1 changes:
>        - Fix sparse warnings in fd_init_format_buf (Fengguang)
>
>      Cc: Martin K. Petersen <martin.petersen@oracle.com>
>      Cc: Christoph Hellwig <hch@lst.de>
>      Cc: Hannes Reinecke <hare@suse.de>
>      Cc: Sagi Grimberg <sagig@mellanox.com>
>      Cc: Or Gerlitz <ogerlitz@mellanox.com>
>      Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_rd.c | 64 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> index 66a5aba..01dda0b 100644
> --- a/drivers/target/target_core_rd.c
> +++ b/drivers/target/target_core_rd.c
> @@ -603,7 +603,7 @@ static int rd_init_prot(struct se_device *dev)
>   {
>   	struct rd_dev *rd_dev = RD_DEV(dev);
>   
> -        if (!dev->dev_attrib.pi_prot_type)
> +	if (!dev->dev_attrib.pi_prot_type)
>   		return 0;
>   
>   	return rd_build_prot_space(rd_dev, dev->prot_length);
> @@ -616,6 +616,67 @@ static void rd_free_prot(struct se_device *dev)
>   	rd_release_prot_space(rd_dev);
>   }
>   
> +static void rd_init_format_buf(struct se_device *dev, unsigned char *buf,
> +			       u32 unit_size, u32 *ref_tag, u16 app_tag,
> +			       bool inc_reftag)
> +{
> +	unsigned char *p = buf;
> +	int i;
> +
> +	for (i = 0; i < unit_size; i += dev->prot_length) {
> +		*((u16 *)&p[0]) = 0xffff;
> +		*((__be16 *)&p[2]) = cpu_to_be16(app_tag);
> +		*((__be32 *)&p[4]) = cpu_to_be32(*ref_tag);
> +
> +		if (inc_reftag)
> +			(*ref_tag)++;
> +
> +		p += dev->prot_length;
> +	}
> +}
> +
> +static int rd_format_prot(struct se_device *dev)
> +{
> +	struct rd_dev *rd_dev = RD_DEV(dev);
> +	u32 ref_tag = 0;
> +	int i,j;
> +	bool inc_reftag = false;
> +	struct rd_dev_sg_table *pt;
> +	struct scatterlist *sg;
> +	void *paddr;
> +
> +	if (!dev->dev_attrib.pi_prot_type) {
> +		pr_err("Unable to format_prot while pi_prot_type == 0\n");
> +		return -ENODEV;
> +	}
> +
> +	switch (dev->dev_attrib.pi_prot_type) {
> +	case TARGET_DIF_TYPE3_PROT:
> +		ref_tag = 0xffffffff;
> +		break;
> +	case TARGET_DIF_TYPE2_PROT:
> +	case TARGET_DIF_TYPE1_PROT:
> +		inc_reftag = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	for (i=0; i < rd_dev->sg_prot_count; i++) {
> +		pt= &rd_dev->sg_prot_array[i];
> +		for_each_sg(pt->sg_table, sg, pt->rd_sg_count, j) {
> +			paddr = kmap(sg_page(sg)) + sg->offset;
> +
> +			rd_init_format_buf(dev, paddr, sg->length, &ref_tag,
> +					0xffff, inc_reftag);
> +			kunmap(paddr);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
>   static struct sbc_ops rd_sbc_ops = {
>   	.execute_rw		= rd_execute_rw,
>   };
> @@ -642,6 +703,7 @@ static struct se_subsystem_api rd_mcp_template = {
>   	.get_device_type	= sbc_get_device_type,
>   	.get_blocks		= rd_get_blocks,
>   	.init_prot		= rd_init_prot,
> +	.format_prot            = rd_format_prot,
>   	.free_prot		= rd_free_prot,
>   };
>   

This patch is redundant altogether. rd_mcp already init protection with 
escape values:
rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);

Re: [PATCH 3/4] target/rd: T10-Dif: Add init/format support

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 5186 bytes --]


Regards,
Quinn Tran




On 3/28/14 5:16 PM, "sagi grimberg" <sagig@mellanox.com> wrote:

>On 3/29/2014 2:05 AM, Quinn Tran wrote:
>> This patch is borrow code from
>>
>> commit 0f5e2ec46dd64579c0770f3822764f94db4fa465
>> Author: Nicholas Bellinger <nab@linux-iscsi.org>
>> Date:   Sat Jan 18 09:32:56 2014 +0000
>>
>>      target/file: Add DIF protection init/format support
>>
>>      This patch adds support for DIF protection init/format support into
>>      the FILEIO backend.
>>
>>      It involves using a seperate $FILE.protection for storing PI that
>>is
>>      opened via fd_init_prot() using the common pi_prot_type attribute.
>>      The actual formatting of the protection is done via
>>fd_format_prot()
>>      using the common pi_prot_format attribute, that will populate the
>>      initial PI data based upon the currently configured pi_prot_type.
>>
>>      Based on original FILEIO code from Sagi.
>>
>>      v1 changes:
>>        - Fix sparse warnings in fd_init_format_buf (Fengguang)
>>
>>      Cc: Martin K. Petersen <martin.petersen@oracle.com>
>>      Cc: Christoph Hellwig <hch@lst.de>
>>      Cc: Hannes Reinecke <hare@suse.de>
>>      Cc: Sagi Grimberg <sagig@mellanox.com>
>>      Cc: Or Gerlitz <ogerlitz@mellanox.com>
>>      Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> ---
>>   drivers/target/target_core_rd.c | 64
>>++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_rd.c
>>b/drivers/target/target_core_rd.c
>> index 66a5aba..01dda0b 100644
>> --- a/drivers/target/target_core_rd.c
>> +++ b/drivers/target/target_core_rd.c
>> @@ -603,7 +603,7 @@ static int rd_init_prot(struct se_device *dev)
>>   {
>>      struct rd_dev *rd_dev = RD_DEV(dev);
>>
>> -        if (!dev->dev_attrib.pi_prot_type)
>> +    if (!dev->dev_attrib.pi_prot_type)
>>              return 0;
>>
>>      return rd_build_prot_space(rd_dev, dev->prot_length);
>> @@ -616,6 +616,67 @@ static void rd_free_prot(struct se_device *dev)
>>      rd_release_prot_space(rd_dev);
>>   }
>>
>> +static void rd_init_format_buf(struct se_device *dev, unsigned char
>>*buf,
>> +                           u32 unit_size, u32 *ref_tag, u16 app_tag,
>> +                           bool inc_reftag)
>> +{
>> +    unsigned char *p = buf;
>> +    int i;
>> +
>> +    for (i = 0; i < unit_size; i += dev->prot_length) {
>> +            *((u16 *)&p[0]) = 0xffff;
>> +            *((__be16 *)&p[2]) = cpu_to_be16(app_tag);
>> +            *((__be32 *)&p[4]) = cpu_to_be32(*ref_tag);
>> +
>> +            if (inc_reftag)
>> +                    (*ref_tag)++;
>> +
>> +            p += dev->prot_length;
>> +    }
>> +}
>> +
>> +static int rd_format_prot(struct se_device *dev)
>> +{
>> +    struct rd_dev *rd_dev = RD_DEV(dev);
>> +    u32 ref_tag = 0;
>> +    int i,j;
>> +    bool inc_reftag = false;
>> +    struct rd_dev_sg_table *pt;
>> +    struct scatterlist *sg;
>> +    void *paddr;
>> +
>> +    if (!dev->dev_attrib.pi_prot_type) {
>> +            pr_err("Unable to format_prot while pi_prot_type == 0\n");
>> +            return -ENODEV;
>> +    }
>> +
>> +    switch (dev->dev_attrib.pi_prot_type) {
>> +    case TARGET_DIF_TYPE3_PROT:
>> +            ref_tag = 0xffffffff;
>> +            break;
>> +    case TARGET_DIF_TYPE2_PROT:
>> +    case TARGET_DIF_TYPE1_PROT:
>> +            inc_reftag = true;
>> +            break;
>> +    default:
>> +            break;
>> +    }
>> +
>> +    for (i=0; i < rd_dev->sg_prot_count; i++) {
>> +            pt= &rd_dev->sg_prot_array[i];
>> +            for_each_sg(pt->sg_table, sg, pt->rd_sg_count, j) {
>> +                    paddr = kmap(sg_page(sg)) + sg->offset;
>> +
>> +                    rd_init_format_buf(dev, paddr, sg->length, &ref_tag,
>> +                                    0xffff, inc_reftag);
>> +                    kunmap(paddr);
>> +            }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   static struct sbc_ops rd_sbc_ops = {
>>      .execute_rw             = rd_execute_rw,
>>   };
>> @@ -642,6 +703,7 @@ static struct se_subsystem_api rd_mcp_template = {
>>      .get_device_type        = sbc_get_device_type,
>>      .get_blocks             = rd_get_blocks,
>>      .init_prot              = rd_init_prot,
>> +    .format_prot            = rd_format_prot,
>>      .free_prot              = rd_free_prot,
>>   };
>>
>
>This patch is redundant altogether. rd_mcp already init protection with
>escape values:
>rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);

QT> I see.  Will re-examine my code to live without this function.

>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6192 bytes --]

[PATCH 4/4] target/rd: T10-Dif: RAM disk is allocating more space than required.

[Quinn Tran]

Ram disk is allocating 8x more space than required for diff data.
For large RAM disk test, there is small potential for memory
starvation.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
---
 drivers/target/target_core_rd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 01dda0b..6df177a 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length)
 	if (rd_dev->rd_flags & RDF_NULLIO)
 		return 0;
 
-	total_sg_needed = rd_dev->rd_page_count / prot_length;
+	/* prot_length=8byte dif data
+	 * tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) + pad
+	 * PGSZ canceled each other.
+	 */
+	total_sg_needed = (rd_dev->rd_page_count * prot_length / 512) +1;
 
 	sg_tables = (total_sg_needed / max_sg_per_table) + 1;
 
-- 
1.8.4.GIT

Re: [PATCH 4/4] target/rd: T10-Dif: RAM disk is allocating more space than required.

[sagi grimberg]

On 3/29/2014 2:05 AM, Quinn Tran wrote:
> Ram disk is allocating 8x more space than required for diff data.
> For large RAM disk test, there is small potential for memory
> starvation.
>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
> ---
>   drivers/target/target_core_rd.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> index 01dda0b..6df177a 100644
> --- a/drivers/target/target_core_rd.c
> +++ b/drivers/target/target_core_rd.c
> @@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length)
>   	if (rd_dev->rd_flags & RDF_NULLIO)
>   		return 0;
>   
> -	total_sg_needed = rd_dev->rd_page_count / prot_length;
> +	/* prot_length=8byte dif data
> +	 * tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) + pad
> +	 * PGSZ canceled each other.
> +	 */
> +	total_sg_needed = (rd_dev->rd_page_count * prot_length / 512) +1;

You probably will want to use block_size instead of hard-coding 512.
Other then that this makes sense.

>   
>   	sg_tables = (total_sg_needed / max_sg_per_table) + 1;
>   

Re: [PATCH 4/4] target/rd: T10-Dif: RAM disk is allocating more space than required.

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]


Regards,
Quinn Tran




On 3/28/14 5:22 PM, "sagi grimberg" <sagig@mellanox.com> wrote:

>On 3/29/2014 2:05 AM, Quinn Tran wrote:
>> Ram disk is allocating 8x more space than required for diff data.
>> For large RAM disk test, there is small potential for memory
>> starvation.
>>
>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
>> ---
>>   drivers/target/target_core_rd.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_rd.c
>>b/drivers/target/target_core_rd.c
>> index 01dda0b..6df177a 100644
>> --- a/drivers/target/target_core_rd.c
>> +++ b/drivers/target/target_core_rd.c
>> @@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev
>>*rd_dev, int prot_length)
>>      if (rd_dev->rd_flags & RDF_NULLIO)
>>              return 0;
>>
>> -    total_sg_needed = rd_dev->rd_page_count / prot_length;
>> +    /* prot_length=8byte dif data
>> +     * tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) +
>>pad
>> +     * PGSZ canceled each other.
>> +     */
>> +    total_sg_needed = (rd_dev->rd_page_count * prot_length / 512) +1;
>
>You probably will want to use block_size instead of hard-coding 512.
>Other then that this makes sense.

QT> agreed

>
>>
>>      sg_tables = (total_sg_needed / max_sg_per_table) + 1;
>>
>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4961 bytes --]

Re: [PATCH 4/4] target/rd: T10-Dif: RAM disk is allocating more space than required.

[Nicholas A. Bellinger]

Hi Quinn & Co,

On Mon, 2014-03-31 at 16:15 +0000, Quinn Tran wrote:
> On 3/28/14 5:22 PM, "sagi grimberg" <sagig@mellanox.com> wrote:
> 
> >On 3/29/2014 2:05 AM, Quinn Tran wrote:
> >> Ram disk is allocating 8x more space than required for diff data.
> >> For large RAM disk test, there is small potential for memory
> >> starvation.
> >>
> >> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >> Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
> >> ---
> >>   drivers/target/target_core_rd.c | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/target/target_core_rd.c
> >>b/drivers/target/target_core_rd.c
> >> index 01dda0b..6df177a 100644
> >> --- a/drivers/target/target_core_rd.c
> >> +++ b/drivers/target/target_core_rd.c
> >> @@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev
> >>*rd_dev, int prot_length)
> >>      if (rd_dev->rd_flags & RDF_NULLIO)
> >>              return 0;
> >>
> >> -    total_sg_needed = rd_dev->rd_page_count / prot_length;
> >> +    /* prot_length=8byte dif data
> >> +     * tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) +
> >>pad
> >> +     * PGSZ canceled each other.
> >> +     */
> >> +    total_sg_needed = (rd_dev->rd_page_count * prot_length / 512) +1;
> >
> >You probably will want to use block_size instead of hard-coding 512.
> >Other then that this makes sense.
> 
> QT> agreed
> 

Applying the following updated patch to target-pending/for-next, along
with Sagi's comment wrt to block_size.

Also adding your missing Signed-off-by.  Please make sure to include
these in future patches.  ;)

Thanks!

--nab

commit 435b88ba4a38adc39842957610b27a8d0fb4584b
Author: Quinn Tran <quinn.tran@qlogic.com>
Date:   Fri Mar 28 19:05:27 2014 -0400

    target/rd: T10-Dif: RAM disk is allocating more space than required.
    
    Ram disk is allocating 8x more space than required for diff data.
    For large RAM disk test, there is small potential for memory
    starvation.
    
    (Use block_size when calculating total_sg_needed - sagi + nab)
    
    Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
    Signed-off-by: Quinn Tran <quinn.tran@qlogic.com>
    Cc: <stable@vger.kernel.org> #3.14+
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 66a5aba..b920db3 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -242,7 +242,7 @@ static void rd_release_prot_space(struct rd_dev *rd_dev)
        rd_dev->sg_prot_count = 0;
 }
 
-static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length)
+static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length, int block_size)
 {
        struct rd_dev_sg_table *sg_table;
        u32 total_sg_needed, sg_tables;
@@ -252,8 +252,13 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length)
 
        if (rd_dev->rd_flags & RDF_NULLIO)
                return 0;
-
-       total_sg_needed = rd_dev->rd_page_count / prot_length;
+       /*
+        * prot_length=8byte dif data
+        * tot sg needed = rd_page_count * (PGSZ/block_size) *
+        *                 (prot_length/block_size) + pad
+        * PGSZ canceled each other.
+        */
+       total_sg_needed = (rd_dev->rd_page_count * prot_length / block_size) + 1;
 
        sg_tables = (total_sg_needed / max_sg_per_table) + 1;
 
@@ -606,7 +611,8 @@ static int rd_init_prot(struct se_device *dev)
         if (!dev->dev_attrib.pi_prot_type)
                return 0;
 
-       return rd_build_prot_space(rd_dev, dev->prot_length);
+       return rd_build_prot_space(rd_dev, dev->prot_length,
+                                  dev->dev_attrib.block_size);
 }
 
 static void rd_free_prot(struct se_device *dev)

Re: [PATCH RFC 0/4] add T10-Dif registration for tcm_qla2xxx

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

+Sagi
+Martin

Regards,
Quinn Tran




On 3/28/14 4:05 PM, "Quinn Tran" <quinn.tran@qlogic.com> wrote:

>Nicholas,
>
>Per our conversation at LSF, the following are the patch set
>of bug fix/tweak found during T10-Dif testing and add ability
>for QLogic FC driver to register with TCM its T10-PI capabilities.
>
>As for the rest of the other patches that does the actual
>T10-Dif data movement, I will submit them to target-devel and
>stable kernel.
>
>Quinn
>-----
>Quinn Tran (4):
>  target/core: T10-Dif: check HW support capabilities
>  tcm_qla2xxx: T10-Dif set harware capability
>  target/rd: T10-Dif: Add init/format support
>  target/rd: T10-Dif: RAM disk is allocating more space than required.
>
> drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 23 +++++++++
> drivers/scsi/qla2xxx/tcm_qla2xxx.h           |  1 +
> drivers/target/target_core_fabric_configfs.c | 20 ++++++++
> drivers/target/target_core_rd.c              | 70
>+++++++++++++++++++++++++++-
> drivers/target/target_core_tpg.c             |  9 ++++
> include/target/target_core_base.h            | 14 ++++++
> include/target/target_core_fabric.h          |  1 +
> 7 files changed, 136 insertions(+), 2 deletions(-)
>
>--
>1.8.4.GIT
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4685 bytes --]

Re: [PATCH RFC 0/4] add T10-Dif registration for tcm_qla2xxx

[sagi grimberg]

On 3/29/2014 2:48 AM, Quinn Tran wrote:
> +Sagi
> +Martin

Thanks  Quinn!

Already had a look.

> Regards,
> Quinn Tran
>