Re: Linux Virtual SCSI HBAs and Virtual disks

Stefan Richter
2007-01-21T11:24:52Z

Aboo Valappil wrote:
> I actually uses the "openers" field in scsi_disk to find out if anyone
> has the scsi_device open or not.

There are several issues with this approach.
- It will fail eventually because some day there may be other users of
a LU than sd. How would sg, sr, st be accommodated?
- The type struct scsi_disk is defined locally in sd.c (not somewhere
in linux/include/scsi/) and you have to copy the struct definition in
your hba.c. That's because struct scsi_disk is not part of any in-kernel
API and you shouldn't use it in an LLD. If you really need to extend the
LLD API, then do it explicitly by patching the SCSI core and its
linux/include/scsi/ files, and do it as cleanly as possible.
- You copied the comment "protected by BKL for now, yuck" on struct
scsi_disk.openers, but you forgot to access openers under actual
protection by BKL. I bet though that there are several more concurrency
issues when poking in dev_get_drvdata(&sdev->sdev_gendev).

(Is it actually still true that the BKL is taken when device files are
opened and closed?)

BTW, the comment on shost_for_each_device() in
linux/include/scsi/scsi_device.h says "This loop takes a reference on
each device and releases it at the end. If you break out of the loop,
you must call scsi_device_put(sdev)." You forgot to do so.

> Aboo Valappil wrote:
>> I tried the sdparms and it failed not due to lack of sense code and
>> status. What happened was that the user space SCSI target died due
>> to a unsupported SCSI command (bug in user space target). When it
>> crashed, the SCSI disk served by that user space target was opened
>> by sdparms. The driver removed the scsi_host which was attached to
>> user space target, thinking that the last registered user space part
>> died.

When the userspace server vanished, it is as if hot-pluggable hardware
was removed. Your queuecommand hook, and probably the eh hooks and the
shutdown paths too, should be aware of such hot removals and act
accordingly. I haven't checked your code in detail but it seems you
already take some precautions. More may be necessary or at least
convenient, e.g. dequeuing and finishing all outstanding commands when a
hot removal was detected.

I can tell you that it is not exactly trivial to make Linux SCSI LLDs
handle hot removal correctly. You probably should look at some other
LLDs which have to deal with hot removal but (a) I don't guarantee you
find elegant solutions and (b) each type of transport or interconnect
has its own special requirements.

On the bright side, if you get the hot removal handling right, you may
be able to *completely avoid LLD API extensions* of the kind discussed
above.

Another more general note: You mentioned earlier that you suggest
vscsihba for inclusion into mainline. Read the following texts in
linux/Documentation: CodingStyle, SubmittingDrivers, SubmitChecklist.
BTW, you could also write a minimalist version of the userspace
counterpart to vscsihba and submit it as a file in
linux/Documentation/scsi/ as a programming example along with the patch
which adds vscsihba.
--
Stefan Richter
-=====-=-=== ---= =-=-=