Merge I2O patches from -mm

DISCUSSION Warren Togami 5 participants 12 messages Aug 15, 2004 – Aug 17, 2004 SCSI

Share & tools

Warren Togami

author

2004-08-15 10:15 UTC #0 lore raw

This is a request to please merge the I2O patches currently in Andrew Morton's -mm tree into the mainline kernel. They resolve all known reported issues with I2O RAID devices. If they can be included soon, it would be possible to implement and test direct installation before FC3 Test2 freeze.

Also because Markus would never ask himself, I nominate Markus Lidel as the "maintainer" of the 2.6 generic I2O layer. He has put a tremendous amount of work into improving an otherwise neglected part of the kernel.
Thanks to his efforts it is today usable and stable on multiple archs
and all known supported cards.

Thank you,
Warren Togami
[email protected]

Andrew Morton

2004-08-17 02:17 UTC #1 lore raw

Quoted 6 lines · Warren Togami

This is a request to please merge the I2O patches currently in Andrew Morton's -mm tree into the mainline kernel. They resolve all known reported issues with I2O RAID devices. If they can be included soon, it would be possible to implement and test direct installation before FC3 Test2 freeze.

We'll get it in when Linus returns.

Quoted 3 lines

Also because Markus would never ask himself, I nominate Markus Lidel as the "maintainer" of the 2.6 generic I2O layer. He has put a tremendous amount of work into improving an otherwise neglected part of the kernel.

No problem with that, but we wouldn't want to go adding Markus to ./MAINTAINERS without his approval.

Quoted 2 lines

Thanks to his efforts it is today usable and stable on multiple archs
and all known supported cards.

yup.

Markus Lidel

2004-08-17 08:25 UTC #2 lore raw

Hi...

Quoted 3 lines · Andrew Morton

Quoted 1 line · Warren Togami
This is a request to please merge the I2O patches currently in Andrew Morton's -mm tree into the mainline kernel. They resolve all known reported issues with I2O RAID devices. If they can be included soon, it would be possible to implement and test direct installation before FC3 Test2 freeze.
We'll get it in when Linus returns.

Wowww, great news \:D/

Quoted 3 lines

Quoted 1 line
Also because Markus would never ask himself, I nominate Markus Lidel as the "maintainer" of the 2.6 generic I2O layer. He has put a tremendous amount of work into improving an otherwise neglected part of the kernel.
No problem with that, but we wouldn't want to go adding Markus to ./MAINTAINERS without his approval.

It would be a great honor for me to be the maintainer of the I2O subsystem.

Best regads,

Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: [email protected]
URL:    http://www.shadowconnect.com

Christoph Hellwig

2004-08-17 11:53 UTC #3 lore raw

In reply to Warren Togami

Quoted 5 lines · Warren Togami

This is a request to please merge the I2O patches currently in Andrew Morton's -mm tree into the mainline kernel. They resolve all known reported issues with I2O RAID devices. If they can be included soon, it would be possible to implement and test direct installation before FC3 Test2 freeze.

I've looked over it and except for the i2o_scsi driver it looks sane.

Cosmetic fixups I'd like to see done befoee merging to mainline:

- run the code through Lindent
- stop the needless file renaming. Splitting up i2o_core.c into multiple files is fine, but please don' rename the other drivers for the sake of it

Now to i2o_scsi:

- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.
- the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut fortunately with the suggestion above it would just go away
- the global list of hosts and wlaking it on exit is a very bad design, that's something the ->remove callback should do on per-device basis
- the completely lack of SCSI EH in this driver scares me, does the firmware really handle all EH?

cosemtic stuff in here:

