Request for review of Linux iSCSI driver version 4.0.0.1

Request for review of Linux iSCSI driver version 4.0.0.1

[Krishna Murthy]

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Thu, Oct 23, 2003 at 05:34:10PM +0530, Krishna Murthy wrote:
> Hi,
> 	Linux iSCSI driver 4.0.0.1 (linux-iscsi-4.0.0.1.tgz) is available at
> 
> http://sf.net/projects/linux-iscsi/

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

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Surekha.PC]

Hi,

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

>	- kill the is_root_disk special casing.  drivers can't know
>	what the root disks is, there might not even be a single one.

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.
	
>	- what the heck is iscsi_set_if_addr?  You have absolutely no
business
>	messing with network device configuration.

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
 

	

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Wed, Oct 29, 2003 at 06:53:43PM +0530, Surekha.PC wrote:
> 
> Hi,
> 
>  I have interspersed my answers below for 2 of your comments.
> 
> >	- kill the is_root_disk special casing.  drivers can't know
> >	what the root disks is, there might not even be a single one.
> 
> 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.

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.

> 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.

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Mike Christie]

Hi Surekha,

>>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.
> 
> 
> 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.
> 

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Clay Haapala]

On Wed, 29 Oct 2003, Mike Christie outgrape:
> Hi Surekha,
> 
>>>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.
>> 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.
>>
> 
> 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,

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 (chaapala@cisco.com) 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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Mike Christie]

> 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.
> 

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

Mike

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Clay Haapala]

On Wed, 29 Oct 2003, Mike Christie outgrape:
> 
>> 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.
>>
> 
> Oops. I forgot you guys use iNBP.com. I was thinking of pxelinux.
> 
> Mike
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 (chaapala@cisco.com) 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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Mike Christie]

Clay Haapala wrote:

> On Wed, 29 Oct 2003, Mike Christie outgrape:
> 
>>>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.
>>>
>>
>>Oops. I forgot you guys use iNBP.com. I was thinking of pxelinux.
>>
>>Mike
> 
> 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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[jd]

--- Mike Christie <mikenc@us.ibm.com> wrote:
> Hi Surekha,
> 
> >>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.
> > 
> > 
> > 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.
> > 
> 
> 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
> 

  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/

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Andre Hedrick]

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

On Wed, 29 Oct 2003, Mike Christie wrote:

> Clay Haapala wrote:
> 
> > On Wed, 29 Oct 2003, Mike Christie outgrape:
> > 
> >>>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.
> >>>
> >>
> >>Oops. I forgot you guys use iNBP.com. I was thinking of pxelinux.
> >>
> >>Mike
> > 
> > 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.
> 
> 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
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Sachin Mhatre (smhatre)]

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: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Christoph Hellwig
Sent: Monday, October 27, 2003 9:10 PM
To: Krishna Murthy
Cc: linux-scsi@vger.kernel.org; davmyers@cisco.com
Subject: Re: Request for review of Linux iSCSI driver version 4.0.0.1


On Thu, Oct 23, 2003 at 05:34:10PM +0530, Krishna Murthy wrote:
> Hi,
> 	Linux iSCSI driver 4.0.0.1 (linux-iscsi-4.0.0.1.tgz) is
available at
> 
> http://sf.net/projects/linux-iscsi/

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 majordomo@vger.kernel.org More majordomo info
at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Thu, Nov 06, 2003 at 03:12:51PM +0530, Sachin Mhatre (smhatre) wrote:
> 
> 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?

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

--
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Andre Hedrick]

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

On Thu, 6 Nov 2003, Sachin Mhatre (smhatre) wrote:

> 
> 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: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Monday, October 27, 2003 9:10 PM
> To: Krishna Murthy
> Cc: linux-scsi@vger.kernel.org; davmyers@cisco.com
> Subject: Re: Request for review of Linux iSCSI driver version 4.0.0.1
> 
> 
> On Thu, Oct 23, 2003 at 05:34:10PM +0530, Krishna Murthy wrote:
> > Hi,
> > 	Linux iSCSI driver 4.0.0.1 (linux-iscsi-4.0.0.1.tgz) is
> available at
> > 
> > http://sf.net/projects/linux-iscsi/
> 
> 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 majordomo@vger.kernel.org More majordomo info
> at  http://vger.kernel.org/majordomo-info.html
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Thu, Nov 06, 2003 at 03:10:59AM -0800, Andre Hedrick wrote:
> 
> 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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Douglas Gilbert]

