Some plans for scsi_cmnd

Some plans for scsi_cmnd

[Matthew Wilcox]

Christoph grabbed me on IRC and we debated some of my plans for scsi_cmnd;
with his permission I'm summarising the result of that debate for posterity.
There's four main things discussed:

1. I'm going to be writing and posting patches over the next week or so
to kill all the (ab)uses of ->scsi_done in drivers.  Once that is done,
I'll post a patch that exports the midlayer's scsi_done and switch all
the drivers to calling that.  After that, I'll post another patch that
changes the prototype *and the semantics* of queuecommand; the midlayer
will no longer take the host_lock for the driver.  I'll just push the
acquisition down into queuecommand, and it'll be up to individual driver
authors to do something sensible with it then.

2. Thanks to a thinko, we also discussed the upper-layer ->done.  We think
it should be feasible to move this from the scsi_cmnd to the scsi_device
since sg doesn't use it.

3. We also discussed scsi_pointer.  It's really quite crufty, and it
gets recycled for storing all kinds of things.  The ambitious plan here
is to change the whole way scsi_cmnds are allocated.  Code is clearer
than my description ...

sym2.c:

struct sym2_cmnd {
	struct scsi_cmnd cmd;
	int Phase;
	char *data_in;
}

struct scsi_host_template sym2_template {
	.cmnd_size = sizeof(sym2_cmnd);
}

int sym2_queuecommand(struct scsi_cmnd *scp)
{
	struct sym2_cmnd *cmnd = container_of(scp, sym2_cmnd, cmd);
}

The midlayer will create a slab pool per host template for allocating
scsi_cmnd.

As I said, it's ambitious.  But it'll let us get rid of scsi_pointer
and host_scribble entirely.

4. We don't understand why sense_buffer is 96 bytes long.  mkp says that
devices are coming which need more than 96 bytes (the standard allows up
to 252).  I propose splitting the 96-byte buffer like so:

-#define SCSI_SENSE_BUFFERSIZE  96
-       unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
+       unsigned char sense_buffer_head[8];
+       unsigned char *sense_buffer_desc;

and then adding:

+int scsi_set_sense_buffer(struct scsi_cmnd *cmd, unsigned char *sense,
+                                                               int sense_len)
+{
+       int len = min(sense[7], sense_len - 8);
+       memcpy(cmd->sense_buffer_head, sense, min(8, sense_len));
+       if (len <= 0)
+               return 0;
+       cmd->sense_buffer_desc = kmalloc(len, GFP_ATOMIC);
+       if (!cmd->sense_buffer_desc)
+               return 1;
+       memcpy(cmd->sense_buffer_desc, sense + 8, len);
+       return 0;
+}
+EXPORT_SYMBOL(scsi_set_sense_buffer);

Drivers then do something like:

-                       memset(&cmd->sense_buffer, 0, sizeof(cmd->sense_buffer))
-                       memcpy(cmd->sense_buffer, cp->sns_bbuf,
-                             min(sizeof(cmd->sense_buffer),
-                                 (size_t)SYM_SNS_BBUF_LEN));
+                       scsi_set_sense_buffer(cmd, cp->sns_bbuf,
+                                                       SYM_SNS_BBUF_LEN);

or even ...

-               /* Copy Sense Data into sense buffer. */
-               memset(cp->sense_buffer, 0, sizeof(cp->sense_buffer));
-
                if (!(scsi_status & SS_SENSE_LEN_VALID))
                        break;
 
-               if (sense_len >= sizeof(cp->sense_buffer))
-                       sense_len = sizeof(cp->sense_buffer);
-
-               CMD_ACTUAL_SNSLEN(cp) = sense_len;
-               sp->request_sense_length = sense_len;
-               sp->request_sense_ptr = cp->sense_buffer;
-
-               if (sp->request_sense_length > 32)
-                       sense_len = 32;
-
-               memcpy(cp->sense_buffer, sense_data, sense_len);
-
-               sp->request_sense_ptr += sense_len;
-               sp->request_sense_length -= sense_len;
-               if (sp->request_sense_length != 0)
-                       ha->status_srb = sp;
+               scsi_set_sense_buffer(cp, sense_data, sense_len);

If any of this seems unwelcome, please say so.  It's going to be a lot of
churn (and part 4 may well take six months or more), so I'd like people
to voice objections now rather than later.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Some plans for scsi_cmnd

[Christoph Hellwig]