- <asm/*.h> after <linux/*.h>.
- please include scsi headers using <scsi/*.h> (after linux and asm headers)
- please use the standard pr_Debug instead of DBG
- please reorder the functions a little so you don't need forward-declarations

Markus Lidel

2004-08-17 13:18 UTC #4 lore raw

Hi...

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Warren Togami
This is a request to please merge the I2O patches currently in Andrew Morton's -mm tree into the mainline kernel. They resolve all known reported issues with I2O RAID devices. If they can be included soon, it would be possible to implement and test direct installation before FC3 Test2 freeze.
I've looked over it and except for the i2o_scsi driver it looks sane. Cosmetic fixups I'd like to see done befoee merging to mainline:
- run the code through Lindent

Okay, will do it...

Quoted 3 lines

- stop the needless file renaming. Splitting up i2o_core.c into multiple files is fine, but please don' rename the other drivers for the sake of it

just wanted to be consistent with the other files, but it shouldn't be a problem to rename them to the original name...

Quoted 3 lines

Now to i2o_scsi:
- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.

But wouldn't it be a waste of resources to allocate a Scsi_Host structure for every I2O device? Note that the i2o_scsi "sees" all disks even if they are in a RAID array, so in most cases there are at least 3 Scsi_Host adapters...

We also now know which disk is on which controller, this information is lost with your approach...

Quoted 2 lines

- the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut fortunately with the suggestion above it would just go away

Yep, i know that it would be better to extend scsi_add_device, so it's possible to pass a pointer to i2o_scsi_slave_alloc. This is only a workaround, which breaks as soon as things are done in parallel :-(

Quoted 2 lines

- the global list of hosts and wlaking it on exit is a very bad design, that's something the ->remove callback should do on per-device basis

But what if the I2O device isn't removed?

Quoted 2 lines

- the completely lack of SCSI EH in this driver scares me, does the firmware really handle all EH?

The i2o_scsi driver is not used to access the disks, it is only for monitoring. The i2o_block driver handles disk access. So if you reset the SCSI channel in the i2o_scsi driver, commands which are transfered by the i2o_block driver will be aborted (this is the reason, why the I2O subsystem didn't work for users, which compiled in i2o_scsi and i2o_block into the kernel)...

Quoted 4 lines

cosemtic stuff in here:
- <asm/*.h> after <linux/*.h>.
- please include scsi headers using <scsi/*.h> (after linux and asm headers)
- please use the standard pr_Debug instead of DBG

This is what i searched for the whole time :-) Thanks for the hint!

Quoted 1 line

- please reorder the functions a little so you don't need forward-declarations

most of the forward-declarations are not needed at all, should i remove unneeded completely?

Thanks for taking time to review my changes!

Best regards,

Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: [email protected]
URL:    http://www.shadowconnect.com

Alan Cox

2004-08-17 15:03 UTC #5 lore raw

Quoted 8 lines · Markus Lidel

Quoted 3 lines
Now to i2o_scsi:
- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.
But wouldn't it be a waste of resources to allocate a Scsi_Host structure for every I2O device? Note that the i2o_scsi "sees" all disks even if they are in a RAID array, so in most cases there are at least 3 Scsi_Host adapters...

Christoph the "I2O" device is a communication processor. You need to preserve the real scsi busses in order to get sane results from scsi tools. If EH is implemented you'll need this to do controlled resets (although this gets quite umm 'interesting' if using i2o_block also)

You've got

I2O comms processor
Multiple scsi busses
Multiple drives
Block driver providing abstract volumes

Sometimes (as with other raid) bypassing the block driver is faster.

Quoted 5 lines

Quoted 2 lines
- the completely lack of SCSI EH in this driver scares me, does the firmware really handle all EH?
The i2o_scsi driver is not used to access the disks, it is only for monitoring. The i2o_block driver handles disk access. So if you reset

(not always - the FC920 is way faster using i2o_scsi for the disk I/O)

Quoted 4 lines

the SCSI channel in the i2o_scsi driver, commands which are transfered by the i2o_block driver will be aborted (this is the reason, why the I2O subsystem didn't work for users, which compiled in i2o_scsi and i2o_block into the kernel)...

All error timeout handling is done by the controller. It does really need minimal EH handlers simply to reissue failed timed out commands.

Christoph Hellwig

2004-08-17 15:06 UTC #6 lore raw

Quoted 10 lines · Alan Cox

Quoted 8 lines · Markus Lidel
Quoted 3 lines
Now to i2o_scsi:
- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.
But wouldn't it be a waste of resources to allocate a Scsi_Host structure for every I2O device? Note that the i2o_scsi "sees" all disks even if they are in a RAID array, so in most cases there are at least 3 Scsi_Host adapters...

Quoted 4 lines

Christoph the "I2O" device is a communication processor. You need to preserve the real scsi busses in order to get sane results from scsi tools. If EH is implemented you'll need this to do controlled resets (although this gets quite umm 'interesting' if using i2o_block also)

Okay, then we'll have to rething the data structures a little more. I was under the impression the scsi busses were completely faked for the OS.

Christoph Hellwig

2004-08-17 15:37 UTC #8 lore raw

In reply to Markus Lidel

Quoted 8 lines · Markus Lidel

Quoted 3 lines
Now to i2o_scsi:
- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.
But wouldn't it be a waste of resources to allocate a Scsi_Host structure for every I2O device? Note that the i2o_scsi "sees" all disks even if they are in a RAID array, so in most cases there are at least 3 Scsi_Host adapters...

I wouldn't wasted ressources but it seems Alan found another problem.

Quoted 2 lines

We also now know which disk is on which controller, this information is lost with your approach...

You could still keep that information in your data structure. But what do you actually need it for?

Quoted 6 lines

Quoted 2 lines
- the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut fortunately with the suggestion above it would just go away
Yep, i know that it would be better to extend scsi_add_device, so it's possible to pass a pointer to i2o_scsi_slave_alloc. This is only a workaround, which breaks as soon as things are done in parallel :-(

Just keep some lookup data structure so you can find the device data by host/target/lun numbers.

Quoted 4 lines

Quoted 2 lines
- the global list of hosts and wlaking it on exit is a very bad design, that's something the ->remove callback should do on per-device basis
But what if the I2O device isn't removed?

you're using the driver model, and that calls ->remove and every device when the driver is unregistered.

Quoted 4 lines

Quoted 1 line
- please reorder the functions a little so you don't need forward-declarations
most of the forward-declarations are not needed at all, should i remove unneeded completely?

Yes, please.

Okay, some brainstorming to get the data structures and lookup right:

What really seems to miss in your model is a callback to i2o_scsi from the main i2o code when a new i2o_controller is found, if you implemented that we'd allocate/deallocate the Scsi_Host in that callback and ->probe/->remove could be sure it'd always have it.

Anyway, I think we could live without that.

i2o_scsi_get_host would get inlined into i2o_scsi_probe. i2o_scsi_remove basically needs a full rewrite, you need to find a way to store a scsi_device ini i2o_dev and it becomes completely trivial.

Not sure about how to sanitize the scsi_devie probing logic in i2o_scsi_probe yet.

Alan Cox

2004-08-17 15:56 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

Okay, then we'll have to rething the data structures a little more. I was under the impression the scsi busses were completely faked for the OS.

Nope. I can send you the i2o 1.5 docs if you would like but they are fairly heavy reading, and a testimony to a neat hardware design and concept destroyed by committee.

Markus Lidel

2004-08-17 16:55 UTC #9 lore raw

In reply to Christoph Hellwig

Hello,

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · Markus Lidel
Quoted 3 lines
Now to i2o_scsi:
- the logic of "demand-allocating" Scsi_Hosts looks rather bad to me, life would be much simpler with a Scsi_Host per i2o device.
But wouldn't it be a waste of resources to allocate a Scsi_Host structure for every I2O device? Note that the i2o_scsi "sees" all disks even if they are in a RAID array, so in most cases there are at least 3 Scsi_Host adapters...
I wouldn't wasted ressources but it seems Alan found another problem.

What problem do you mean?

Quoted 3 lines

Quoted 1 line
We also now know which disk is on which controller, this information is lost with your approach...
You could still keep that information in your data structure. But what do you actually need it for?

Okay, it's not that it is crucial, but why should we throw this information over board? What will we gain, if we throw it over board, and use a single Scsi_Host for each I2O device?

Quoted 5 lines

Quoted 3 lines
Quoted 2 lines
- the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut fortunately with the suggestion above it would just go away
Yep, i know that it would be better to extend scsi_add_device, so it's possible to pass a pointer to i2o_scsi_slave_alloc. This is only a workaround, which breaks as soon as things are done in parallel :-(
Just keep some lookup data structure so you can find the device data by host/target/lun numbers.

We already had this, but we don't need it at all... This structure would take unnecessary resources, and what is the benefit? We don't need the mapping table after initialization anymore. IMHO it's better to add a parameter to scsi_add_device, which will be passed to scsi_slave_alloc afterwords, or do it the same way like the Scsi_Host structure...

Quoted 5 lines

Quoted 3 lines
Quoted 2 lines
- the global list of hosts and wlaking it on exit is a very bad design, that's something the ->remove callback should do on per-device basis
But what if the I2O device isn't removed?
you're using the driver model, and that calls ->remove and every device when the driver is unregistered.

Okay, i don't remember why i added this, but i will try to get rid of it
if it is possible (i agree with you, that it is ugly)...

Quoted 6 lines

Okay, some brainstorming to get the data structures and lookup right:
What really seems to miss in your model is a callback to i2o_scsi from the main i2o code when a new i2o_controller is found, if you implemented that we'd allocate/deallocate the Scsi_Host in that callback and ->probe/->remove could be sure it'd always have it.

Okay, sounds very good to me! This also solves the problem, when a new controller is added at runtime :-)

I'll look at the PCI driver to do it the same way...

Quoted 4 lines

Anyway, I think we could live without that. i2o_scsi_get_host would get inlined into i2o_scsi_probe. i2o_scsi_remove basically needs a full rewrite, you need to find a way to store a scsi_device ini i2o_dev and it becomes completely trivial.

Okay, will look at it...

Thanks again for your help!

Best regards,

Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: [email protected]
URL:    http://www.shadowconnect.com

Christoph Hellwig

2004-08-17 16:57 UTC #10 lore raw

In reply to Markus Lidel

The patch below adds a __scsi_add_device that can "preload" sdev->hostdata.

drivers/scsi/scsi_scan.c +14 ~~-11~~

@@ -200,7 +200,7 @@
  *     scsi_Device pointer, or NULL on failure.
  **/
 static struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *shost,