'Christoph Hellwig' wrote:
> On Thu, Nov 06, 2003 at 03:12:51PM +0530, Sachin Mhatre (smhatre) wrote:
> 
>>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?
> 
> 
> I think we should handle it in the midlayer.  Could you come up with a
> patch to implement it?

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Fri, Nov 07, 2003 at 06:55:01PM +1000, Douglas Gilbert wrote:
> > I think we should handle it in the midlayer.  Could you come up with a
> > patch to implement it?
> 
> 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.

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

> 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).

Looks like we need to handle that case anyway..

> ** 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).

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Patrick Mansfield]

On Fri, Nov 07, 2003 at 06:55:01PM +1000, Douglas Gilbert wrote:
> 'Christoph Hellwig' wrote:
> > On Thu, Nov 06, 2003 at 03:12:51PM +0530, Sachin Mhatre (smhatre) wrote:

> >>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?
> > 
> > 
> > I think we should handle it in the midlayer.  Could you come up with a
> > patch to implement it?

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

On Wednesday 29 October 2003 07:15 pm, 'Christoph Hellwig' wrote:
> On Wed, Oct 29, 2003 at 06:53:43PM +0530, Surekha.PC wrote:
> > Hi,
> >
> >  I have interspersed my answers below for 2 of your comments.
> >
> > >	- kill the is_root_disk special casing.  drivers can't know
> > >	what the root disks is, there might not even be a single one.
> >
> > 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.
>
> 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.

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)?

>
> > 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.
>
> 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.

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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Tue, Nov 11, 2003 at 05:26:36PM +0530, Naveen Burmi wrote:
> 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.

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

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

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

> > that instead of creating such bogus hacks.  A driver has no business
> > dealing with the network configuration.
> 
> 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. 

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

> Q. Is ip autoconfig code helps us to bring up the newtwork interface at the 
> time of shutdown?

no.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[James Bottomley]

On Tue, 2003-11-11 at 03:56, Naveen Burmi wrote:
> 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.

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

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Sachin Mhatre (smhatre)]

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: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Christoph Hellwig
Sent: Monday, October 27, 2003 9:10 PM
To: Krishna Murthy
Cc: linux-scsi@vger.kernel.org; davmyers@cisco.com
Subject: Re: Request for review of Linux iSCSI driver version 4.0.0.1


On Thu, Oct 23, 2003 at 05:34:10PM +0530, Krishna Murthy wrote:
> Hi,
> 	Linux iSCSI driver 4.0.0.1 (linux-iscsi-4.0.0.1.tgz) is
available at
> 
> http://sf.net/projects/linux-iscsi/

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 majordomo@vger.kernel.org More majordomo info
at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Matthew Wilcox]

On Thu, Nov 13, 2003 at 08:00:11PM +0530, Sachin Mhatre (smhatre) wrote:
> 	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?

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Krishna Murthy]

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



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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Matthew Wilcox]

On Wed, Nov 19, 2003 at 06:10:30PM +0530, Krishna Murthy wrote:
> 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?

Yes, he did.

> 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?

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

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Sachin Mhatre (smhatre)]

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Wed, Nov 19, 2003 at 06:34:39PM +0530, Sachin Mhatre (smhatre) wrote:
> 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?

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..

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Wed, Nov 19, 2003 at 06:10:30PM +0530, Krishna Murthy wrote:
> 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?

Yes.

> 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?

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Wed, Nov 19, 2003 at 08:18:32PM +0530, Naveen Burmi wrote:
> On Monday 27 October 2003 09:09 pm, Christoph Hellwig wrote:
> 
> > iscsi-probe.c:
> > 	- the whole iscsi_do_cmnd use looks very very strange.  
> 
> 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.

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

On Monday 27 October 2003 09:09 pm, Christoph Hellwig wrote:

