RE: [PATCH][RFC] iSCSI transport class

Surekha.PC
2004-04-05T13:09:57Z

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.
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.
Similarly host_id is iSCSI transport specific and gives the host number.

> 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.

/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.

> > 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 ?

Thanks,
surekha