ibmvfc oddities

ibmvfc oddities

[Hannes Reinecke]

Hi Brian,

I was looking at the ibmvfc code (trying to hook up libfc), and have
found this definition:

struct ibmvfc_fcp_rsp_info {
	__be16 reserved;
	u8 rsp_code;
	u8 reserved2[4];
}__attribute__((packed, aligned (2)));

in comparison, libfc has this:

struct fcp_resp_rsp_info {
    __u8      _fr_resvd[3];       /* reserved */
    __u8      rsp_code;           /* Response Info Code */
    __u8      _fr_resvd2[4];      /* reserved */
};

So both look _nearly_ identical, except the missing byte at the start.
It might be inserted due to some compile alignment magic, but I'd rather
not rely on this.
Could you clarify if the two structures really are different, or if this
is a simple oversight?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: ibmvfc oddities

[Brian King]

On 09/21/2017 05:02 AM, Hannes Reinecke wrote:
> Hi Brian,
> 
> I was looking at the ibmvfc code (trying to hook up libfc), and have
> found this definition:
> 
> struct ibmvfc_fcp_rsp_info {
> 	__be16 reserved;
> 	u8 rsp_code;
> 	u8 reserved2[4];
> }__attribute__((packed, aligned (2)));
> 
> in comparison, libfc has this:
> 
> struct fcp_resp_rsp_info {
>     __u8      _fr_resvd[3];       /* reserved */
>     __u8      rsp_code;           /* Response Info Code */
>     __u8      _fr_resvd2[4];      /* reserved */
> };
> 
> So both look _nearly_ identical, except the missing byte at the start.
> It might be inserted due to some compile alignment magic, but I'd rather
> not rely on this.
> Could you clarify if the two structures really are different, or if this
> is a simple oversight?

Looks like a bug to me. We should probably just have ibmvfc use the
libfc definition.

Tyrel - can you do this conversion and run a bit of regression testing?
Looking at the possible values of rsp_code, the most likely place where
this might cause us some issues is in TMF handling. I'm a little
worried this could result in a potential double completion in error
handling in some rare cases.... Tyrel - are you aware of any issues
like that, which this might explain?

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: ibmvfc oddities

[Tyrel Datwyler]

On 09/21/2017 08:13 AM, Brian King wrote:
> On 09/21/2017 05:02 AM, Hannes Reinecke wrote:
>> Hi Brian,
>>
>> I was looking at the ibmvfc code (trying to hook up libfc), and have
>> found this definition:
>>
>> struct ibmvfc_fcp_rsp_info {
>> 	__be16 reserved;
>> 	u8 rsp_code;
>> 	u8 reserved2[4];
>> }__attribute__((packed, aligned (2)));
>>
>> in comparison, libfc has this:
>>
>> struct fcp_resp_rsp_info {
>>     __u8      _fr_resvd[3];       /* reserved */
>>     __u8      rsp_code;           /* Response Info Code */
>>     __u8      _fr_resvd2[4];      /* reserved */
>> };
>>
>> So both look _nearly_ identical, except the missing byte at the start.
>> It might be inserted due to some compile alignment magic, but I'd rather
>> not rely on this.
>> Could you clarify if the two structures really are different, or if this
>> is a simple oversight?
> 
> Looks like a bug to me. We should probably just have ibmvfc use the
> libfc definition.

Yes, after looking at the FC spec we most definitely have it defined wrong, and I'm pretty
certain that we aren't getting saved by any compiler magic.

> 
> Tyrel - can you do this conversion and run a bit of regression testing?
> Looking at the possible values of rsp_code, the most likely place where
> this might cause us some issues is in TMF handling. I'm a little
> worried this could result in a potential double completion in error
> handling in some rare cases.... Tyrel - are you aware of any issues
> like that, which this might explain?

I certainly can. I recollect something like a double completion issue, but its been so
long I can't remember if we were seeing it in the vfc driver or the vscsi driver. Anyhow,
I do feel like from what I recall it seems like rsp_code is always zero in reported error
messages.

-Tyrel

> 
> Thanks,
> 
> Brian
> 

Re: ibmvfc oddities

[Hannes Reinecke]

On 09/21/2017 08:29 PM, Tyrel Datwyler wrote:
> On 09/21/2017 08:13 AM, Brian King wrote:
>> On 09/21/2017 05:02 AM, Hannes Reinecke wrote:
>>> Hi Brian,
>>>
>>> I was looking at the ibmvfc code (trying to hook up libfc), and have
>>> found this definition:
>>>
>>> struct ibmvfc_fcp_rsp_info {
>>> 	__be16 reserved;
>>> 	u8 rsp_code;
>>> 	u8 reserved2[4];
>>> }__attribute__((packed, aligned (2)));
>>>
>>> in comparison, libfc has this:
>>>
>>> struct fcp_resp_rsp_info {
>>>     __u8      _fr_resvd[3];       /* reserved */
>>>     __u8      rsp_code;           /* Response Info Code */
>>>     __u8      _fr_resvd2[4];      /* reserved */
>>> };
>>>
>>> So both look _nearly_ identical, except the missing byte at the start.
>>> It might be inserted due to some compile alignment magic, but I'd rather
>>> not rely on this.
>>> Could you clarify if the two structures really are different, or if this
>>> is a simple oversight?
>>
>> Looks like a bug to me. We should probably just have ibmvfc use the
>> libfc definition.
> 
> Yes, after looking at the FC spec we most definitely have it defined wrong, and I'm pretty
> certain that we aren't getting saved by any compiler magic.
> 
>>
>> Tyrel - can you do this conversion and run a bit of regression testing?
>> Looking at the possible values of rsp_code, the most likely place where
>> this might cause us some issues is in TMF handling. I'm a little
>> worried this could result in a potential double completion in error
>> handling in some rare cases.... Tyrel - are you aware of any issues
>> like that, which this might explain?
> 
> I certainly can. I recollect something like a double completion issue, but its been so
> long I can't remember if we were seeing it in the vfc driver or the vscsi driver. Anyhow,
> I do feel like from what I recall it seems like rsp_code is always zero in reported error
> messages.
> 
Sure it would, given that the ibmvfc driver would always use the
reserved bytes as rsp_code :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)