> iscsi-probe.c:
> 	- the whole iscsi_do_cmnd use looks very very strange.  

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.

>          you're probably
> 	much better of to use the scsi_request based interfaces and use midlayer
> 	queueing and sometimes even higher level interfaces

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

On Wednesday 19 November 2003 08:18 pm, Christoph Hellwig wrote:
> On Wed, Nov 19, 2003 at 08:18:32PM +0530, Naveen Burmi wrote:
> > On Monday 27 October 2003 09:09 pm, Christoph Hellwig wrote:
> > > iscsi-probe.c:
> > > 	- the whole iscsi_do_cmnd use looks very very strange.
> >
> > 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.
>
> 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.


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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Wed, Nov 19, 2003 at 08:49:36PM +0530, Naveen Burmi wrote:
> 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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Patrick Mansfield]

On Wed, Nov 19, 2003 at 08:18:32PM +0530, Naveen Burmi wrote:

> 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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

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.

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

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.
 
>
> 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.
>

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

> 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

Naveen

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Thu, Nov 20, 2003 at 07:02:06PM +0530, Naveen Burmi 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.
> 
> > If you already have the LUN specified, why do you need to issue a REPORT
> > LUN command?
> 
> 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.
>  
> >
> > 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.
> >
> 
> User might not want to configure all the LUNs of a target. 

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

On Thursday 20 November 2003 07:04 pm, Christoph Hellwig wrote:
> On Thu, Nov 20, 2003 at 07:02:06PM +0530, Naveen Burmi 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.
> >
> > > If you already have the LUN specified, why do you need to issue a
> > > REPORT LUN command?
> >
> > 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.
> >
> > > 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.
> >
> > User might not want to configure all the LUNs of a target.
>
> 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. 

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Clay Haapala]

On Mon, 27 Oct 2003, Christoph Hellwig said:
> 
> highlevel:
> 	- kill md5.c and iscsi-crc.c and use common cryptoapi
> 	md5 / common crc code instead.

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 (chaapala@cisco.com) 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?

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Matthew Wilcox]

On Fri, Nov 21, 2003 at 10:42:40AM -0600, Clay Haapala wrote:
> On Mon, 27 Oct 2003, Christoph Hellwig said:
> > 
> > highlevel:
> > 	- kill md5.c and iscsi-crc.c and use common cryptoapi
> > 	md5 / common crc code instead.
> 
> 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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Clay Haapala]

On Fri, 21 Nov 2003, Matthew Wilcox spake thusly:
> On Fri, Nov 21, 2003 at 10:42:40AM -0600, Clay Haapala wrote:
>> On Mon, 27 Oct 2003, Christoph Hellwig said:
>> > 
>> > highlevel:
>> > 	- kill md5.c and iscsi-crc.c and use common cryptoapi
>> > 	md5 / common crc code instead.
>> 
>> 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.
> 
> 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.

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 (chaapala@cisco.com) 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?

How to generate ILI condtion on a tape device

[Kallol Biswas]

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 <nucleodyne@comcast.net>
www.nucleodyne.com

RE: Request for review of Linux iSCSI driver version 4.0.0.1

[Surekha.PC]

Hi,

>>  	- 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.

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Mon, Nov 24, 2003 at 11:39:48AM +0530, Surekha.PC wrote:
> 
> Hi,
> 
> >>  	- 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.
> 
> 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 ?

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.

Re: How to generate ILI condtion on a tape device

[Josef Möllers]

> Kallol Biswas wrote:
> 
> Hi,
>   I was wondering how to generate ILI condition while reading from a
> tape.

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Patrick Mansfield]

On Mon, Nov 24, 2003 at 07:48:30AM +0000, 'Christoph Hellwig' wrote:
> On Mon, Nov 24, 2003 at 11:39:48AM +0530, Surekha.PC wrote:
> > 
> > Hi,
> > 
> > >>  	- 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.
> > 
> > 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 ?
> 
> 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

Re: How to generate ILI condtion on a tape device

[Kallol Biswas]

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

