Christoph,
I'll seperate the patches and fix the things you mention. I'm still not
entirely pleased with the locking and Patrick Mansfield has suggested a
better method for traversing the host_queue structure that I may consider.
Regarding WWN translation in userspace, there are several reasons why
this apporach is inferior:
1. When a device is "inserted" into the system and isn't present, there
is no way to map WWNs to target ids. Processing in userspace would work
for removals, but what about insertions? Sure the driver could export
its list of known WWNs (the firmware maintains a list of active WWNs on
the bus and their associated scsi target, even if the SCSI subsystem
doesn't know the device is there), but read #2 below:
2. There is no simple way to know what WWN maps to which SCSI ID from
user space.
Typical FibreChannel drivers maintain this information, but it isn't
exported to user space in any useable way. I suppose a list of
device->WWN maps could be present in /proc or driverfs but that adds
alot of string processing from user space and requires a standard format
between drivers to be generically usable. Further, what if two drivers
want to keep track of their device to WWN map? They must each register
with some function that exports the list to userspace. Why go through
all the trouble when the driver can just do the work?
3. Hotswap operations should be fast (10 msec or so) and userspace
processing is slow.
Compare two operations one using the defined hotswap mapping interface
and one processing a file in proc
processing in userspace:
while (not_eol) {
read syscall
convert string WWNS to 64 bit number
compare WWNS
if matched, convert string scsi target information (host, channel,
target, lun) into 32 bit numbers
issue hotswap command using host channel target lun information
}
processing in kernel:
call wwn to scsi target converter in driver
in driver, each WWN is compared against wwn in WWN list
if match found, scsi target information returned
Now, assume the above with 32 or more devices. That is alot of syscalls
using userspace methodology (and slow). Let the kernel do the
conversion directly. It has direct access to the data. Another choice
would be to put a 64 bit field for wwn's into the scsi device structure.
Then the scsi device list could be traveresed to find the correct
target to hotswap. Would this be preferrrable to the interface
currently implemented?
4. Having to map WWNs to scsi IDs in userspace is harder to use
its alot easier to remove a device to echo "wwn" >
/sys/bus/scsi/hotswap_comamnds/remove_by_fc_wwn_wildcard
compare that with writing an application to process /proc entries (or
something similiar).
other comments below:
Christoph Hellwig wrote:
>Umm, stop.
>
>Scsi midlayer patches don't go directly to Linus, but through the linux-scsi
>list and James into the linux-scsi bk repository first.
>
>The patch still has a bunch of problem not solved, and contains two things
>that should be independant patches.
>
>The first patch should be the host_queue locking you added, this one currently
>has the following issues:
>
>* you call spin_lock on a semaphore once!
>
>
sloppy I'll fix.
>* you take semaphores inside spinlocks and with interrupts disabled
>
good point I think I need to look at this patch more.
>* the coding style needs some imnprovements (you adds lots of empty lines,
> and there's a space before the opening brakes of function calls).
>
>the actual driver still has other issues:
>
>* you still duplicated lots of code from scsi.c
>
I'll remove the code from scsi.c and have it call the hotswap interfaces
directly.
>* your header is still in include/linux instead of include/scsi,
> but imho it should be merged into scsi.h anyway
>
I'll merge into scsi.h.
>* you still havent explain why wwn -> host id translation can't
> be done in userspace
>
see above
>* you still have useage information in the driverfs files.
>
>
>
i'll remove.
Thanks for the great comments.
-steve
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Thu Oct 31 2002 - 22:00:49 EST