-	       	uint channel, uint id, uint lun)
+	       	uint channel, uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev, *device;
 	unsigned long flags;

@@ -224,6 +224,8 @@
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	spin_lock_init(&sdev->list_lock);
 
+	/* usually NULL and set by ->slave_alloc instead */
+	sdev->hostdata = hostdata;
 
 	/* if the device needs this changing, it may do so in the
 	 * slave_configure function */

@@ -697,7 +699,7 @@
  **/
 static int scsi_probe_and_add_lun(struct Scsi_Host *host,
 		uint channel, uint id, uint lun, int *bflagsp,
-		struct scsi_device **sdevp, int rescan)
+		struct scsi_device **sdevp, int rescan, void *hostdata)
 {
 	struct scsi_device *sdev;
 	struct scsi_request *sreq;

@@ -726,7 +728,7 @@
 		}
 	}
 
-	sdev = scsi_alloc_sdev(host, channel, id, lun);
+	sdev = scsi_alloc_sdev(host, channel, id, lun, hostdata);
 	if (!sdev)
 		goto out;
 	sreq = scsi_allocate_request(sdev, GFP_ATOMIC);

@@ -874,7 +876,7 @@
 	 */
 	for (lun = 1; lun < max_dev_lun; ++lun)
 		if ((scsi_probe_and_add_lun(shost, channel, id, lun,
-		      NULL, NULL, rescan) != SCSI_SCAN_LUN_PRESENT) &&
+		      NULL, NULL, rescan, NULL) != SCSI_SCAN_LUN_PRESENT) &&
 		    !sparse_lun)
 			return;
 }

