RE: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

RE: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Moore, Eric Dean]

I will work on that.  

I had avoided it due the comments in the code
from sralston and pdelaney about sg interface 
gererating wrong direction in some cases.  

Eric


On Monday, January 26, 2004 9:45 AM, James Bottomley wrote:
> On Mon, 2004-01-26 at 10:37, Moore, Eric Dean wrote:
> > Here is another update for the MPT Fusion drivers.
> 
> Thanks.
> 
> > * added CONFIG_LBA, READ16, WRITE16 support
> 
> I'd like to see this routine to determine direction go away 
> in favour of
> just using the direction the mid layer sends down.  However, 
> this isn't
> a blocking item, I'll try and get this update into early 2.6.2
> 
> James
> 
> 

RE: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[James Bottomley]

On Mon, 2004-01-26 at 11:01, Moore, Eric Dean wrote:
> I will work on that.  

Thanks.

> I had avoided it due the comments in the code
> from sralston and pdelaney about sg interface 
> gererating wrong direction in some cases.  

If you find a problem, I'll fix the generic code...

James

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Christoph Hellwig]

On Mon, Jan 26, 2004 at 11:05:26AM -0600, James Bottomley wrote:
> > I had avoided it due the comments in the code
> > from sralston and pdelaney about sg interface 
> > gererating wrong direction in some cases.  
> 
> If you find a problem, I'll fix the generic code...

I think it's just very old comments.  We had such a problem in early 2.4
kernels, but it has been fixed for ages.  If it wasn't most of the scsi
drivers would have a big problem.

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Jeremy Higdon]

On Mon, Jan 26, 2004 at 07:34:13PM +0000, Christoph Hellwig wrote:
> On Mon, Jan 26, 2004 at 11:05:26AM -0600, James Bottomley wrote:
> > > I had avoided it due the comments in the code
> > > from sralston and pdelaney about sg interface 
> > > gererating wrong direction in some cases.  
> > 
> > If you find a problem, I'll fix the generic code...
> 
> I think it's just very old comments.  We had such a problem in early 2.4
> kernels, but it has been fixed for ages.  If it wasn't most of the scsi
> drivers would have a big problem.

Aren't there three interfaces for sg, or did the early ones go away
in 2.6?  I recall that we ran into trouble with sg setting the
direction incorrectly in 2.4 kernels when using older interface,
I think.

jeremy

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Douglas Gilbert]

Jeremy Higdon wrote:
> On Mon, Jan 26, 2004 at 07:34:13PM +0000, Christoph Hellwig wrote:
> 
>>On Mon, Jan 26, 2004 at 11:05:26AM -0600, James Bottomley wrote:
>>
>>>>I had avoided it due the comments in the code
>>>>from sralston and pdelaney about sg interface 
>>>>gererating wrong direction in some cases.  
>>>
>>>If you find a problem, I'll fix the generic code...
>>
>>I think it's just very old comments.  We had such a problem in early 2.4
>>kernels, but it has been fixed for ages.  If it wasn't most of the scsi
>>drivers would have a big problem.
> 
> 
> Aren't there three interfaces for sg, or did the early ones go away
> in 2.6?  I recall that we ran into trouble with sg setting the
> direction incorrectly in 2.4 kernels when using older interface,
> I think.

Jeremy,
I can't see any such report in my changelogs for the sg
driver during the lk 2.4 series. What was difficult with
the older sg interface (sg_header based) was setting the
command length as this was "guessed" from the first 3 bits
of the opcode (SCSI_IOCTL_SEND_COMMAND ioctl still does this).

Doug Gilbert

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Jeremy Higdon]

