Request for review of Linux iSCSI driver version 4.0.0.1

Hi,

	Linux iSCSI driver 4.0.0.1 (linux-iscsi-4.0.0.1.tgz) is available at

http://sf.net/projects/linux-iscsi/

Could you please review the same and let us know whether it is OK for
inclusion into the linux 2-6 kernel?

The following are the changes w.r.t version 3.4 of our driver which we had earlier sent for review:
1)Compatible with linux -2.6.0 kernel.
2)Review comments from linux-scsi have been incorporated:
- Indentation /Coding style has been taken care of.
- Device probing and /dev/iscsi links creation have been moved from kernel to user mode.
- linked list macros from list.h is being used now. -few of our kernel files were including <endian.h>. This has been
rectified.

Thanx
N.C.Krishna Murthy

Here's a list of a few comments I've come up with. I haven't yet looked at the data structurws much, but it seems there's a complete lack of refcounting. Also there's lots of comments were ysou complain about scsi midlayer misbehaviour or bugs - please submit patches or submit workarounds when you encounter them, that's what open source is all about!

Also the same question that went to the unh folks goes to you: why do you think your implementation is better than theirs and why do you think cooperation on one iscsi stack is impossible.

here's the list:

highlevel:
- kill md5.c and iscsi-crc.c and use common cryptoapi md5 / common crc code instead.
- please kill you thousands of atoi/aton reimplementations and look for proper linux functions like simple_strtoul.

README:
- very outdated with 2.4/vendor issues

Makefile:
- lots of unused kernel detection stuff

iscis-kernel.h:
- only backward-compat cruft, should go away.

iscsi.c:
- doesn't need at least smp_lock.h - please audit header usage.
- kill stupid MODULE/MODULE_LICENSE ifdefs.
- kill = 0 / = NULL initializers for static variables, not needed.
- kill LINK_DIR/session->target_link_dir - kernel must not know about device pathes.
- kill max_*_devices - not needed in kernelspace (and wrong in 2.6)
- fix up strange indentation artefacts from indent, like:

static
struct iscsi_trace_entry

    trace_table[ISCSI_TRACE_COUNT];

should be:

static struct iscsi_trace_entry trace_table[ISCSI_TRACE_COUNT];

- first argument to memset is void * - don't cast pointers to char *
- dito for memcpy
- lose stupid braces, e.g.

list_del_init(&(task->task_group_link));

should be just

list_del_init(&task->task_group_link);

- don't use <hosts.h> but <scsi/scsi_host.h>, similarly use <scsi/scsi*.h> instead of scsi.h if possible. There's still some stuff not moved away from scsi.h yet, but you should at least avoid anything explicitly marked deprecated in that file and not in <scsi/scsi_*.h>
- no need to implement ->slave_alloc and ->slave_destroy if they are noops anyway.
- kill that slab name randomization - if kmem_cache_destroy fails you module is buggy and needs to be fixed.
- instead of the single linked list of commands use a lists.h list in the scsi_pointer
- host and hba can't be NULL in ->queuecommand per defintion, similarly id, lun cdbsize and use_sg can't be bigger than you set in the template.
- iscsi_info is far too noisy. I'd suggest killing it and just setting the name field in the template propely
- the procfs code is a mess. Please move it over to proper per-device / per-host sysfs attributes as procfs support in HBA drivers is deprecated.
- kill ctl_open/ctl_release and move add an owner to the fops structure for refcounting.
- the ifdef MODULE around iscsi_init/iscsi_cleanup is bogus
- lots of ifdef mess - iscsi_kernel.h still has lots of old kernel compat stuff - kill it.
- target_reset_occured() is a dup of scsi_report_device_reset()
- initialization is a total mess. You try to emulated the pre-2.6 init ordering. Use something like:

shost = scsi_host_alloc();
<content of ->detect>
scsi_add_host()

