Re: [PATCH][RFC] iSCSI transport class

Mike Christie
2004-04-05T20:22:38Z

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