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

[Quinn Tran <quinn.tran@qlogic.com>]

[-- 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 --]