instead, and read the comment abote .legacy_hosts in the scsi_host_template defintion! Similar inline the code
in iscsi_release into the exitfunc.
- iscsi.c is _far_ too big. Needs to be split into a few files. I'd suggest avoiding the name iscsi.c so you can
build into iscsi.ko
- the ioctl API looks very broken, I need more time to
audit it. As a start slit ctl_ioctl into a function per ioctl subcommand.
- why can't you store a pointer to the session in host-private data of the scsi_device so you don't have to search in each ->queuecommand?
- kill the is_root_disk special casing. drivers can't know what the root disks is, there might not even be a single one.
- compilation fails for me in line 480, looks like this is because the left-over ifdefs are wrong - just use set_user_nice unconditionally. Or even better stop messing with priorities, kill iscsi_daemonize and use daemonize() directly.
- having multiple kmap() in the same process at the same time can kmap(), you redesign iscsi_xmit_task/iscsi_xmit_data/iscsi_recv_data not to do that. for the tx path use ->sendpage to avoid a data copy and kmapping altogether.
- the explanation of PREVENT_DATA_CORRUPTION doesn't make sense.
- why do you have SLAB_NO_REAP optional? Either it works or it doesn't..
- please stop that preallocation, you destroy the effect of per-cpu caches with that.
- you're only allocating one hba struct but still do complex list walking to find. Either move to multiple hbas (and proper standard lists) or kill all that overhead.
- INVALID_ORDERING_ASSUMPTIONS should probably a rutime option (?)
- what the heck is iscsi_set_if_addr? You have absolutely no business messing with network device configuration.
- device_flags() is strange. Better put a separately allocated struct into the scsi_device (allocated in slave_alloc, freed in slave_destroy) that only contains the bitmask for now. (and maybe the session, see above)
- similar for command flags - put your own overlay over the scsi_pointer, including a bitmask.
- kfree(NULL) is fine, don't check for variables beeing non-NULL before freeing them.

memset(session, 0, sizeof (*session));
kfree(session);

is rather bad for the cache.

- this comment indicates you don't want normal EH:

* check condition with no sense. We need to avoid this,
* since the Linux SCSI code could put the command in
* SCSI_STATE_FAILED, which it's error recovery doesn't appear
* to handle correctly, and even if it does, we're trying to
* bypass all of the Linux error recovery code
* to avoid blocking all I/O to the HBA. Fake some sense
* data that gets a retry from Linux.

so don't use scsi_error.c and define your own eh_strategy_handler method.

- please explain on this:

* Philsophically we ought to complete the command and let the
* midlayer or high-level driver deal with retries. Since the
* way the midlayer does retries is undesirable, we instead
* keep the command in the driver, but requeue it for the same
* cases the midlayer checks for retries. This lets us ignore
* the command's retry count, and do retries until the command
* timer expires.

why can't you simply mess with the retry count instead of duplicating all the retry logic. really, the midlayer queues are there for a reason.

- this doesn't seem to be an issue for a 2.6-only driver:

* not to log? In 2.5 we can put a pointer in
* Scsi_Device->hostdata, but 2.4 doesn't
...

- your probably want to rewrite your TCQ code to use the block layer tcq code, see include/linux/scsi_tcq.h

- there's lots of if (sc->device) checks where sc is a scsi_cmnd - but the device field never can be NULL for those, that's guaranteed by scsi_get_command. Indeed you have far too many checks for impossible conditions. Just use BUG_ON for those if you don't trust the midlayer, but trying to handle those conditions is just insane.

- replace iscsi_inet_aton with in_aton?

* FIXME: it'd probably be cleaner to move the timeout logic to the rx thread.

 *         The only danger is if the rx thread somehow blocks indefinately.
 *         Doing timeouts here makes sure the timeouts get checked, at the
 *         cost of having this code constantly loop.

- yes, you really need to reduce the amount of threads in the driver..
- shost->can_queue is not supposed to be changed once the host is online, as it's not locked down.
- please produce a simple testcase for the bug mentioned above iscsi_eh_device_reset.
- the bug mentioned above iscsi_eh_bus_reset should be long fixed. if not please report to linux-scsi.
- is the iscsi fake geometry documented somewhere? If not why don't you simply use the default, CAM one?

iscsi-probe.c:
- lots of unchecked kmalloc()s
- you can't allocate scsi_cmnds yourself but _must_ use scsi_get_command
- dito for scsi_device.
- the whole iscsi_do_cmnd use looks very very strange. you're probably much better of to use the scsi_request based interfaces and use midlayer queueing and sometimes even higher level interfaces, e.g. your iscsi_send_tur coudld be replaced by an scsi_ioctl(sdp, SCSI_IOCTL_TEST_UNIT_READY, NULL) I still don't understand, though why the heck you need to do a TUR in a lowlevel driver and even more need a thread do a single TUR, started from another kernel tread. there more I dig into it your probing and thread abuse looks horribly screwed.
- reinitilialize disk is another such candidate. again why do you need to send START_STOP commands from the LLDD, why do you need another thread instead of a state machine and again life would be a lot simpler by using the midlayer queueing infrastructure.
- dito for the reports luns and inquiry code in iscsi_detect_luns. I'm very very unhappy to see a lowlevel driver mess with all this. Even if you need to do it from a LLDD use the midlayer functionality for it. But I still want to see a very good explanation why you need this at all.