On Tue, Sep 25, 2007 at 07:37:33AM -0600, Matthew Wilcox wrote:
> 2. Thanks to a thinko, we also discussed the upper-layer ->done.  We think
> it should be feasible to move this from the scsi_cmnd to the scsi_device
> since sg doesn't use it.

I suspect putting it into the scsi_driver would be even better.

> 3. We also discussed scsi_pointer.  It's really quite crufty, and it
> gets recycled for storing all kinds of things.  The ambitious plan here
> is to change the whole way scsi_cmnds are allocated.  Code is clearer
> than my description ...
> 
> sym2.c:
> 
> struct sym2_cmnd {
> 	struct scsi_cmnd cmd;
> 	int Phase;
> 	char *data_in;
> }
> 
> struct scsi_host_template sym2_template {
> 	.cmnd_size = sizeof(sym2_cmnd);
> }

I'd prefer to add alloc_mnd and destroy_cmnd methods as per struct
inode.  That also allows drivers to do things like dma_pool allocations
ahead of time and not worry about needing to do this kind of allocations
from softirq context which is at least theoretically deadlockable
without emergency pool schemes.

Re: Some plans for scsi_cmnd

[Matthew Wilcox]

On Tue, Sep 25, 2007 at 02:47:50PM +0100, Christoph Hellwig wrote:
> On Tue, Sep 25, 2007 at 07:37:33AM -0600, Matthew Wilcox wrote:
> > 2. Thanks to a thinko, we also discussed the upper-layer ->done.  We think
> > it should be feasible to move this from the scsi_cmnd to the scsi_device
> > since sg doesn't use it.
> 
> I suspect putting it into the scsi_driver would be even better.

That could work, but I don't think we have a pointer from the
scsi_device (or scsi_cmnd) to the scsi_driver.

> I'd prefer to add alloc_mnd and destroy_cmnd methods as per struct
> inode.  That also allows drivers to do things like dma_pool allocations
> ahead of time and not worry about needing to do this kind of allocations
> from softirq context which is at least theoretically deadlockable
> without emergency pool schemes.

OK, we clearly weren't quite envisaging the same thing for this part.
Good job I put some pseudo-code down.  I don't have any objection to
this scheme.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Some plans for scsi_cmnd

[Boaz Harrosh]

On Tue, Sep 25 2007 at 15:37 +0200, Matthew Wilcox <matthew@wil.cx> wrote:
> Christoph grabbed me on IRC and we debated some of my plans for scsi_cmnd;
> with his permission I'm summarising the result of that debate for posterity.
> There's four main things discussed:
> 
> 1. I'm going to be writing and posting patches over the next week or so
> to kill all the (ab)uses of ->scsi_done in drivers.  Once that is done,
> I'll post a patch that exports the midlayer's scsi_done and switch all
> the drivers to calling that.  After that, I'll post another patch that
> changes the prototype *and the semantics* of queuecommand; the midlayer
> will no longer take the host_lock for the driver.  I'll just push the
> acquisition down into queuecommand, and it'll be up to individual driver
> authors to do something sensible with it then.
> 
> 2. Thanks to a thinko, we also discussed the upper-layer ->done.  We think
> it should be feasible to move this from the scsi_cmnd to the scsi_device
> since sg doesn't use it.
> 
> 3. We also discussed scsi_pointer.  It's really quite crufty, and it
> gets recycled for storing all kinds of things.  The ambitious plan here
> is to change the whole way scsi_cmnds are allocated.  Code is clearer
> than my description ...
> 
> sym2.c:
> 
> struct sym2_cmnd {
> 	struct scsi_cmnd cmd;
> 	int Phase;
> 	char *data_in;
> }
> 
> struct scsi_host_template sym2_template {
> 	.cmnd_size = sizeof(sym2_cmnd);
> }
> 
> int sym2_queuecommand(struct scsi_cmnd *scp)
> {
> 	struct sym2_cmnd *cmnd = container_of(scp, sym2_cmnd, cmd);
> }
> 
> The midlayer will create a slab pool per host template for allocating
> scsi_cmnd.
> 
I have in my Q a small variation on this principle where I wanted
to allocate bigger commands for bidi-able hosts like iscsi_tcp. So
not to pay the extra allocation per command. Above will do just fine.

> As I said, it's ambitious.  But it'll let us get rid of scsi_pointer
> and host_scribble entirely.
> 
This all is an excellent idea and you will find that in the patchset to
gdth, I have made the work of one driver a bit easier for you.

