iSCSI transport class

RFC PATCH In review Mike Christie 4 participants 7 messages Mar 26, 2004 – Apr 6, 2004

Subsystems (2)

SCSI iSCSI

lore

Share & tools

lore

Mike Christie

author

2004-03-26 20:50 UTC #0 lore raw

RFC PATCH 1/1 iSCSI transport class

Attached is the begining of an iSCSI transport class. Currently, the Cisco driver has some sysfs issues, so the patch in the next mail will convert their print_info attribute to use this class instead.

This patch was built against 2.6.5-rc2. And, here is a tree. Not much yet, so please send comments. Maybe it should be per session instead of per device?

[mc@minna 1:0:0:0]$ pwd
/sys/class/iscsi_transport/1:0:0:0
[mc@minna 1:0:0:0]$ tree
.

|-- device -> ../../../devices/platform/iscsi/1:0:0:0
|-- driver -> ../../../bus/scsi/drivers/sd
|-- target_addr
|-- target_name

`-- target_port

Mike Christie

drivers/scsi/Kconfig +8 -0

diff -Naurp linux-2.6.5-rc2/drivers/scsi/Kconfig linux-2.6.5-rc2-iscsi/drivers/scsi/Kconfig
@@ -214,6 +214,14 @@ config SCSI_FC_ATTRS
 	  each attached FiberChannel device to sysfs, say Y.
 	  Otherwise, say N.
 
+config SCSI_ISCSI_ATTRS
+	tristate "iSCSI Transport Attributes"
+	depends on SCSI
+	help
+	  If you wish to export transport-specific information about
+	  each attached iSCSI device to sysfs, say Y.
+	  Otherwise, say N.
+
 endmenu
 
 menu "SCSI low-level drivers"

drivers/scsi/Makefile +1 -0

diff -Naurp linux-2.6.5-rc2/drivers/scsi/Makefile linux-2.6.5-rc2-iscsi/drivers/scsi/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SCSI)		+= scsi_mod.o
 # --------------------------
 obj-$(CONFIG_SCSI_SPI_ATTRS)	+= scsi_transport_spi.o
 obj-$(CONFIG_SCSI_FC_ATTRS) 	+= scsi_transport_fc.o
+obj-$(CONFIG_SCSI_ISCSI_ATTRS)	+= scsi_transport_iscsi.o
 
 
 obj-$(CONFIG_SCSI_AMIGA7XX)	+= amiga7xx.o	53c7xx.o

drivers/scsi/scsi_transport_iscsi.c +133 -0

133 lines not on this page · raw message

include/scsi/scsi_transport_iscsi.h +41 -0

41 lines not on this page · raw message

Surekha.PC

2004-03-30 14:21 UTC #1 lore raw

Hi Mike,

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

/* iSCSI target specific attributes */
|-- 1:0:1/

           |-- target_addr
           |-- target_name
           |-- target_port
           |-- ....

/* lun specific attributes */
|-- 1:0:1:0/

             |-- device -> ../../../devices/platform/iscsi/1:0:0:0
             |-- driver -> ../../../bus/scsi/drivers/sd
             |-- lun_status
             |-- ....

This way the relevant attributes can be used from respective paths.

Thanks,
surekha

Quoted 29 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Mike Christie
Sent: Saturday, March 27, 2004 2:21 AM
To: SCSI Mailing List; [email protected]
Subject: [PATCH][RFC] iSCSI transport class
Attached is the begining of an iSCSI transport class. Currently, the Cisco driver has some sysfs issues, so the patch in the next mail will convert their print_info attribute to use this class instead.
This patch was built against 2.6.5-rc2. And, here is a tree. Not much yet, so please send comments. Maybe it should be per session instead of per device?
[mc@minna 1:0:0:0]$ pwd
/sys/class/iscsi_transport/1:0:0:0
[mc@minna 1:0:0:0]$ tree
.
|-- device -> ../../../devices/platform/iscsi/1:0:0:0
|-- driver -> ../../../bus/scsi/drivers/sd
|-- target_addr
|-- target_name
`-- target_port
Mike Christie

Mike Christie

author

2004-03-30 21:02 UTC #2 lore raw

Hey Surekha,

Thanks for the comments.

Quoted 8 lines · Surekha.PC

Hi Mike,
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.

Quoted 6 lines

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

Also, the host layout here does not look right. What happens when I have multiple HBAs?

Quoted 6 lines

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

Quoted 6 lines

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

Quoted 2 lines

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.

Mike

Surekha.PC