iscsi-kernel-event.c:
- sleep_on is racy.
- the concept of this is screwed. (See below)

iscsi-event-handler.c:
- the kernel-event mechanism is totally bogus. you're not going to wait in kernelspace for an userspace upcall doing procfs stuff. As I already said procfs might not even be mounted. use scsi_add_device/scsi_remove_device and kill that stupid symlinking crap. Really, if you driver has to deal with pathnames something is horribly wrong. If you really want devices to show up in /dev/iscsi/ submit patches to udev.

iscsi.h:
- Linux now has (lowecase) versions of DRIVER_BYTE & co.
- Linux now has unshifted status codes

Hi,

I have interspersed my answers below for 2 of your comments.

This flag is needed for our iSCSI network boot implementation to check if we are booted on iSCSI disk. This is used while trying to shutdown the iSCSI service.

business

This is again needed for network boot. The n/w driver and iSCSI driver are loaded during early boot. Since the n/w interface is not setup at that time, we need to bring up the interface through this call in iSCSI driver.

Thanks,
surekha

What if the root device changes (pivot_root syscall, namespaces)? You need to find a way to do what you want without having knowledge about beeing a root device in the LLDD - there might not be a single root device and it could change all the time.

That's bogus. Look at the ip autoconfig code used by rootnfs and reuse that instead of creating such bogus hacks. A driver has no business dealing with the network configuration.

Hi Surekha,

Your driver does not even need to mess with that code. Just modify your usersapce tools in the linux-iscsi package that run in the initrd to do this in userspace.

Mike

On Wed, 29 Oct 2003, Mike Christie outgrape:

Mike,

This may turn out to be a discussion about where this code resides and what code is best to (re)use, not if it is needed. "Early boot" here is from PXE -- setting up the iSCSI initiator to reach a target at some IP where the rest of the boot (initrd and friends) is.

I have not yet looked at the ip autoconfig code that Chris mentions above, though. Can't comment on that.

Clay Haapala ([email protected]) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
      "They're selling independence, Actors in the White House,
       Acid indigestion, Mortage on my life."
         -- from "Ramble Tamble" by CCR 

 "Vote for me if you want to live" -- the Gubernator

Oops. I forgot you guys use iNBP.com. I was thinking of pxelinux.

Mike

On Wed, 29 Oct 2003, Mike Christie outgrape:

Oh, now I have to find out about pxelinux ... google ... OK. :-)

Yeah, like that only different -- using iSCSI to a target to fetch blocks rather than TFTP of files from a filestore.

Clay Haapala ([email protected]) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
      "They're selling independence, Actors in the White House,
       Acid indigestion, Mortage on my life."
         -- from "Ramble Tamble" by CCR 

 "Vote for me if you want to live" -- the Gubernator

Once inbp.com reads in the kernel and initrd, it looks like your linuxrc just insmods the netwrok driver and iscsi mod. At this point (after you insmod the network driver but before you insmod iscsi.ko.) you can start the networking interface from userspace. Include a statically compiled dhcp program and run it from the linuxrc.

Mike

How about if IBM releases the patent clauses on iBOOT so that any body can use the INT13/INT19 BIOS callers so Linux can be found as a bootable iscsi disk during POST?

Just a comment, given IBM's statements of "open spec" and/or "open standards". Clearly this would cause IBM to lose a market position, and would be bad business practices, but see the fore mentioned position the organization promotes.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

Hi Surekha,

You can use a set of tools from "busybox" that includes a small statically linked "shell" and dhcp client utility program(s) and create a initrd (ramdisk) that download via pxelinux.

The linuxrc script would then bring up the
network something like :

insmod <your nic>
ifconfig up eth0 my_ip_addr
dhcpl > target_info # query the server
insmod iscsi_mod
iscsi_conf cat target_info # user utility to start iscsi

Ping me offline and I can share the
diskless boot sequence I did with unh-iscsi
ramdisk if your interested.

John Donnelly AATTTT HH PP dott COM

__________________________________
Do you Yahoo!?
Exclusive Video Premiere - Britney Spears
http://launch.yahoo.com/promos/britneyspears/

During shutdown iSCSI driver gets signal to stop itself. In turn iSCSI driver umounts all the partitions present of iSCSI disks, makes all the iSCSI disks unavailble to the system and gracefully terminate.
But if iSCSI disk contains the root partition OR is part of root partition then iSCSI driver must not try to umount the root partition and the iSCSI disks must be available until the buffer cache gets flushed properly. For this to happen iSCSI driver must be aware of all the iSCSI disk(s) that constitute the root partition/devices.

