Some plans for scsi_cmnd

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

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

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.

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

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

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.

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.

struct scsi_host_template sym2_template {
	.cmnd_size = sizeof(sym2_cmnd);
	.sense_size = SYM_SNS_BBUF_LEN;
	.bidi_cmnd = 1;
}

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

Boaz

;-)

Yup, incremental work is always good.

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

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

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

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

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