Re: [PATCH][RFC] iSCSI transport class

[Mike Christie <michaelc@cs.wisc.edu>]

Hey,

Surekha.PC wrote:
>  Hi Mike,
> 
>   Sorry for the delay.
>   The structure which I represented was meant to have iSCSI transport
> attributes
>   under their respective category. Right now, in our iSCSI driver all
> attributes are defined
>   in `iscsi-attr.c` and `iscsi-debug-attr.c` which need to be either per lun
> or per target.
>>> I have reviewed your changes for iSCSI transport class. 
>>
>>The changes 
>>
>>>look fine.  I have few suggestions on further improvements. The 
>>>attributes can be organised
>>> under iSCSI transport class in the following structure.
>>>
>>># ls /sys/class/iscsi_transport/*
>>>
>>>  /* iSCSI host specific attributes */
>>>  |-- shutdown
>>>  |-- host_id
>>>  |-- ....
>>
>>The examples you placed here are not tranport specific. They 
>>are just HBA values, and should stay under the host attributes.
> 
> 
> AFAIU Transport class is basically to hold all private attributes of the
> iSCSI driver.

That is wrong. Look at the qla2xxx or 53c700 driver. The transport
classes represent transport specifics. They are not there for a
driver that is a particular transport to place all its attributes, and
each driver that is a particular transport does not implement its
own class.

> Since iSCSI driver has attributes which are hba/target/lun specific it would
> be good to have them in a place away from the generic scsi attributes.
> For eg, the "shutdown" attribute is a iSCSI attribute to shutdown the iSCSI
> hba.

shutdown is a hba thing, and has nothing to do with iscsi itself.

> Similarly host_id is iSCSI transport specific and gives the host number.

How is the struct scsi_host's host_no related to the transport?
It is assigned from the SCSI layer.

>>Also, the host layout here does not look right. What happens 
>>when I have multiple HBAs?
> 
> Right now iSCSI driver supports single HBA, hence this representation is OK.
> However in multiple HBA case, it can be represented as below.

Again, only specific to your driver.

>  /sys/class/iscsi_transport/
>    |--host1/
>        |-- shutdown_hba
>        |-- show_hba_id
>    |--host2/
>        |-- ---"----
>        |-- ---"----
> 
>   
> 
>>>  /* iSCSI target specific attributes */
>>>  |-- 1:0:1/
>>>           |-- target_addr
>>>           |-- target_name
>>>           |-- target_port
>>>           |-- ....
>>
>>I agree these would be nice under a target attribute. Did
>>you have something to post since the last time we talked,
>>or did you write the example by hand?
> 
> 
> I have written the example by hand, I am working towards implementing it.
>>>  /* lun specific attributes */
>>>  |-- 1:0:1:0/
>>>             |-- device -> ../../../devices/platform/iscsi/1:0:0:0
>>>             |-- driver -> ../../../bus/scsi/drivers/sd
>>>             |-- lun_status
>>>             |-- ....
>>
>>I don't think anything in lun_status is iscsi specific.
> 
> 
> We have lun specific attributes defined in iSCSI driver some of which can be
> used in debug mode for testing purposes e.g initiate command timeout on a
> lun, set lun queue as busy, query the lun status etc.
> I think since they are private to iSCSI, this is the right place holder.

Again, specific to your particular iscsi driver vs transport.

>>> This way the relevant attributes can be used from respective paths.
>>
>>I get your point (you just chose the above attributes based 
>>on name to accentuate this right). However, in your example 
>>it seems like you would want to show the parent-children 
>>relationships like
>>here:
>>
>>/sys/class/iscsi_transport_host
>>|-- host1
>>|   |-- a_host_attribute
>>|   |-- 1:0:1
>>|   |   |-- target_name
>>|   |   |-- 1:0:1:0
>>|   |   |   |- some_lun_attribute
>>
>>Or you could also do something like
>>
>>/sys/class/iscsi_transport_host
>>|-- host1
>>|   |- a_host_attribute
>>|   |- 1:0:1 -> ../iscsi_transport_target/1:0:1
>>|   `- 1:0:2 -> ../iscsi_transport_target/1:0:2
>>/sys/class/iscsi_transport_target
>>|-- 1:0:1
>>|   |- target_name
>>|   |- ../iscsi_transport_lun/1:0:1:0
>>........
>>
>>Making /sys/class/iscsi_transport the parent for everything 
>>just does not look right.
> 
> 
> In the above example do you mean that iscsi_transport_target is registered
> as 
> another transport class along with iscsi_transport_host? Is this OK ?
> 

Your proposed layout is messy. That is all I am saying.


Mike