On Tue, Jan 27, 2004 at 06:56:35PM +1000, Douglas Gilbert wrote:
> Jeremy Higdon wrote:
> >On Mon, Jan 26, 2004 at 07:34:13PM +0000, Christoph Hellwig wrote:
> >
> >>On Mon, Jan 26, 2004 at 11:05:26AM -0600, James Bottomley wrote:
> >>
> >>>>I had avoided it due the comments in the code
> >>>>from sralston and pdelaney about sg interface 
> >>>>gererating wrong direction in some cases.  
> >>>
> >>>If you find a problem, I'll fix the generic code...
> >>
> >>I think it's just very old comments.  We had such a problem in early 2.4
> >>kernels, but it has been fixed for ages.  If it wasn't most of the scsi
> >>drivers would have a big problem.
> >
> >
> >Aren't there three interfaces for sg, or did the early ones go away
> >in 2.6?  I recall that we ran into trouble with sg setting the
> >direction incorrectly in 2.4 kernels when using older interface,
> >I think.
> 
> Jeremy,
> I can't see any such report in my changelogs for the sg
> driver during the lk 2.4 series. What was difficult with
> the older sg interface (sg_header based) was setting the
> command length as this was "guessed" from the first 3 bits
> of the opcode (SCSI_IOCTL_SEND_COMMAND ioctl still does this).
> 
> Doug Gilbert

Okay.  Here is where we ran into problems.

In sg_write(), if old_hdr.reply_len is greater than SZ_SG_HEADER,
then hp->dxfer_direction is set to SG_DXFER_TO_FROM_DEV.  Then
in sg_common_write(), if hp->dxfer_direction is SG_DXFER_TO_FROM_DEV,
then SRpnt->sr_data_direction is set to SCSI_DATA_READ.

For whatever reason, there seem to be apps out there that issue
SCSI data_out commands using the old interface and yet set the
reply_len in the old header to something larger than SZ_SG_HEADER.

Since we couldn't fix the app (it wasn't ours and we didn't have
source), we added code to our xscsi-scsi i/f.  It's a variation of
code that is in both the LSI and Qlogic drivers (borrowed from LSI,
as you can see from the comment).  Without it, some sg apps just
wouldn't work properly (the app in question that we had trouble
with was a Mylex RAID management tool).

So if you remove this code from the Fusion driver, some of these
things will stop working.  One answer would be to change the
apps, but that isn't really practical.  Another would be to put
this logic into sg, so that it's at least centralized.

jeremy