On Mon, 2003-11-24 at 00:31, Josef Möllers wrote:
> > Kallol Biswas wrote:
> > 
> > Hi,
> >   I was wondering how to generate ILI condition while reading from a
> > tape.
> 
> IIRC all you need is a tape with variable block size and you just read
> the wrong blocksize.
> 
> 
> Josef
-- 
Kallol Biswas <nucleodyne@comcast.net>

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Fri, Nov 21, 2003 at 12:18:17PM -0600, Clay Haapala wrote:
> Question:  Should the implementation use the crypto api
> or be added to the lib/crc32 code?

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

['Christoph Hellwig']

On Mon, Nov 24, 2003 at 12:45:43PM -0800, Patrick Mansfield wrote:
> 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.

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.  

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Krishna Murthy]

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
		

On Wednesday 19 Nov 2003 8:18 pm, Christoph Hellwig wrote:
> On Wed, Nov 19, 2003 at 08:18:32PM +0530, Naveen Burmi wrote:
> > On Monday 27 October 2003 09:09 pm, Christoph Hellwig wrote:
> > > iscsi-probe.c:
> > > 	- the whole iscsi_do_cmnd use looks very very strange.
> >
> > 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.
>
> 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.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[James Bottomley]

On Mon, 2003-12-01 at 06:10, Krishna Murthy wrote:
> 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) 

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Christoph Hellwig]

On Mon, Dec 01, 2003 at 05:40:09PM +0530, Krishna Murthy wrote:
> 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?

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[N.C.Krishna Murthy]

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



On Monday 01 Dec 2003 8:57 pm, Christoph Hellwig wrote:
> On Mon, Dec 01, 2003 at 05:40:09PM +0530, Krishna Murthy wrote:
> > 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?
>
> Case b) could easily use scsi_add_device/scsi_remove_device, couldn't it?
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Patrick Mansfield]

On Tue, Dec 02, 2003 at 11:22:47AM +0530, N.C.Krishna Murthy wrote:
> 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

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[N.C.Krishna Murthy]

Hi,
	Please find my replies/quieries interspersed in your reply.
On Monday 01 Dec 2003 8:52 pm, James Bottomley wrote:
> On Mon, 2003-12-01 at 06:10, Krishna Murthy wrote:
> > 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)
>
> 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?
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?


>
> We could do with a generic infrastructure within SCSI for generating
> these events, anyway, if you want to go that route...
>
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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[James Bottomley]

On Thu, 2003-12-04 at 07:30, N.C.Krishna Murthy wrote:
> 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?

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.

> > We could do with a generic infrastructure within SCSI for generating
> > these events, anyway, if you want to go that route...
> >
> 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"?

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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Brian King]

James Bottomley wrote:
>>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"?
> 
> 
> 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.
> 


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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[N.C.Krishna Murthy]

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
	
On Friday 05 Dec 2003 9:03 pm, James Bottomley wrote:
> On Thu, 2003-12-04 at 07:30, N.C.Krishna Murthy wrote:
> > 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?
>
> 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.
>
> > > We could do with a generic infrastructure within SCSI for generating
> > > these events, anyway, if you want to go that route...
> >
> > 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"?
>
> 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

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Scott M. Ferris]

N.C.Krishna Murthy wrote:
> 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?

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,
sferris@acm.org 

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[N.C.Krishna Murthy]

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




On Monday 08 Dec 2003 9:16 pm, Scott M. Ferris wrote:
> N.C.Krishna Murthy wrote:
> > 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?
>
> 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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Scott M. Ferris]

N.C.Krishna Murthy wrote:
>
> 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.
> 

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,
sferris@acm.org 

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[Naveen Burmi]

On Monday 24 November 2003 01:18 pm, 'Christoph Hellwig' wrote:
> On Mon, Nov 24, 2003 at 11:39:48AM +0530, Surekha.PC wrote:
> > Hi,
> >
> > >>  	- 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.
> >
> > 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 ?
>
> 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.

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.

Re: Request for review of Linux iSCSI driver version 4.0.0.1

[N.C.Krishna Murthy]

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

On Friday 05 Dec 2003 9:03 pm, James Bottomley wrote:
>
> 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
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html