I suggest gradual work, where both Scp and host_scribble are intact
but the .cmnd_size and mechanics are available with few major drivers
moved to that. Than one kernel after that deprecating both, while 
converting lots of drivers, and 1-2 kernels after that remove them when
all drivers are converted. Don't sit on a large patchset with lots of
drivers and submit them all at once.

> 4. We don't understand why sense_buffer is 96 bytes long.  mkp says that
> devices are coming which need more than 96 bytes (the standard allows up
> to 252).  I propose splitting the 96-byte buffer like so:
> 
> -#define SCSI_SENSE_BUFFERSIZE  96
> -       unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> +       unsigned char sense_buffer_head[8];
> +       unsigned char *sense_buffer_desc;
> 
> and then adding:
> 
> +int scsi_set_sense_buffer(struct scsi_cmnd *cmd, unsigned char *sense,
> +                                                               int sense_len)
> +{
> +       int len = min(sense[7], sense_len - 8);
> +       memcpy(cmd->sense_buffer_head, sense, min(8, sense_len));
> +       if (len <= 0)
> +               return 0;
> +       cmd->sense_buffer_desc = kmalloc(len, GFP_ATOMIC);
> +       if (!cmd->sense_buffer_desc)
> +               return 1;
> +       memcpy(cmd->sense_buffer_desc, sense + 8, len);
> +       return 0;
> +}
> +EXPORT_SYMBOL(scsi_set_sense_buffer);
> 
> Drivers then do something like:
> 
> -                       memset(&cmd->sense_buffer, 0, sizeof(cmd->sense_buffer))
> -                       memcpy(cmd->sense_buffer, cp->sns_bbuf,
> -                             min(sizeof(cmd->sense_buffer),
> -                                 (size_t)SYM_SNS_BBUF_LEN));
> +                       scsi_set_sense_buffer(cmd, cp->sns_bbuf,
> +                                                       SYM_SNS_BBUF_LEN);
> 
> or even ...
> 
> -               /* Copy Sense Data into sense buffer. */
> -               memset(cp->sense_buffer, 0, sizeof(cp->sense_buffer));
> -
>                 if (!(scsi_status & SS_SENSE_LEN_VALID))
>                         break;
>  
> -               if (sense_len >= sizeof(cp->sense_buffer))
> -                       sense_len = sizeof(cp->sense_buffer);
> -
> -               CMD_ACTUAL_SNSLEN(cp) = sense_len;
> -               sp->request_sense_length = sense_len;
> -               sp->request_sense_ptr = cp->sense_buffer;
> -
> -               if (sp->request_sense_length > 32)
> -                       sense_len = 32;
> -
> -               memcpy(cp->sense_buffer, sense_data, sense_len);
> -
> -               sp->request_sense_ptr += sense_len;
> -               sp->request_sense_length -= sense_len;
> -               if (sp->request_sense_length != 0)
> -                       ha->status_srb = sp;
> +               scsi_set_sense_buffer(cp, sense_data, sense_len);
> 
Please review my patches to scsi_error and the REQUEST_SENSE paths
(James are they not going to be accepted into 2.6.24-rcx?)
I have introduced an abstract way to convert a command to point to
it's sense buffer, So drivers can now transfer data to the sense buffer
the way they do to regular IO, throw an sg_list.

Also if you are converting to pointers than please do not do the above.
struct request already has a sense pointer and length. Directly use
these. The transient first 8 bytes are not necessary, and just complicate
the code. The all sense allocation can be settled with part 3 of your mail 
above. we can widen it to be:
struct scsi_host_template sym2_template {
	.cmnd_size = sizeof(sym2_cmnd);
	.sense_size = SYM_SNS_BBUF_LEN;
	.bidi_cmnd = 1;
}
By default .sense_size will be 96 allocated from cmnd-pool and pointed
to by the struct request pointers.
Drivers that want privately allocated space can put 0 in .sense_size 
and use Christoph's alloc/destroy_cmnd to set up their own. 
Drivers should access all these members throw accessors So to be abstracted 
from all that.

> If any of this seems unwelcome, please say so.  It's going to be a lot of
> churn (and part 4 may well take six months or more), so I'd like people
> to voice objections now rather than later.
> 

RFC patches early and set up a git tree, if possible. I will help in any way 
I can also with drivers.

Boaz

Re: Some plans for scsi_cmnd

[Matthew Wilcox]