Q. What kernel data structures/API's are available that provide the
information about root device(s)?

During shutdown iSCSI driver must bring up the network interface, if it goes down, so to make available the iSCSI disk(s) on which the root partition exists. iscsi_set_if_addr is used to bring up the newtwork interface at the time of shutdown.
Q. Is ip autoconfig code helps us to bring up the newtwork interface at the time of shutdown?

Thanks,
Naveen.

I think you'd rather want a reboot notifier for after the final sync..

there is no such information. you can only know the root device of a particular process and that information is not available to drivers.

you don't need to shut down interfaces on system shutdown.

no.

Hi ,
The following was one of your comments on the iSCSI driver:

"having multiple kmap() in the same process at the same time can kmap(), you redesign iscsi_xmit_task/iscsi_xmit_data/iscsi_recv_data not to do that. for the tx path use ->sendpage to avoid a data copy and kmapping altogether"

In the first line did you intend to say having multiple kmap() can deadlock? Regarding usage of sendpage and avoiding kmapping altogether,
we need to access the data, to calculate data digest. If we do not use kmap how else do we achieve it?

Please let us know.

Thanx
N.C.Krishna Murthy

Yes, he did.

Use the digest API. See include/linux/crypto.h. For example, crypto_digest_update() takes a scatterlist. Scatterlists include a page/offset, so your code doesn't need to kmap() at all.

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Yes.

Well, with software crypto someone will have to do the kmap (or kmap_atomic), but I'd rather leave that as an implementation detail to the crypto API as I already suggested earlier.

I see what you're trying to do, but I don't see how this is any different from the NFS root problem, which has already been long solved in distribution startup systems.

James

Hi,
The following is the reason for sending the TEST_UNIT_READY and
START_STOP initialization command in the iSCSI driver:-

The iSCSI driver receives a CHECK CONDITION with sense key, NOT READY and ASC/ASCQ=04/02 (i.e. an initialization command required). The Linux SCSI mid-layer does not handle this condition. Hence the TUR and START STOP initialization commands are sent from the iSCSI driver.

Should this condition be necessarily handled in the SCSI mid-layer or the low-level driver?

Thanks,
Sachin

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Christoph Hellwig
Sent: Monday, October 27, 2003 9:10 PM
To: Krishna Murthy
Cc: [email protected]; [email protected]
Subject: Re: Request for review of Linux iSCSI driver version 4.0.0.1

available at

Here's a list of a few comments I've come up with. I haven't yet looked at the data structurws much, but it seems there's a complete lack of refcounting. Also there's lots of comments were ysou complain about scsi midlayer misbehaviour or bugs - please submit patches or submit workarounds when you encounter them, that's what open source is all about!

Also the same question that went to the unh folks goes to you: why do you think your implementation is better than theirs and why do you think cooperation on one iscsi stack is impossible.

here's the list:

iscsi-probe.c:
- lots of unchecked kmalloc()s
- you can't allocate scsi_cmnds yourself but _must_ use
scsi_get_command
- dito for scsi_device.
- the whole iscsi_do_cmnd use looks very very strange. you're
probably
much better of to use the scsi_request based interfaces and use
midlayer
queueing and sometimes even higher level interfaces, e.g. your
iscsi_send_tur
coudld be replaced by an scsi_ioctl(sdp,
SCSI_IOCTL_TEST_UNIT_READY, NULL)
I still don't understand, though why the heck you need to do a
TUR in
a lowlevel driver and even more need a thread do a single TUR,
started from
another kernel tread. there more I dig into it your probing and
thread
abuse looks horribly screwed.
- reinitilialize disk is another such candidate. again why do
you
need to send START_STOP commands from the LLDD, why do you need
another
thread instead of a state machine and again life would be a lot
simpler
by using the midlayer queueing infrastructure.
- dito for the reports luns and inquiry code in
iscsi_detect_luns.
I'm very very unhappy to see a lowlevel driver mess with all
this.
Even if you need to do it from a LLDD use the midlayer
functionality for
it. But I still want to see a very good explanation why you
need this at all.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I think we should handle it in the midlayer. Could you come up with a patch to implement it?

--
Christoph Hellwig <[email protected]> - Freelance Hacker
Contact me for driver hacking and kernel development consulting

Interesting challenge. For a start START_STOP UNIT ** is a SBC-2 command so the mid-layer shouldn't be generating it, the sd driver should. The sd_rw_intr() function in the sd driver could be made to detect ASC/ASCQ=04/02 but it is pretty obviously executing in interrupt context. The sd driver has a tailor made routine to spin up a disk called sd_spinup_disk() but since it calls schedule_timeout() it must be called from process context.

