One more change pushed to bk tree

Like last time, this has been through a full compile test, but not a boot test. I'm heading up to the office next so the boot test will come shortly. But, because it touches lots of files, again, I thought I would go ahead and push it up for people to see. If there are no objections to this or my previous patches, then once the boot test is complete and I'm satisfied things are working properly, I'll push the whole lot to Linus (which will necessarily include the scsi-misc-2.5 changes because they are in my tree as well).

bk pull bk://linux-scsi.bkbits.net/scsi-dledford

New Changeset added since last night:
[email protected], 2002-11-22 12:59:10-05:00, [email protected]
Convert from host->host_queue to host->my_devices list usage Add in usage of new same_target_siblings list
Add scsi_release_commandblocks() call to scsi_free_sdev()
Make all scsi device freeing use scsi_free_sdev()

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

Heh, now you managed to get stuff commited before me and I need to merge up :) But your changes help me a lot in fact so I surely won't complain.

Two comments:

o as you already had to change all callers of host->host_queue/ host->my_devices could you add a lock to properly protect it? That's pretty important for hotplugging/etc.. (like the fc hotplug patch from montavista)
o what's the reason in continuing after an error from scsi_add_host in scsi_add_host? IMHO it doesn't make much sense to continue the bus scan in that case.

Quoting from my email yesterday:

It's a race Charlie Brown!

;-)

I'm actually thinking that the locking of the devices is not needed as long as the next step is to A) make sure that all people walking the my_devices list already are holding a reference to the parent host vi host_get_*() and B) that proper locking for hosts is actually implemented in the host_get*() routines instead of the stub comments that are there now.

There's no guarantee that devices later on in the chain will fail to attach, so I saw little reason to leave things half done. If they all fail after the first failure, then we are no worse off then by bailing early, but if some actually succeed then we are better off.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

Yupp.

BTW, boot and run tests were successful. No I'm trying to isolate why certain modules blow up when you load them, unload them, then reload them. Right now, the aic7xxx_old, sd_mod, and scsi_mod modules are working. The st module is blowing chunks. I haven't tried sr_mod yet. I also haven't tried any other lldd modules yet.

Anyway, back to hacking st.c

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

st.c problem solved. At this point, these changes are ready for sucking up by Linus.

bk pull bk://linux-scsi.bkbits.net/scsi-dledford

when you have network connectivity again Linus ;-)

There are two more new Changesets at the top of the changeset list relative to my last email, plus the tree has been merged with 2.4.49 since my last email. The total local changeset list is attached.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