static int
xscsi_scsi_io_direction(Scsi_Cmnd *cmd)
{
        /*
         * Code borrowed from drivers/message/fusion/mptscsih.c::mptscsih_io_direction()
         * 09/23/2003 by Michael Reed, SGI:
         *    Copied, renamed, removed ifdef (and code) and tweaked.
         */

        switch (cmd->cmnd[0]) { /* hot path */
        case READ_6:
        case READ_10:
                return SCSI_DATA_READ;
                break;
        case WRITE_6:
        case WRITE_10:
                return SCSI_DATA_WRITE;
                break;
        }

        switch (cmd->cmnd[0]) {
        /*  _DATA_OUT commands  */
        case WRITE_LONG:        case WRITE_SAME:        case WRITE_BUFFER:
        case WRITE_12:          case WRITE_VERIFY:      case WRITE_VERIFY_12:
        case COMPARE:           case COPY:              case COPY_VERIFY:                                      
        case SEARCH_EQUAL:      case SEARCH_HIGH:       case SEARCH_LOW:                                       
        case SEARCH_EQUAL_12:   case SEARCH_HIGH_12:    case SEARCH_LOW_12:                                    
        case MODE_SELECT:       case MODE_SELECT_10:    case LOG_SELECT:                                       
        case SEND_DIAGNOSTIC:   case CHANGE_DEFINITION: case UPDATE_BLOCK:                                     
        case SET_WINDOW:        case MEDIUM_SCAN:       case SEND_VOLUME_TAG:                                  
        case REASSIGN_BLOCKS:   case PERSISTENT_RESERVE_OUT:                                                   
        case 0xea:              case 0xa3:                                                                     
                return SCSI_DATA_WRITE;                                                                        
                                                                                                               
        /*  No data transfer commands  */                                                                      
        case SEEK_6:            case SEEK_10:                                                                  
        case RESERVE:           case RELEASE:                                                                  
        case TEST_UNIT_READY:   case START_STOP:                                                               
        case ALLOW_MEDIUM_REMOVAL:                                                                             
                return SCSI_DATA_NONE;                                                                         
                                                                                                               
        /*  Conditional data transfer commands  */                                                             
        case FORMAT_UNIT:                                                                                      
                if (cmd->cmnd[1] & 0x10)        /* FmtData (data out phase)? */                                
                        return SCSI_DATA_WRITE;                                                                
                else                                                                                           
                        return SCSI_DATA_NONE;                                                                 
                                                                                                               
        case VERIFY:                                                                                           
                if (cmd->cmnd[1] & 0x02)        /* VERIFY:BYTCHK (data out phase)? */                          
                        return SCSI_DATA_WRITE;                                                                
                else                                                                                           
                        return SCSI_DATA_NONE;                                                                 
                                                                                                               
        case RESERVE_10:                                                                                       
                if (cmd->cmnd[1] & 0x03)        /* RESERVE:{LongID|Extent} (data out phase)? */                
                        return SCSI_DATA_WRITE;                                                                
                else                                                                                           
                        return SCSI_DATA_NONE;                                                                 
                                                                                                               
        /*  Covered write cases, presume read. */                                                              
        default:                                                                                               
                return SCSI_DATA_READ;                                                                         
        }                                                                                                      

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[Douglas Gilbert]

James Bottomley wrote:
> On Mon, 2004-01-26 at 11:01, Moore, Eric Dean wrote:
> 
>>I will work on that.  
> 
> 
> Thanks.
> 
> 
>>I had avoided it due the comments in the code
>>from sralston and pdelaney about sg interface 
>>gererating wrong direction in some cases.  
> 
> 
> If you find a problem, I'll fix the generic code...

James,
The newer sg_io_hdr based code (as used by the SG_IO ioctl)
requires that the user explicitly define the data direction
if any data transfer is involved. There is however a
SG_DXFER_UNKNOWN which maps to SCSI_DATA_UNKNOWN which
maps to DMA_BIDIRECTIONAL **.

The older sg_header based code (and the SCSI_IOCTL_SEND_COMMAND
ioctl) deduced the direction from various data buffer lengths
provided by the user. If those length implied both read and write
then the data direction is set as "FROM_DEVICE" (i.e. read
dominates).

So the sg (or SG_IO/SCSI_IOCTL_SEND_COMMAND ioctl) user controls
the data direction and they are free to get it wrong:-)


** The Object Storage Device (OSD) commands use bidirectional
data transfers so perhaps we should think about catering for
that.

Doug Gilbert

Re: [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

[James Bottomley]

On Tue, 2004-01-27 at 02:48, Douglas Gilbert wrote:
> So the sg (or SG_IO/SCSI_IOCTL_SEND_COMMAND ioctl) user controls
> the data direction and they are free to get it wrong:-)

Since we do have a mid layer library for this, perhaps we should set it
in sg if the user leaves it apparently unset.

> ** The Object Storage Device (OSD) commands use bidirectional
> data transfers so perhaps we should think about catering for
> that.

Actually, a much more fundamental problem for OSD is going to be >16
byte CDBs.  The mid and block layers today are coded with an internal 16
byte array for the CDB.  That's not very scaleable to the hundreds of
bytes that OSD needs.

As far as bidirectional goes, we should be able to support that today
providing there's just a *single* data buffer, which the OSD spec
implies is the easiest way to support this (although I suspect most SCSI
processor coding won't be expecting to see both a data in and a data out
from a single command).

As I see it, OSD seems to require a different interface from the current
bio layer, so there would be some type of object layer glue between the
fs and the mid-layer, with the block layer relegated simply to queueing
specials for the mid-layer, but since no-one's actually come forward
with an implementation, that's just a guess.

James

bidirectional, long commands + OSD

[Douglas Gilbert]

James Bottomley wrote in thread:
   [PATCH] 2.6.2-rc2 - MPT Fusion driver 3.00.02 update