@@ -1085,7 +1087,7 @@
 			int res;
 
 			res = scsi_probe_and_add_lun(sdev->host, sdev->channel,
-				sdev->id, lun, NULL, NULL, rescan);
+				sdev->id, lun, NULL, NULL, rescan, NULL);
 			if (res == SCSI_SCAN_NO_RESPONSE) {
 				/*
 				 * Got some results, but now none, abort.

@@ -1111,14 +1113,15 @@
 	return 0;
 }
 
-struct scsi_device *scsi_add_device(struct Scsi_Host *shost,
-				    uint channel, uint id, uint lun)
+struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
+		uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
 	int res;
 
 	down(&shost->scan_mutex);
-	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL, &sdev, 1);
+	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
+				     &sdev, 1, hostdata);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);

@@ -1178,7 +1181,7 @@
 		 * Scan for a specific host/chan/id/lun.
 		 */
 		scsi_probe_and_add_lun(shost, channel, id, lun, NULL, NULL,
-				       rescan);
+				       rescan, NULL);
 		return;
 	}

@@ -1187,7 +1190,7 @@
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
 	res = scsi_probe_and_add_lun(shost, channel, id, 0, &bflags, &sdev,
-				     rescan);
+				     rescan, NULL);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*

@@ -1316,7 +1319,7 @@
 {
 	struct scsi_device *sdev;
 
-	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0);
+	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0, NULL);
 	if (sdev) {
 		sdev->borken = 0;
 	}

===== drivers/scsi/scsi_syms.c 1.49 vs edited =====

drivers/scsi/scsi_syms.c +1 -1

@@ -73,7 +73,7 @@
 
 EXPORT_SYMBOL(scsi_io_completion);
 
-EXPORT_SYMBOL(scsi_add_device);
+EXPORT_SYMBOL(__scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);

include/scsi/scsi_device.h +4 -2

@@ -129,8 +129,10 @@
 #define transport_class_to_sdev(class_dev) \
 	container_of(class_dev, struct scsi_device, transport_classdev)
 
-extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
-		uint, uint, uint);
+extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
+		uint, uint, uint, void *hostdata);
+#define scsi_add_device(host, channel, target, lun) \
+	__scsi_add_device(host, channel, target, lun, NULL)
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_device_cancel(struct scsi_device *, int);

Markus Lidel

2004-08-17 18:24 UTC #11 lore raw

Hello,

Quoted 1 line · Christoph Hellwig

The patch below adds a __scsi_add_device that can "preload" sdev->hostdata.

Thank you very much for changing it!

At the moment i only have a patch without the i2o_scsi changes, but that will follow ASAP...

Best regards,

Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: [email protected]
URL:    http://www.shadowconnect.com