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

2014-04-02 Thread 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 sa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-02 Thread 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 sa...@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

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

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

2014-04-02 Thread 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 sa...@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

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

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

2014-04-02 Thread Quinn Tran 

Regards,
Quinn Tran




On 4/2/14 11:20 AM, Nicholas A. Bellinger n...@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 sa...@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: winmail.dat

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

2014-04-01 Thread Sagi Grimberg 

On 4/1/2014 4:19 AM, Nicholas A. Bellinger wrote:

On Mon, 2014-03-31 at 17:53 +, Quinn Tran wrote:

On 3/28/14 6:24 PM, sagi grimberg sa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-01 Thread Martin K. Petersen 
 Sagi == Sagi Grimberg sa...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-01 Thread sagi grimberg 

On 4/1/2014 8:09 PM, Martin K. Petersen wrote:

Sagi == Sagi Grimberg sa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-01 Thread 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 sa...@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





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

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

2014-03-31 Thread Quinn Tran 

Regards,
Quinn Tran




On 3/28/14 6:24 PM, sagi grimberg sa...@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: winmail.dat

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

2014-03-31 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in

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

2014-03-31 Thread Nicholas A. Bellinger 
On Mon, 2014-03-31 at 17:53 +, Quinn Tran wrote:
 On 3/28/14 6:24 PM, sagi grimberg sa...@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

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

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

2014-03-28 Thread 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 n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@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_ttpg_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

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

2014-03-28 Thread Quinn Tran 
Answer in line.

Regards,
Quinn Tran




On 3/28/14 5:05 PM, sagi grimberg sa...@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 n...@linux-iscsi.org
 Signed-off-by: Giridhar Malavali giridhar.malav...@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

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

2014-03-28 Thread 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.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html