> On Tue, 2004-01-27 at 02:48, Douglas Gilbert wrote:
> 
>>So the sg (or SG_IO/SCSI_IOCTL_SEND_COMMAND ioctl) user controls
>>the data direction and they are free to get it wrong:-)
> 
> 
> Since we do have a mid layer library for this, perhaps we should set it
> in sg if the user leaves it apparently unset.

It is valid for vendor specific commands to have command lengths
other than those dictated by the top 3 bits of the opcode. The
variable length opcode (0x7f) breaks that convention as well.
The pass-through philosophy is (within reason) "you asked for
it ....".

>>** The Object Storage Device (OSD) commands use bidirectional
>>data transfers so perhaps we should think about catering for
>>that.
> 
> 
> Actually, a much more fundamental problem for OSD is going to be >16
> byte CDBs.  The mid and block layers today are coded with an internal 16
> byte array for the CDB.  That's not very scaleable to the hundreds of
> bytes that OSD needs.

Just did a check on recent drafts and SPC-3 doesn't have any
commands with lengths > 16 bytes but SBC-2 has:
    XDREAD(32)
    XDWRITE(32)
    XDWRITEREAD(32)
    XPWRITE(32)

> As far as bidirectional goes, we should be able to support that today
> providing there's just a *single* data buffer, which the OSD spec
> implies is the easiest way to support this (although I suspect most SCSI
> processor coding won't be expecting to see both a data in and a data out
> from a single command).

... and XDWRITEREAD (both 10 and 32 byte variants) is bidirectional.

> As I see it, OSD seems to require a different interface from the current
> bio layer, so there would be some type of object layer glue between the
> fs and the mid-layer, with the block layer relegated simply to queueing
> specials for the mid-layer, but since no-one's actually come forward
> with an implementation, that's just a guess.

Both Jens and Linus jumped on me when I suggested
similar heresy :-)

For anyone interested, an overview of the proposed Object
Storage Device (OSD) architecture can be found at:
http://www.t10.org/ftp/t10/drafts/osd/osd-r08.pdf
in section 4.2 .

Doug Gilbert

Re: bidirectional, long commands + OSD

[Liran Schour]

Douglas Gilbert wrote in thread:
   bidirectional, long commands + OSD


>>>** The Object Storage Device (OSD) commands use bidirectional
>>>data transfers so perhaps we should think about catering for
>>>that.
>>
>>
>> Actually, a much more fundamental problem for OSD is going to be >16
>> byte CDBs.  The mid and block layers today are coded with an internal 16
>> byte array for the CDB.  That's not very scaleable to the hundreds of
>> bytes that OSD needs.

We can allow the upper layer driver to set the cmd_len and the cdb pointer
to the
desired length, the default will stay 16 byte.

>Just did a check on recent drafts and SPC-3 doesn't have any
>commands with lengths > 16 bytes but SBC-2 has:
>    XDREAD(32)
>    XDWRITE(32)
>    XDWRITEREAD(32)
>    XPWRITE(32)

>> As far as bidirectional goes, we should be able to support that today
>> providing there's just a *single* data buffer, which the OSD spec
>> implies is the easiest way to support this (although I suspect most SCSI
>> processor coding won't be expecting to see both a data in and a data out
>> from a single command).

>... and XDWRITEREAD (both 10 and 32 byte variants) is bidirectional.

A single buffer for both data in and data out will force the user to
re-copy the buffer.
We can solve it by using a scatter-gather list. Each buffer will have its
own data
direction flag.

>> As I see it, OSD seems to require a different interface from the current
>> bio layer, so there would be some type of object layer glue between the
>> fs and the mid-layer, with the block layer relegated simply to queueing
>> specials for the mid-layer, but since no-one's actually come forward
>> with an implementation, that's just a guess.

>Both Jens and Linus jumped on me when I suggested
>similar heresy :-)

Intel's iscsi reference implementation includes an upper layer driver for
OSD, it
also includes a mini filesystem that works on top of it.
This implementation only demonstrate the need to solve this 3 issues: (the
implementation just work around them)
      1. Coupling of the scsi layer with the bio layer.
      2. Bidirectional support.
      3. Extended CDB support.

We will be happy to open this issues for a discussion.

- Liran