Also once a spinup has commenced, it will take about 30 seconds to complete. In that time an impatient user could try and mount a different partition on the same disk. That process should see a ASC/ASCQ=04/01 ("Logical unit is in process of becoming ready") and it should wait till it clears (and not issue another spinup).

Seems like some infrastructure changes would be needed.

Testing with lk 2.6.0-test9: spinning down a disk that the scsi subsystem had already seen but with nothing mounted on it, then trying to mount a partition on it, causes a lot of ugly messages in my log (at least the machine didn't crash).

** There is also the Power Condition mode page that all SCSI devices may optionally support. Trouble is with my SCSI-3 disks they either don't support it or the one that does gives ASC/ASCQ=04/02 when the disk is spun down (violating SPC-3).

Doug Gilbert

We could call sd_spinup_disk from schedule_work. This still seems better than having a thread in a LLDD just for this.

Looks like we need to handle that case anyway..

Interesting. Now that the scsi_device is part of the driver model we could actually do some fancy things about spinning down disks in a controlled mannor on suspend and wake them up again on resume.

Why doesn't the iscsi target handle the problem?

We already spun the drive up in sd.c during scan + probe, or for removable or write protected media, on open (sd_open calls check_disk_change).

That is, whoever spun down the drive should spin it up.

In Doug's example, he knows he spun it down, and can spin it up again before mounting the disk.

-- Patrick Mansfield

It needs to be handled in the iSCSI driver.
Extending the operation to the entire midlayer will cause great grief to all the HBA's which do not even support the abridged version of SBC call. Some support only a subset of the subset of SCSI commands. Until all HBA code has a means to report a positive list of commands supported, only exception callers in the local real/virtual HBA appears doable.

Christoph, until you deal with an iSCSI-FC router and the double transport callers to sanely address FC appearing as DAS, you will not see the reason for nested threads for TRU.

Yeah FC local looks like DAS, but there are several timing layers that local DAS verses ip-storage do not match up.

Just my $0.02

Cheers,

Andre Hedrick
LAD Storage Consulting Group

in the midlayer != enabled unconditionally. We just have much better infrastructure for this than a LLDD.

Hi,
We need the md5 and crc functions from the user level as well as
from the kernel. The crypto API and the CRC32 kernel library functions can solve the problem of md5 and crc computation in the kernel. But its not possible to use these APIs from the user level. One way could be to use system calls. But wouldn't that make the code cumbersome?

Thanks,
Sachin

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Christoph Hellwig
Sent: Monday, October 27, 2003 9:10 PM
To: Krishna Murthy
Cc: [email protected]; [email protected]
Subject: Re: Request for review of Linux iSCSI driver version 4.0.0.1

available at

Here's a list of a few comments I've come up with. I haven't yet looked at the data structurws much, but it seems there's a complete lack of refcounting. Also there's lots of comments were ysou complain about scsi midlayer misbehaviour or bugs - please submit patches or submit workarounds when you encounter them, that's what open source is all about!

Also the same question that went to the unh folks goes to you: why do you think your implementation is better than theirs and why do you think cooperation on one iscsi stack is impossible.

here's the list:

highlevel:
- kill md5.c and iscsi-crc.c and use common cryptoapi md5 / common crc code instead.
- please kill you thousands of atoi/aton reimplementations and look for proper linux functions like simple_strtoul.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Uhm, your user code is free to do whatever it likes, including using its own md5 routines instead of the ones in libssl. But the kernel code should use the kernel facilities.

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Hi Christopher,
Could you please elaborate on this review comment?
- kill ctl_open/ctl_release and move add an owner to the fops
structure for refcounting.

Especially, what do you mean by "move add an owner to the fops structure for refcounting" ? We already use the .owner field in the file_operations template. What else are you refering to?

Thanks,
Sachin

I currently do not have the code in front of me, but IIRC ctl_open/ctl_release was only doing MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT (or even a broken equivalent). These days all refcounting is done by higher levels if you provide a owner (=THIS_MODULE) field in struct file_operations and these methods could be removed. If you already have the owner set that's even easier..

After a device gets detected by iSCSI LLD, it uses iscsi_do_cmnd to send scsi commands to that device even before registering it with the scsi mid-layer.

For example user can configure in iscsi LLD configuration (iscsi.conf) file what various LUNs of a particular target needs to be activated. To acheive this we need to execute REPORT LUN command on target, to find out what various luns are present and selectively issue scsi_add_device for luns that needs to be activated.

All these interfaces requires scsi_device instance which implies that device must be registered with the scsi mid-layer, before using these interfaces.

Naveen.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Couldn't you just use scsi_scan_host_selected from scsi_scan.c for that if it was exported? We already have all that big and perfectly multi-lun capable probing code in the scsi midlayer and it would be a shame to not use it.

scsi_scan_host_selected can be used for activating LUNs, selectively. But that is the second step.
First step is to execute the REPORT LUN command without registering a device with scsi mid-layer. That's what we are acheiving through iscsi_do_cmnd . Once we know what all luns are available for a target, scsi_scan_host_selected can be used to activate the luns, selectively.

Why do you insist on not using a struct scsi_device? You acn still free it if you don't want it. scsi_report_lun is doing all the REPORT_LUNS work for you, no need to duplicate it.

Hi,
We could use probing code from mid layer provided
scsi_scan_host_selected is exported.

iscsi driver does probe for devices at two places
a)Once after session establishment.
b)Whenever an iSCSI async message indicates a change in "REPORTED LUNS DATA". (addition/deletion of luns on the target)

