Re: [ANNOUNCE] iscsi-initiator-core 1.6.2.0-rc1 for 2.6.12-rc1

["Nicholas A. Bellinger" <nick@pyxtechnologies.com>]

On Sat, 2005-04-23 at 21:49 -0500, James Bottomley wrote:
> OK, I assessed this for inclusion.
> 
> The feedback I had based on my last review was
> 
> 1) Use the iscsi transport class.

Ok, this is something that I will work on against 2.6.12-rc3 tonight.
My primary concern is the parameters that are currently defined as
session wide when they are specific to only an connection context:

1) HeaderDigest & DataDigest
2) IP/Port
3) IFMarker, OFMarker, OFMarkInt and IFMarkInt.

My primary concern is that this will cause breakage between the current
linux-iscsi implementation and what correct parameter contexts that are
defined by RFC 3720.  These are used in the correct contexts in
drivers/scsi/iscsi_initiator_core/iscsi_initiator_sysfs.c, and I will
look into seeing how the correct map to what scsi_transport_iscsi.c
currently provides without breaking linux-iscsi, and without causing too
much changes within iscsi-initiator-core.

Aside from the parameter bits, is there anything else that can go into
scsi_transport_iscsi.c that can be shared?  What about
scsi_internal_device_block() and scsi_internal_device_unblock()?  Is
there anything else?

> 2) get rid of MC/S in favour of dm-multipath.

Is it acceptable to disable MC/S in the short term?  I am concerned that
removing something as basic as MC/S will cause other issues that can
potentially cause stability problems.  I have no issues with removing
this feature but would prefer it be done after this stable release.

> 3) Don't try to subvert the SCSI error handler
> 
> Point 3) is much better, but still present in this:
> 
> +	/*
> +	 * This is completion of a given struct scsi_cmnd after an
> +	 * iSCSI exception occured.  Based upon iSCSI exceptions and/or
> +	 * passed action parameter, struct scsi_cmnd->eh_timeout may have
> +	 * been stopped, and needs to be rstarted before completion to
> +	 * the SCSI stack.
> +	 */
> +	if (!(timer_pending(&sc->eh_timeout))) {
> +		sc->eh_timeout.data = (unsigned long) sc;
> +		sc->eh_timeout.expires = (jiffies + sc->timeout_per_command);
> +		sc->eh_timeout.function = scsi_timeout_function;
> +		add_timer(&sc->eh_timeout);
> 

Ok, I think I may have missed this one when I was removing the
eh_timeout bits.  I need to double check that there is no possibility of
the timer being expired at this point.

> However the other two still have not been addressed, so this patch is
> not suitable for inclusion.
> 

I will keep working towards your requests, thanks again your
consideration!


-- 
Nicholas A. Bellinger <nick@pyxtechnologies.com>