Re: bidirectional, long commands + OSD

[James Bottomley]

On Thu, 2004-01-29 at 08:07, Douglas Gilbert wrote:
> James Bottomley wrote in thread:
> > As I see it, OSD seems to require a different interface from the current
> > bio layer, so there would be some type of object layer glue between the
> > fs and the mid-layer, with the block layer relegated simply to queueing
> > specials for the mid-layer, but since no-one's actually come forward
> > with an implementation, that's just a guess.
> 
> Both Jens and Linus jumped on me when I suggested
> similar heresy :-)

Well ... I merely stated what it seems to require.  I didn't recommend
implementing it.

>From what I can tell, the OSD spec seems to be aimed at large clustered
filesystems.  The recommended mapping seems to be <multiple objects> <->
<one file>.  However, since different objects are distinct and currently
require distinct commands to access them, I believe using OSD for a
typical Linux filesystem would be a big loss because most files are
small and we can no-longer elevate the requests.  It would be
interesting to see if this loss could be offset by not having to do
indirect block lookups, but I have my doubts.

James

Re: bidirectional, long commands + OSD

[Liran Schour]

James Bottomley wrote in thread:
   bidirectional, long commands + OSD:

>On Thu, 2004-01-29 at 11:08, Liran Schour wrote:
>> A single buffer for both data in and data out will force the user to
>> re-copy the buffer.
>> We can solve it by using a scatter-gather list. Each buffer will have
its
>> own data
>> direction flag.

>This is not necessarily true.  Just because it's a single buffer doesn't
>mean that the read and write areas cannot be separated inside it.  The
>actual implementation in most drivers uses a single data pointer, so if
>the device starts with DATA IN and transitions to DATA OUT, you should
>get the output data beginning directly after the input data.

>I know the spec implies that you keep two data pointers (because the
>device is allowed to do the DATA IN<->DATA OUT transition many times),
>so possibly we can't get away with something simple as above, but
>implementing separate data pointers would require modifying every
>driver.

Most of the HBAs today can't support execution of OSD SCSI commands due to
the
extended CDB's length and the bidirectional data movement. But when OSD
technology
will catch a momentum we will see more and more vendors that will support
this features.
My point is that maybe we can keep the current interface while expanding it
to support
the needed features by the SCSI OSD commands.

>which OSD devices have you used?  Just ISCSI one's, I assume; what do
>they do with the data in/out transitions?

I have used only ISCSI and TCP/IP (non SCSI) OSD devices. In the devices
that
I have used the transitions of data where isolated - first DATA OUT then
DATA IN.
But an OSD device can do the following data transitions: DATA IN (user
data) ; DATA OUT
(set attributes data and get attributes list) ;  DATA IN (retrieved
attributes data).

- Liran

Re: bidirectional, long commands + OSD

[James Bottomley]

On Fri, 2004-01-30 at 05:39, Liran Schour wrote:
> Most of the HBAs today can't support execution of OSD SCSI commands due to
> the
> extended CDB's length and the bidirectional data movement. But when OSD
> technology
> will catch a momentum we will see more and more vendors that will support
> this features.
> My point is that maybe we can keep the current interface while expanding it
> to support
> the needed features by the SCSI OSD commands.

Actually, I'm sure they can.  Most HBA's operate using a scripted
firmware interface (like the LSI scripts or the Adaptec sequencer).  As
long as the scripts are updated, it's no trouble to support multiple
data pointers and large CDBs (even the 53c700, a 1990s era chip can do
this).  I also know seagate is working on an actual single disc that
supports OSD, so I expect even the closed firmware chip sets will
eventually be updated.

> I have used only ISCSI and TCP/IP (non SCSI) OSD devices. In the devices
> that
> I have used the transitions of data where isolated - first DATA OUT then
> DATA IN.
> But an OSD device can do the following data transitions: DATA IN (user
> data) ; DATA OUT
> (set attributes data and get attributes list) ;  DATA IN (retrieved
> attributes data).

Yes, that's what I feared.  The spec implies this as well.  Behaviour
like that requires two data pointers.

James