On Tue, Sep 25, 2007 at 04:51:06PM +0200, Boaz Harrosh wrote:
> > As I said, it's ambitious.  But it'll let us get rid of scsi_pointer
> > and host_scribble entirely.
> > 
> This all is an excellent idea and you will find that in the patchset to
> gdth, I have made the work of one driver a bit easier for you.

;-)

> I suggest gradual work, where both Scp and host_scribble are intact
> but the .cmnd_size and mechanics are available with few major drivers
> moved to that. Than one kernel after that deprecating both, while 
> converting lots of drivers, and 1-2 kernels after that remove them when
> all drivers are converted. Don't sit on a large patchset with lots of
> drivers and submit them all at once.

Yup, incremental work is always good.

> Please review my patches to scsi_error and the REQUEST_SENSE paths
> (James are they not going to be accepted into 2.6.24-rcx?)
> I have introduced an abstract way to convert a command to point to
> it's sense buffer, So drivers can now transfer data to the sense buffer
> the way they do to regular IO, throw an sg_list.

Brilliant.  I'd only just started looking at the sense buffer issue, so
now I can stop entirely ;-)

> RFC patches early and set up a git tree, if possible. I will help in any way 
> I can also with drivers.

I was planning on just throwing patches at this list and seeing what
sticks.  You'll see a few from me today ... looks like item 2 on the
original mail (getting rid of ->done) may be easier than we thought, and
lead to some really nice cleanups.  Of course, gdth is still the
sticking point for that ;-(

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: Some plans for scsi_cmnd

[Moore, Eric]

On Tuesday, September 25, 2007 7:38 AM, Matthew Wilcox wrote: 

> As I said, it's ambitious.  But it'll let us get rid of scsi_pointer
> and host_scribble entirely.
> 


Are you serious about removing the host_scribble?     In fusion we
currently are hanging our per request message frame pointer there.
Its used for two reasons:

(1)  Old fibre channel firmware bug.  The bug is the same message frame
completed twice.  1st completion is okay and the scsi_cmd is reallocated
to some other IO.  Meanwhile the driver receives a double completion of
the same message frame, and the driver attempts to complete the
reallocated scsi_cmd to some other IO.  The sanity check on
host_scribble avoids this.

(2)  error recovery threads decides to complete a scsi_cmd before its
been returned back by the scsi lld, and the scsi_cmd is reused for some
other IO.  Meanwhile fusion firmware finally  decides it wants to
command the command, then the driver is completing the  reallocated
scsi_cmd that is mapped to some other IO.  Again, host_scribble sanity
check makes sure we finish off the scsi_cmd that its intended for.


Thus said, someone upgrades to newer FC firmware, they will not have
issue #1.    

Regarding issue #2, if eh threads allow scsi lld to cleanup ts own scsi
cmds, then we shouldn't need this.


Eric

Re: Some plans for scsi_cmnd

[Matthew Wilcox]

On Fri, Sep 28, 2007 at 04:15:56PM -0600, Moore, Eric wrote:
> On Tuesday, September 25, 2007 7:38 AM, Matthew Wilcox wrote: 
> 
> > As I said, it's ambitious.  But it'll let us get rid of scsi_pointer
> > and host_scribble entirely.
> 
> Are you serious about removing the host_scribble?     In fusion we
> currently are hanging our per request message frame pointer there.

Yes.  But you snipped the explanation about why we wouldn't need it any
more.  You'd not be allocating scsi_cmnds, you'd be allocating a

struct fusion_scsi_cmnd {
	struct scsi_cmnd scmd;
	... everything currently in host_scribble ...
}

Of course, if you need the message frame pointer to be separated from
the command, you can just do:

struct fusion_scsi_cmnd {
	struct scsi_cmnd scmd;
	struct message_frame_pointer *host_scribble;
}

Or call it something more useful than host_scribble ;-)

> Its used for two reasons:
> 
> (1)  Old fibre channel firmware bug.  The bug is the same message frame
> completed twice.  1st completion is okay and the scsi_cmd is reallocated
> to some other IO.  Meanwhile the driver receives a double completion of
> the same message frame, and the driver attempts to complete the
> reallocated scsi_cmd to some other IO.  The sanity check on
> host_scribble avoids this.
> 
> (2)  error recovery threads decides to complete a scsi_cmd before its
> been returned back by the scsi lld, and the scsi_cmd is reused for some
> other IO.  Meanwhile fusion firmware finally  decides it wants to
> command the command, then the driver is completing the  reallocated
> scsi_cmd that is mapped to some other IO.  Again, host_scribble sanity
> check makes sure we finish off the scsi_cmd that its intended for.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."