2004-04-05 13:09 UTC #3 lore raw

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.

Quoted 8 lines

Quoted 1 line
I have reviewed your changes for iSCSI transport class.
The changes
Quoted 6 lines
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/*

Quoted 7 lines

Quoted 4 lines
/* 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.

Quoted 2 lines

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

Quoted 11 lines

Quoted 7 lines
/* 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.

Quoted 8 lines

Quoted 6 lines
/* 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.

Quoted 26 lines

Quoted 1 line
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

Quoted 4 lines

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

Mike Christie

author

2004-04-05 20:22 UTC #4 lore raw

Hey,

Quoted 23 lines · Surekha.PC

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.
Quoted 14 lines
Quoted 1 line
I have reviewed your changes for iSCSI transport class.
The changes
Quoted 9 lines
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
 |-- ....

Quoted 5 lines

Quoted 1 line
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.

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 4 lines

Quoted 1 line
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.

Quoted 30 lines

/sys/class/iscsi_transport/
|--host1/
       |-- shutdown_hba
       |-- show_hba_id
   |--host2/
       |-- ---"----
       |-- ---"----
Quoted 10 lines
Quoted 6 lines
/* 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.
Quoted 7 lines
Quoted 6 lines
/* lun specific attributes */
|-- 1:0:1:0/
            |-- device -> ../../../devices/platform/iscsi/1:0:0:0
            |-- driver -> ../../../bus/scsi/drivers/sd
            |-- lun_status
            |-- ....

Quoted 7 lines

Quoted 1 line
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.

Quoted 26 lines

Quoted 26 lines
Quoted 1 line
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
........

Quoted 6 lines

Quoted 1 line
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

Mike Christie

2004-04-05 21:39 UTC #5 lore raw

Quoted 39 lines · Mike Christie

Hey,
Quoted 27 lines · Surekha.PC
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.
Quoted 16 lines
Quoted 1 line
I have reviewed your changes for iSCSI transport class.
The changes
Quoted 9 lines
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
 |-- ....
Quoted 6 lines
Quoted 1 line
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.

Quoted 5 lines

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.

Let me clarify what I wrote. I am just saying becuase Cisco's software iscsi driver has some attribute which a fc or spi or whatever driver lacks does not necessarily mean it is transport related. Some of your attributes are HBA or device related, and some are a result of your implmentation, and some are iscsi related.

Quoted 61 lines

Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 38 lines
/sys/class/iscsi_transport/
|--host1/
       |-- shutdown_hba
       |-- show_hba_id
   |--host2/
       |-- ---"----
       |-- ---"----
Quoted 11 lines
Quoted 6 lines
/* 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.
Quoted 9 lines
Quoted 6 lines
/* 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.

Quoted 7 lines

Quoted 4 lines
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.

Just another clarification. I am not saying everying you are doing at the lun level is not iscsi specific.

Heiko Carstens

2004-04-06 08:51 UTC #6 lore raw

In reply to Surekha.PC

Hi,

Quoted 16 lines

Quoted 16 lines
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

Quoted 9 lines

/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

Looking at all the different transport classes recently introduced, I don't think this is the way to go. The sysfs layout for scsi devices gets too complicated in my opinion.

To give you another example for an ugly sysfs layout, caused by the lack of a generic solution, here is what we do within the zfcp lldd:

/sys/devices/css0/0.0.000e/0.0.50dc

|-- <zfcp lldd hba specific attributes>
|-- host0
|   |- <host_attributes>
|   |- 0:0:1:0
|   .  |- <lun attributes>
|   .
|
|-- <wwpn0> (this and stuff below created by zfcp lldd)
|   |-- <fc port attributes>
|   |-- <zfcp lldd specific fc port attributes>
|   |-- <fcp_lun0>
|   .    |-- <fcp lun attributes>
|   .    |-- <zfcp lldd specific fcp lun attributes>
|
|-- <wwpn1>

. .
. .

So with using the fc transport class we would end up with even another place where one has to look for things:

/sys/class/fc_transport/0:0:1:0

Now that there are transport classes available, the fc port and lun attributes should move there. But we still have quite a bunch of zfcp lldd specific fc port and lun attributes that don't fit into any other place. It would be nice to have a common place for all the different attributes, i.e. lldd attributes, midlayer attributes and transport attributes.

Quoted 1 line

From my point of view it would be better to go for a simple

hierarchical structure like this:

In addition I can think of getting rid of hostnumber, or is it really needed for anything?

Any comments/flames?

Heiko