Re: [PATCH 3/3] iscsi_tcp: Enable any size command

[Boaz Harrosh <bharrosh@panasas.com>]

James Bottomley wrote:
> On Tue, 2008-05-13 at 11:52 +0300, Boaz Harrosh wrote:
>> James Bottomley wrote:
>>> On Wed, 2008-04-30 at 11:30 +0300, Boaz Harrosh wrote:
>>>> Let through upto the largest command of 260 defined by the scsi standard.
>>>> iscsi core supports this already. Now that the scsi-ml supports it we can
>>>> start using large commands.
>>>>
>>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>>>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>>>> ---
>>>>  drivers/scsi/iscsi_tcp.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
>>>> index 72b9b2a..826c97c 100644
>>>> --- a/drivers/scsi/iscsi_tcp.c
>>>> +++ b/drivers/scsi/iscsi_tcp.c
>>>> @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = {
>>>>  	.host_template		= &iscsi_sht,
>>>>  	.conndata_size		= sizeof(struct iscsi_conn),
>>>>  	.max_conn		= 1,
>>>> -	.max_cmd_len		= 16,
>>>> +	.max_cmd_len		= SCSI_MAX_VARLEN_CDB_SIZE,
>>>>  	/* session management */
>>>>  	.create_session		= iscsi_tcp_session_create,
>>>>  	.destroy_session	= iscsi_tcp_session_destroy,
>>> OK, this isn't quite right.  The escb definition in iscsi.h is:
>>> struct iscsi_ecdb_ahdr {
>>> 	__be16 ahslength;	/* CDB length - 15, including reserved byte */
>>> 	uint8_t ahstype;
>>> 	uint8_t reserved;
>>> 	/* 4-byte aligned extended CDB spillover */
>>> 	uint8_t ecdb[260 - ISCSI_CDB_SIZE];
>>> };
>>>
>>> Either that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE or we need to
>>> hard code 260 in the max_cmd_len.
>>>
>> Yes that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE. The reason it
>> is not is because that code is much older than the definition of 
>> SCSI_MAX_VARLEN_CDB_SIZE.
>>
>>> Since SCSI_MAX_VARLEN_CDB_SIZE is really a useless constant (nothing
>>> depends on it), and internal packets in iscsi depend on this, it
>>> probably makes the most sense for this to be an iscsi local constant.
>>>
>> As you said below, this is not an iscsi limitation it is a scsi 
>> limitation. Logically it belongs to scsi.h near the varlen definitions.
>> If you prefer hard coded constants I don't mind, just that from the school
>> I came from they would fail me if I did that, even for a single user.
>>
>>> The value (260) also looks a bit bogus, isn't 262 the maximum possible
>>> size for a 0x7f variable length command?  The iSCSI maxiumum is far
>>> higher than this (but no protocol sends anything above the 0x7f maximum
>>> currently).
>>>
>> 260 comes from the scsi standard. The 8th byte of a scsi varlen header
>> is a one byte length specifier. (see struct scsi_varlen_cdb_hdr in scsi.h)
>> Now the standard says that the header must be 4 bytes aligned so the 
>> maximum that can be written in that byte is 252, plus the constant 8.
> 
> I don't think it can be alignment issues otherwise six byte commands
> like READ_6/WRITE_6 would be illegal.  I don't think there are any
> alignment requirements per se.  However, it does look like the
> definition section of SAM-3:3.1.15 does say "... or a variable length of
> between 12 and 260 bytes" with no reason given, so that will do.
> 

It's only for varlen. 
>From SCSI_Primary_Commands-3-spc3r23
section 4.3.3 The variable length CDB formats:

"The ADDITIONAL CDB LENGTH field specifies the number of additional CDB bytes. 
This value in the ADDITIONAL CDB LENGTH field shall be a multiple of 4"

> James
> 

Thanks
Boaz