There are no issues in calling scsi_scan_host_selected() at a). Addition of luns could be handled by calling scsi_scan_host_selected
with rescan parameter set to 1.
Deletion of luns is not handled. scsi_scan_target does not worry about
devices which were previously configured, but currently not available.Would you recommend handling this in scsi_scan_target (or a new API) or handle in iSCSI using scsi_request based interfaces?

Please let us know.

Thanx
N.C.Krishna Murthy

This sounds like an async event which could be handled by the hotplug system. Would there be a problem with simply transmitting this to a user level utility and having it trigger the rescan through the exported sysfs interfaces?

We could do with a generic infrastructure within SCSI for generating these events, anyway, if you want to go that route...

James

Hi,
Please find my replies/quieries interspersed in your reply.

Having user level utility trigger the rescan thru sysfs interfaces will call scsi_scan() which will inturn call scsi_scan_host_selected(). Instead wouldn't it be preferable to call scsi_scan_host_selected() from the iSCSI driver itself?

I am not much familiar with SCSI hotplug.

If the generic infrastructure which you mentioned does not exist now and in case there is a plan to do one such thing is there any framework for that? Wouldn't that require a kernel API which is capable of both "adding new luns" and "deleting luns which no longer exist"?

Please let us know
Thanx
N.C.Krishna Murthy

Well, consider all the ramifications: what happens if the reconfiguration is deleting an existing LUN with a mounted filesystem? What happens if the user wants to do something with the new LUN as soon as it appears (like fold it into a VM volume, or format it). The hotplug system is designed to propagate events that others might be interested in, which it strikes me this is. So, if you have to trigger a hotplug event anyway, you may as well go the whole hog and do the reconfiguration as part of the hotplug subsystem.

Well, a hotplug event feeds certain information into the environment when it triggers the call. All we really need is a wrapper in SCSI to add the SCSI information to the environment and trigger the event in a generic fashion.

James

This would be wonderful. I am currently writing a LLD and was going to use the scsi_add_device and scsi_remove_device interfaces to add/remove devices as the devices show up and disappear at runtime, but this was going to require me to go through a bit of complexity using a workqueue to do it at task level. If a LLD had interfaces to do this that could be called at interrupt context these interfaces would be much easier to use.

Brian King
eServer Storage I/O
IBM Linux Technology Center

Hi,
We agree to using the wrapper when we are notified about
"reported lun data change".

However when a session is initially established to an iSCSI target,
luns on the target need to be configured. As mentioned in one of my previous mails , we need to use scsi_scan_host_selected().
Will the function be exported in the next release of 2.6 or do I need to generate a patch for the same?

Please let us know.

Thanx
N.C.Krishna Murthy

You'd be better off letting the OS decide which LUNs to use, and not masking anything in the low-level driver. If LUN masking is needed, adding a general mechanism to the kernel to provide LUN masking for any particular target would be a better approach than having each low-level driver implement its own way of masking LUNs.

The main motivation for LUN masking is the limited number of SCSI disk devices Linux can support. Now that Linux has 32-bit device numbers, the SCSI disk driver could be changed to support many more LUNs per target, and the need for LUN masking would be greatly reduced. This would probably be less work than adding a general LUN masking mechanism, and would be useful to a larger number of users than LUN masking would.

Scott M. Ferris,
[email protected] 

Hi,
We do not need scsi_scan_host_selected() for lun masking.