[email protected], 2002-11-22 18:26:02-05:00, [email protected]
fs/proc/inode.c: Make proc use new module loader semantics so that
touching a /proc/* file doesn't pin a module in memory

[email protected], 2002-11-22 18:25:05-05:00, [email protected]
st.c: Clean up init failure paths
Fix the detach code so it doesn't call sysfs unregister with
a lock held

[email protected], 2002-11-22 17:53:11-05:00, [email protected]
Merge bk://linux.bkbits.net/linux-2.5
into flossy.devel.redhat.com:/usr/local/home/dledford/bk/linus-2.5

[email protected], 2002-11-22 12:59:10-05:00, [email protected]
Convert from host->host_queue to host->my_devices list usage Add in usage of new same_target_siblings list
Add scsi_release_commandblocks() call to scsi_free_sdev()
Make all scsi device freeing use scsi_free_sdev()

[email protected], 2002-11-22 00:36:31-05:00, [email protected]
scsi: Update lldd API to slave_alloc/slave_configure/slave_destroy interface
instead of slave_attach/slave_detach interface. Change all users of existing interface to new interface, update scan code for new device attachement semantics

[email protected], 2002-11-21 11:59:45-06:00, [email protected]
[PATCH] cleanup code for /proc add/remove single device

Hi -

hardirq.h and init.h were removed from scsi.h. So I had to make the following changes to compile. I also removed two extraneous includes.

[email protected], 2002-11-21 12:55:58-05:00, [email protected]
Merge aladin.rdu.redhat.com:/usr/local/home/dledford/bk/linus-2.5 into aladin.rdu.redhat.com:/usr/src/2.5

[email protected], 2002-11-21 10:06:03-06:00, [email protected]
[PATCH] turn scsi_allocate_device into readable code

[email protected], 2002-11-21 10:05:24-06:00, [email protected]
[PATCH] some HBA compile warning fixes

[email protected], 2002-11-21 10:04:47-06:00, [email protected]
[PATCH] remove useless includes from scsi.h

[email protected], 2002-11-21 10:04:16-06:00, [email protected]
[PATCH] get rid of scan_scsis

this function was a multiplexer for two very different things, but with my last patch one of those faded away and only one callers is left. Simplify it to what's still needed and rename to scsi_scan_host.

[email protected], 2002-11-21 10:03:50-06:00, [email protected]
[PATCH] cleanup code for /proc add/remove single device

two new helpers in scsi_scan.c: scsi_add_single_device and scsi_remove_single_device that do all hard work. Thanks to beeing in scsi_scan.c and using proper helpers they're a lot smaller and cleaner.

[email protected], 2002-11-21 10:03:28-06:00, [email protected]
[PATCH] make a few more routines private to scsi_lib.c

[email protected], 2002-11-21 10:03:10-06:00, [email protected]
[PATCH] split busy check out of scsi_remove_host

just a simple code cleanup

[email protected], 2002-11-21 10:02:57-06:00, [email protected]
[PATCH] remove unused include in scsi_ioctl.c

This time I explicitly checked for the in_atomic mess..

[email protected], 2002-11-20 19:05:37-05:00, [email protected]
Merge aladin.rdu.redhat.com:/usr/local/home/dledford/bk/linus-2.5 into aladin.rdu.redhat.com:/usr/src/2.5

[email protected], 2002-11-20 19:05:09-05:00, [email protected]
scsi.c, scsi.h, scsi_syms.c, aic7xxx_old: add new function to track queue
full events at the mid layer instead of at the low level device driver

[email protected], 2002-11-19 17:07:50-05:00, [email protected]
Merge bk://linux.bkbits.net/linux-2.5
into flossy.devel.redhat.com:/usr/local/home/dledford/bk/linus-2.5

[email protected], 2002-11-17 22:09:07-05:00, [email protected]
aic7xxx_old: update biosparam function with the (ugly) detail that
cylinder values > 65535 get truncated

[email protected], 2002-11-17 18:57:50-05:00, [email protected]
aic7xxx_old: fix up the biosparam function to do 64bit math safely

Hi Doug -

It booted and runs fine on a netfinity + aic. I haven't updated my modutils yet, so did not try the qla driver (ported to your new naming scheme, I need the qla to test with a qlogics 2300 with a multi-lun target).

Was there a missing scsi_release_commandblocks call?

Why not remove the next/prev in scsi_device? Any code referencing them is broken, usage should at least generate a compilation warning.

The slave_destroy should be called before the channel/id/lun is modified during scanning, so slave_destroy if needed can index off of host/channel/id/lun and figure out properly what to destroy. Perhaps there should be a common function for this that calls slave_destroy, modifies sdevscan channel/id/lun, and then calls slave_alloc; such a function might also release and build command blocks.

The report lun usage does not modify the channel/id/lun, so does not need the slave_destroy/slave_alloc calls.

IMO the traversal of scsi_devices should be completely transparent to the user, such as a wrapper or call back function, so we would not have to change anything if the underlying data structure changed (the list of scsi_hosts contains a list of scsi_devices). Namely, for multi-path it is useful to have all the scsi_devices on one list, and not have a host centric view of the scsi_devices.

-- Patrick Mansfield

Yep (at least it looked like it, so I decided better safe than sorry since scsi_release_commandblocks() is safe even when there are none to release).

I was going to in my next patch. The first was to change it over, the next would have been various cleanups.

Hmmm...I thought that's how I did it....

Well, before we go too far down this road, I do want to pipe up and publically announce how much I thoroughly *DETEST* that whole section of code with its totally convoluted hoop jumping crap that it does to avoid calling blk_init_queue() on each scan target. Personally, if it were up to me I would make a much simpler/faster queue init routine, quit this crap of passing around and reusing the same pointer, and clean up *tons* of crap in the scsi scan code. That being said, anything that increases the complexity of that code is bad IMHO.

Furthermore, one of my intended near future patches, pending review of the idea because this is a change that would touch *A LOT* of files, is to get rid of scsi_{build,release}_commandblocks entirely and instead change the scsi subsystem to use generic command blocks that are applicable to all hosts, modify all low level drivers to use the cmd->device pointer to get the host/bus/target id/lun values so that they don't need device specific command blocks, then to allocate a small pool of these on startup and lazy allocate/deallocate them as we run and as scsi_adjust_queue_depth() starts getting called for various devices. If that happens, then most of the discussion about command blocks is moot. Then, since currently I'm the only user of slave_alloc(), it would also be easy enough for me to make a rule that no device driver is allowed to hard code device numbers in thier alloced struct so that we could do this scanning of devices, not need to rebuild command blocks, not need to realloc slave structs, and still have everything work. Good enough?

Umm...doesn't report lun scan get called on successive targets during a host scan using a recycled sdevscan?

I personally think it makes more sense to keep the host centric view of devices, and for multipath the most I would do is add another list struct for chaining together device structs that all point to the same device.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

scsi_adjust_queue_depth() would ideally just change an integer, say device->max_device_cmds or something like that (OSLT). This would play nicely with int dev->device_cmds_held or int device_cmds_active OSLT, also being an integer and it wouldn't matter if you _decrease_ the max, but currently there are more active in the device. (you may disallow this or set some policy if you wish, etc...)

As a spin off of this, if you set max_device_cmds to 0, you've effectively turned off the device, but letting it complete what's active -- one may not want to do this like that of course, but mentioned here just as a consequence.

The point of all this is that this makes a lot more things a lot more flexible and makes SCSI core shrink (long, long awaited).

BTW, you've mentioned this change many times before. I don't know why you haven't implemented it yet, or how structural (non-cosmetic) changes are voted for in linux-scsi for the SCSI layer, but you have my vote on this change.

Luben

Hi Doug -

I meant the ones done during scanning, like these back to back calls in scsi_probe_and_add_lun, where we call both functions with the new values of host/chan/id/lun:

	if (sdevscan->host->hostt->slave_destroy)
		sdevscan->host->hostt->slave_destroy(sdevscan);
	if (sdevscan->host->hostt->slave_alloc)
		sdevscan->host->hostt->slave_alloc(sdevscan);

The really ugly loop stuff of 2.4 is gone. We still must pass host/channel/id/lun, even if the sdevscan is removed, so we would still have to pass around a bunch of args or a pointer to a host/chan/id/lun.

The worst part about the queue setup is that we allocate our own request, as do the scsi character devices (st, osst and sg), never using the make_request, so we need no struct request's. So during the scan we are avoiding allocations/frees of data structures we do not even use.

We need a two piece blk queue setup - one to setup the queue, and one to allocate the number of struct request's needed. The latter can be called at upper level driver attach time (maybe even in scsi_attach_device, needs another argument or use an sdev field).

Sounds good, though lazy allocation (even like we have now) can have cases where we are trying to swap out, and suddenly load up the swap device, and the swap device must then allocate new command blocks while low on memory - IO will happen but could be slower than optimal.

I would like abstract interfaces so the device drivers don't have to be modified if the data structures ever change. i.e. something like:

scsi_get_cmd_host(scmd)
scsi_get_cmd_channel(scmd)
scsi_get_cmd_id(scdm)
scsi_get_cmd_lun(scmd)

If the above returned handles (or structs) the interface could be abstracted even further, making future changes easier to deal with (like changing the the lun size; you can grep for the names everywhere but that's not easy). The above helps me with the scsi multi-path (even if you don't agree with the approach).

Yes, but the host/chan/id/lun has is already setup for scanning lun 0. It does not hurt and is cleaner to do the same setup/teardown in all cases.

But no matter which way it goes, having an abastracted interface (callback or whatever) makes it possible to use a host or non-host centric list of scsi_devices.

The current scsi multi-path patch I have uses a list to chain together paths to one scsi_device, not a scsi_device per path. If we have a scsi_device per path, we have the overhead of duplicate block queues (and lack of sorting/merging across queues), duplicated data in a scsi_device, we have to figure out what scsi_device to use or expose to user land, and passing errors into the block layer is difficult (generally, externalizing the code/data contained in scsi_decide_disposition).

-- Patrick Mansfield