In the current code, after establishing a session with an iSCSI target we a)issue report luns to find out what luns exist on the target. b)Configure each lun using scsi_add_device.
To accomplish a), we use iscsi_do_cmnd().
Linux scsi folks had suggested that we could use the "report lun based" probing infrastructure that exists in the 2.6 kernel instead of duplicating the same in our driver. We need scsi_scan_host_selected() function to utilize the same. As of now (2.6.0-test11) the function is not exported.

Thanx
N.C.Krishna Murthy

The point people are trying to make is that the low-level driver should not be doing either a or b. When an iSCSI target is added, tell hotplug, and let it initiate a scan of that target through sysfs.

Scott M. Ferris,
[email protected] 

Hi,
Thanks for the clarification. As you pointed out,
upon establishing a session with iSCSI target
OR
upon getting notified of Lun(s) additio/deletion.
we will inform hotplug and let it initiate a scan(re) of that target thru sysfs. Please provide the wrapper in SCSI layer.

Thanx
N.C.Krishna Murthy

Case b) could easily use scsi_add_device/scsi_remove_device, couldn't it?

Hi,
The iSCSI async message does not provide information about which
lun(s) got added or deleted. The iSCSI async message only contains a sense data indicating that "reported luns data" changed.(ASC/ASCQ = 3F/0E). Upon receiving this we would have to do a report luns/inquiry, compare with list of luns which were configured, find out the list of luns which got added or deleted and subsequently use scsi_add_device / scsi_remove_device.

Doing this in iSCSI driver would duplicated some code from scsi_report_lun_scan/scsi_sequential_scan.

Instead if we can have scsi_report_lun_scan and scsi_sequential_scan return back a list of luns they configured(includes discovered and already configured luns), scsi_scan_target can traverse the list of devices on this target and use scsi_remove_device for devices (provided device access count = 0) that to do not have an entry in the list. This takes care of both added/deleted luns

Please let me know.

Thanx
N.C.Krishna Murthy

That sounds like a good idea, but do not bother with scsi_sequential_scan, since sparse LUNs usage could cause problems - removing a LUN that still exists and is in use.

So, the calls to scsi_remove_device would only be triggered via report lun scanning.

Their should probably be a different code path and flags/arguments to conditionally trigger the check and possible removal of LUNs.

And the caller needs to have process context, so we can't just call the into the code from the mid-layer.

It would be best to trigger a hotplug callout for the event, and let user land trigger the scan/delete. But, we need early boot time handling or something else (always rescan with removal via startup scripts?) to handle LUNs changing while booting.

Code would also need fixing so we always try a report lun scan if the target exists (code snippet mentioned by James in another post). So if LUN 0 goes away, we still remove devices.

-- Patrick Mansfield

If you already have the LUN specified, why do you need to issue a REPORT LUN command?

I don't know iSCSI specification details, but I would want to specify a target, not a LUN, in a configuration file and have all LUNs on the target be automatically configured.

The iSCSI target itself, or actual storage (if iSCSI is just passing through commands to an actual disk drive) could control what LUNs are seen by an individual host.

-- Patrick Mansfield

On Wednesday 19 November 2003 10:47 pm, Patrick Mansfield wrote:

Lun masking feature is provided by the linux-iscsi driver. In the linux-iscsi driver configuration file user can specify the luns of a target that needs to be activated upon the detection of that target.

User has specified the LUNs that needs to be activated upon the detection of a target. Once that target gets detected then to find out weather the LUNs specified by the user are actually present, we need to issue REPORT LUN command.

User might not want to configure all the LUNs of a target.

Naveen

I'm not sure I agree with that. Users might also want whatever but it's not sensible to do that. Ignore the user request now, if we see real-world use for that we'll implement it in the midlayer. And if you think it's really important implement it in the midlayer now and keep the patch around for users that want it.

we are actually trying to do optimization by sending REPORT LUN command before actually configuring the target in the mid layer and configure only the devices that are actually present.
Agreed its not sensible to do it right now. Let's the need of it should be felt in the real-world, after that it can be implemented in the mid-layer.

We will remove iscsi_do_cmnd as pointed out by you from iscsi LLD.

Thanks,
Naveen.

Hi,
I was wondering how to generate ILI condition while reading from a
tape.

A few lines from read_tape(), file st.c
.....
if (SRpnt->sr_sense_buffer[2] & 0x20) { /* ILI */

             if (STp->block_size == 0) {
                  if (transfer < 0) {
                         if (STps->drv_block >= 0)
                             STps->drv_block+= 1;
                             return (-ENOMEM);
                  }
                  (STp->buffer)->buffer_bytes = bytes - transfer;
              } else {
                    scsi_release_request(SRpnt);
                    SRpnt = *aSRpnt = NULL;
                    if (transfer == blks) { /* We did not get anything,
error */
                       printk(KERN_NOTICE "st%d: Incorrect block
size.\n", dev);
                                                                      
                if(STps->drv_block>=0)                                                                               STps->drv_block+= blks - transfer + 1;
                                                                                st_int_ioctl(STp, MTBSR, 1);
                        return (-EIO);
                      }

/* We have some data, deliver it */

(STp->buffer)->buffer_bytes = (blks - transfer) * STp->block_size;
                
        DEBC(printk(ST_DEB_MSG                                                        "st%d: ILI but enough data received %ld %d.\n",
               dev, count, (STp->buffer)->buffer_bytes));
         if (STps->drv_block >= 0)
                STps->drv_block += 1;
        if (st_int_ioctl(STp, MTBSR, 1))    return (-EIO);
       }

I want to make the driver print the message "ILI but enough data received." to generate an error condition that my new driver (similar to tape driver) encountering while running with a software from Veritas.

Kallol Biswas <[email protected]>
www.nucleodyne.com

IIRC all you need is a tape with variable block size and you just read the wrong blocksize.

Josef

Josef Möllers (Pinguinpfleger bei FSC)
	If failure had no penalty success would not be a prize
						-- T.  Pratchett
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thank you. I already have tested the code path with dd command.

Kallol Biswas <[email protected]>

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

md5: That appears straight-forward, and has been implemented, subject
to testing. SELECT CRYPTO and SELECT MD5 makes it available in the kernel.

crc32: Ah, the iSCSI crc32 uses a different polynomial (stronger) than
Ethernet and the rest. We argued during the RFC process for just using Ethernet's as good enough, but were voted down. So we are stuck with a separate implementation in this case.

Clay Haapala ([email protected]) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
     Well, looks like hypocrisy is back on the airwaves.
       C'mon, Rush!  Do the crime, do the time, right?

OK, but please convert it to live in the crypto/ directory and use the crypto interfaces. That way if some unrelated project decides to implement the same algorithm, we don't end up with two copies of it.

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

On Fri, 21 Nov 2003, Matthew Wilcox spake thusly:

I'm happy to do that. Having established that it is not a redundant implementation, though, I would like to have it not be a factor with respect to driver inclusion, as there are bigger fish to fry.

That being said, I'll probably jump on it, anyway.

Question: Should the implementation use the crypto api or be added to the lib/crc32 code?

Clay Haapala ([email protected]) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
     Well, looks like hypocrisy is back on the airwaves.
       C'mon, Rush!  Do the crime, do the time, right?

I've talen a quick look at the code and it looks like you're doing the crc not only for control but also for data packages, thus it seems the best idea to make it a digest in the crypto framework.

Hi,

eh_strategy_handler method.

By implementing the iSCSI driver specific strategy_handler we will need to duplicate most of the mid layer strategy handler logic only to accommodate the above condition.

Is that justified ? Or is it ok, to fix the above case in scsi_error.c ?

Thanks,
surekha

I haven't followed the code closely, but the comment reads like scsi_error.c is mostly useless for you - if that were true I'd suggest using eh_strategy_handler, if on the other hand you can fix scsi_error.c easily it's better to use it.

The eh_strategy_handler only allows changing what happens after error handling has started, but cannot modify when the error handler runs.

Using your own won't "avoid blocking all I/O to the HBA".

i.e., we wake up the error handler in scsi_error.c via:

void scsi_eh_wakeup(struct Scsi_Host *shost)

{
	if (shost->host_busy == shost->host_failed) {
		up(shost->eh_wait);
		SCSI_LOG_ERROR_RECOVERY(5,
				printk("Waking error handler thread\n"));
	}
}

We need a new timeout + canceller mechanism, so that a single command can be cancelled without waiting for all other IO on an adapter to complete. Errors must still escalate some how so device, bus, or adapter resets can (under some conditions) still be issued.

The qla2xxx hacks around this by adding its own timer that is shorter than the scsi cmd timeout. Is that a good or a bad hack? I don't know.

-- Patrick Mansfield

Well, it's a hack :) Although one that works. I agree with your above
statement that we need to add support for that to the midlayer.

James Bottomley agreed to add "DID_" code which ensures retry of scsi command without affecting the retry count.
Once that will be in place then instead of faking sense data in case of "check condition with no sense" iSCSI LLD will use it to make sure command will be retried.

Naveen.