use scatter lists for all block pc requests and simplify hw handlers

The goal is next convert the scsi 'special' requests to use these functions, so scsi will be able to use block layer functions for scatter lists setup for all requests. And then hopefully one day we will not need those stinking "if (sc->use_sg)" paths all over our scsi drivers.

Quoted 4 lines · Mike Christie

The goal is next convert the scsi 'special' requests to use these functions, so scsi will be able to use block layer functions for scatter lists setup for all requests. And then hopefully one day we will not need those stinking "if (sc->use_sg)" paths all over our scsi drivers.

Here's the proof of concept for this one. It converts scsi_wait_req to do correct REQ_BLOCK_PC submission (and works nicely in my setup).

The final goal should be to eliminate struct scsi_request, but that can't be done until the character submission paths of sg and st are also modified.

There's some loss of functionality to this: retries are no longer controllable (except by setting REQ_FASTFAIL) and the wait_req API needs to be altered, but it looks very nice.

inlined below is a patch built over yours + my patch set (I put all my patches here http://www.cs.wisc.edu/~michaelc/block/use-sg/v3/) that converts scsi_scan.c and removes its scsi_request usage. I cheated and added a new function that is basically your scsi_wait_req but it uses the block layer's blk_execute_rq function instead of blk_insert_request. Also to send block pc commands at scan time without the special bit set I had to modify the scsi_prep_fn. This is just a RFC/test-patch so oh well. There is the larger problem of handling queue limits to handle.

Quoted 4 lines

There's some loss of functionality to this: retries are no longer controllable (except by setting REQ_FASTFAIL) and the wait_req API needs to be altered, but it looks very nice.

While the focus is in this area, I'd like to mention multipath tools userland checkers will need to insert SG request on queue head (Ed Gaggin will explain this at OLS). I guess the API is lacking there, and would like to know if this feature is on your TODOs ...

Quoted 4 lines · christophe varoqui

While the focus is in this area, I'd like to mention multipath tools userland checkers will need to insert SG request on queue head (Ed Gaggin will explain this at OLS). I guess the API is lacking there, and would like to know if this feature is on your TODOs ...

It's on the TODO list. It's just not done yet. Essentially we need to be able to set BW_RW_FAILFAST from user-space for SGIO.

This is Novell bugzilla #81694 and RHAT bugzilla #154436.

Quoted 3 lines

It's on the TODO list. It's just not done yet. Essentially we need to be able to set BW_RW_FAILFAST from user-space for SGIO.

Isn't this a seperate issue? It looks like the RHAT bugzilla asks to be able to insert directly at the head of the queue so you do you not have to wait for possibly a hundred IO to complete before the test IO. Setting failfast just means in case of en error it will be returned faster (no wasting time retying).

(quoted diff) +2 -0

+int scsi_execute_req(struct scsi_device *sdev, unsigned char *cmd,
+                    int data_direction, void *buffer, unsigned

bufflen,
+ unsigned char *sense, int timeout, int retries)

+ if (!req)

Finally, there's coming up with a replacement API for scsi_do_req that returns via the end_io callback ... since that doesn't do a wait/wake, perhaps this should be the core API upon which the others are built?

Quoted 3 lines · James Bottomley

Finally, there's coming up with a replacement API for scsi_do_req that returns via the end_io callback ... since that doesn't do a wait/wake, perhaps this should be the core API upon which the others are built?

OK, well, the API is easy ... it's attached. Converting sg and st to use it is quite another matter. sg in particular is a nasty tangle when it comes to doing its own sg mapping, which the block layer will now do for it.

James

drivers/scsi/scsi_lib.c +17 -0

@@ -177,6 +177,23 @@ int scsi_queue_insert(struct scsi_cmnd *
 	return 0;
 }
 
+struct scsi_do_req_cb {
+	void (*done)(void *, int);
+	void *data;
+};
+
+static void scsi_do_req_end_io(struct request *req)
+{
+	int result = req->errors;
+	struct scsi_do_req_cb *cb = req->end_io_data;
+	
+	BUG_ON(cb == NULL);
+	if (result)
+		result |= DRIVER_ERROR << 24;
+	blk_put_request(req);
+	cb->done(cb->data, result);
+	kfree(cb);
+}
 /*
  * Function:    scsi_do_req
  *

@@ -203,38 +220,49 @@ int scsi_queue_insert(struct scsi_cmnd *
  *		now inject requests on the *head* of the device queue
  *		rather than the tail.

There is no max_length on sense_buffer. How does one know how much, if any, sense data was written (returned).

resid is a very useful return parameter.

Perhaps the *done() callback should have
two more arguments (and scsi_do_command()
one more argument). And 'buffer' and 'bufflen' have not changed in name but I assume they have in nature.

Additionally perhaps it is time to start thinking about a clean pass through. Something that can pass a packet command (like sg can at the moment) but to a scsi_host (or a transport host). An additional argument would be needed to pass addressing information to the LLD. I'm thinking about the SAS Serial Management Protocol (SMP) that has nothing to do with the block layer or the SCSI mid level. We want to bind to a SAS host (initiator port) and supply a SAS address (of the SMP target which is most likely an expander).

Well ... while I've got your attention, the only complexity I see to converting sg is the hd->iovec_count != 0 case, which is doing vectored SG from user land.

This can be implemented (basically you do multiple bio attachment to a request), and probably in the block layer (since block SG_IO would then pick up the capability).

How important is this functionality? Are there any applications using it?

Quoted 3 lines

There is no max_length on sense_buffer. How does one know how much, if any, sense data was written (returned).

Well, given the way most initiators work we can't do variable length sense buffer sizes; it's limited by SCSI_SENSE_BUFFERSIZE, which seems to be a good fixed value assumption. The reason we don't have this is because the block layer doesn't. It also assumes SCSI_SENSE_BUFFERSIZE is the correct maximum.

Quoted 1 line

resid is a very useful return parameter.

Yes, I spotted that too ... already in my version II ...

Quoted 5 lines

Perhaps the *done() callback should have
two more arguments (and scsi_do_command()
one more argument). And 'buffer' and 'bufflen' have not changed in name but I assume they have in nature.

Quoted 10 lines

Additionally perhaps it is time to start thinking about a clean pass through. Something that can pass a packet command (like sg can at the moment) but to a scsi_host (or a transport host). An additional argument would be needed to pass addressing information to the LLD. I'm thinking about the SAS Serial Management Protocol (SMP) that has nothing to do with the block layer or the SCSI mid level. We want to bind to a SAS host (initiator port) and supply a SAS address (of the SMP target which is most likely an expander).

Well, pretty much the block layer SG_IO is a clean passthrough.

direction I think it makes most sense to move in is towards services provided by the block layer (even if that means additions to it) rather than away from them. Even for arbitrary SAS expanders, we can detect and bind to them; once that's done we have a connection and a management application can read and set sysfs parameters for the object.

Quoted 2 lines · James Bottomley

Well ... while I've got your attention, the only complexity I see to converting sg is the hd->iovec_count != 0 case, which is doing

Quoted 3 lines

SG from user land.

This can be implemented (basically you do multiple bio attachment to

Quoted 1 line

request), and probably in the block layer (since block SG_IO would

Quoted 3 lines

pick up the capability).

How important is this functionality? Are there any applications

Quoted 1 line

it?

I don't think it is important and I'm not aware of any real world applications that are using it. Naturally, I will find out if that is actually true after it gets removed.

My company (Cybernetics) sells several products that run embedded Linux which use the userspace scatter-gather capability of the SG driver. They are still running the 2.4 kernel series, but we will probably switch to 2.6 at some point.

Quoted 1 line

My company (Cybernetics) sells several products that run embedded

Quoted 3 lines

which use the userspace scatter-gather capability of the SG driver. They are still running the 2.4 kernel series, but we will probably switch to 2.6 at some point.

What do they actually use it for? As in why is the data scattered

in the virtual address space?

Quoted 1 line

Quoted 1 line

My company (Cybernetics) sells several products that run embedded

Linux

Quoted 5 lines

Quoted 3 lines

which use the userspace scatter-gather capability of the SG driver. They are still running the 2.4 kernel series, but we will probably switch to 2.6 at some point.

What do they actually use it for? As in why is the data scattered

even

Quoted 1 line

in the virtual address space?

One of our products is a RAID-like controller for SCSI tape drives. Parallel SCSI or iSCSI in via a target-mode SCSI driver and parallel SCSI, iSCSI, or FC out via the SG driver, with various functions in between for data buffering, emulation, striping, mirroring, etc. The program that controls everything has a complex internal buffering scheme that uses userspace scatter-gather for the main read/write I/O path to the tape drives.

I don't think it is important and I'm not aware of any real world applications that are using it. Naturally, I will find out if that is actually true after it gets removed.

I believe that I am guilty for SCSI_SENSE_BUFFERSIZE. But implementation details shouldn't necessarily limit new interface functions. Again OSD could be a problem here as it uses descriptor sense format with larger descriptors (than SPC-3). Perhaps Liran might comment on whether the current 96 bytes is sufficient. [According to SPC-3 there is still a limit of 252 bytes.]

Also scsi_do_command() could drop the direction argument and have an "in" pair (pointer and length) and an "out" pair. Then it could cope with OSD and advanced SBC-2 commands.

Depending on the driver which controls the device node that has been opened, there are various policy decisions to maneouver (e.g. sr and st require O_NONBLOCK on open() otherwise they will wait until media is loaded, making the SCSI Test Unit Ready command pointless). Also

there are state machines in some drivers, that can either be confused by the pass through or vice versa (e.g. try using the sg_prevent utility in sg3_utils via a sr device node, then
try again with a sg device node). Should maximum transfer size limits that apply to the READ/WRITE SBC commands also apply to READ/WRITE_BUFFER SPC commands?

The SAS SMP protocol is for discovering what is out there. In the extreme case no block device has been found. There is just a SAS HBA on a PCI bus with sysfs information telling us about some phys and what they are attached to. If the attached device type is an expander that implements a SMP target then we need to issue SMP frames (commands) through a local phy to the SAS address of the expander. I'm not sure we need scatter-gather, command merging, tagged queues or pseudo block devices to accomplish this.

Quoted 4 lines · Douglas Gilbert

I don't think it is important and I'm not aware of any real world applications that are using it. Naturally, I will find out if that is actually true after it gets removed.

Heh, true. OK, let's do this: I'll remove it from the sg driver and make sg use the do_command (or it's successor) interface. If an actual user for the iovecs does turn up, I'll add it to the block layer (it's a fairly well understood multi-bio request setup) and make sg and also the block layer SG_IO use it.

Why multi-bio? No one should ever have to build a request with multiple bio's in one go, that's pointless. The only reason multi-bio requests exist is because of the file systems not submitting big extents in one submission. The whole io path would be faster and simpler were it not for multi-bio requests :-)

Quoted 5 lines · Jens Axboe

Why multi-bio? No one should ever have to build a request with multiple bio's in one go, that's pointless. The only reason multi-bio requests exist is because of the file systems not submitting big extents in one submission. The whole io path would be faster and simpler were it not for multi-bio requests :-)

OK, OK, sorry thinko ... I meant multi-biovec (which can be a single bio) ... however, I'm not the only one who keeps being confused by this ...

Incidentally, do I take it you're happy with all of this and I can take (at least the block pieces) through the SCSI tree?

Quoted 2 lines · James Bottomley

Here's the proof of concept for this one. It converts scsi_wait_req to do correct REQ_BLOCK_PC submission (and works nicely in my setup).

Quoted 3 lines

There's some loss of functionality to this: retries are no longer controllable (except by setting REQ_FASTFAIL) and the wait_req API needs to be altered, but it looks very nice.

Quoted 4 lines · James Bottomley

Quoted 3 lines

Here's the proof of concept for this one. It converts scsi_wait_req to do correct REQ_BLOCK_PC submission (and works nicely in my setup).

That is nice ... I didn't realize Mike's change allowed that.

Quoted 3 lines

Quoted 3 lines

There's some loss of functionality to this: retries are no longer controllable (except by setting REQ_FASTFAIL) and the wait_req API needs to be altered, but it looks very nice.

Mike your patch did not replace the retries.

Why can't the retries be passed down via a new blk req->retries?

Quoting Patrick Mansfield <[email protected]>:

Quoted 2 lines

Why can't the retries be passed down via a new blk req->retries?

Is that all that is needed for dm-multipath? I thought this would be a good time to add some code to allow the finer grained control of retries that could also be used by dm and md. For example do you only want to retry certain failures X numbeer of times?

Quoted 6 lines · Michael Christie

Quoting Patrick Mansfield <[email protected]>:

Quoted 2 lines

Why can't the retries be passed down via a new blk req->retries?

Quoted 4 lines

Is that all that is needed for dm-multipath? I thought this would be a good time to add some code to allow the finer grained control of retries that could also be used by dm and md. For example do you only want to retry certain failures X numbeer of times?

I was looking/thinking about the scsi_scan retries not multipath.

Quoted 17 lines · Patrick Mansfield

Quoted 13 lines · Michael Christie

Quoting Patrick Mansfield <[email protected]>:

Quoted 2 lines

Why can't the retries be passed down via a new blk req->retries?

Is that all that is needed for dm-multipath? I thought this would be a good time
to add some code to allow the finer grained control of retries that could also
be used by dm and md. For example do you only want to retry certain failures X
numbeer of times?

Quoted 1 line

I was looking/thinking about the scsi_scan retries not multipath.

And I was hoping the same code/framework can be used for scsi, DM, MD, SG_IO and all other block layer drivers.

Quoted 18 lines · Mike Christie

Quoted 17 lines · Patrick Mansfield

Quoted 13 lines · Michael Christie

Quoting Patrick Mansfield <[email protected]>:

Quoted 2 lines

Why can't the retries be passed down via a new blk req->retries?

Is that all that is needed for dm-multipath? I thought this would be a good time
to add some code to allow the finer grained control of retries that could also
be used by dm and md. For example do you only want to retry certain failures X
numbeer of times?

Quoted 4 lines

Quoted 1 line

I was looking/thinking about the scsi_scan retries not multipath.

And I was hoping the same code/framework can be used for scsi, DM, MD, SG_IO and all other block layer drivers.

So use a req->retries instead of the fail fast flag?

Quoted 1 line

So use a req->retries instead of the fail fast flag?

Would be fine. Some drivers are messing with ->errors for that anyways, so would be nice to clean that up.

It would need to be done _very_ carefully though!

Quoted 5 lines · Jens Axboe

Quoted 1 line

So use a req->retries instead of the fail fast flag?

Would be fine. Some drivers are messing with ->errors for that anyways, so would be nice to clean that up.

Quoted 1 line

It would need to be done _very_ carefully though!

Actually, I think there's still a need for both. Fail fast implies more than simply no retry. In the SCSI case it should eventually mean fail the command before we begin transport recovery (at the moment, we simply use the command in transport recovery, however long it takes, and then return the failure).

drivers/scsi/scsi_lib.c +9 -0

+		if (req->rq_disk) {
+			drv = *(struct scsi_driver **)req->rq_disk->private_data;
+			if (unlikely(!drv->init_command(cmd))) {
+				scsi_release_buffers(cmd);
+				scsi_put_command(cmd);
+				return BLKPREP_KILL;
+			}
+		} else {
+			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would make life easier for the callers.

Quoted 14 lines

drivers/scsi/scsi_lib.c +14 -0

+		if (req->rq_disk) {
+			drv = *(struct scsi_driver **)req->rq_disk->private_data;
+			if (unlikely(!drv->init_command(cmd))) {
+				scsi_release_buffers(cmd);
+				scsi_put_command(cmd);
+				return BLKPREP_KILL;
+			}
+		} else {
+			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
+			if (rq_data_dir(req) == WRITE)
+				cmd->sc_data_direction = DMA_TO_DEVICE;
+			else if (req->data_len)
+				cmd->sc_data_direction = DMA_FROM_DEVICE;
+			else

drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1176

Quoted 5 lines

drivers/scsi/scsi_lib.c +5 -0

+				cmd->sc_data_direction = DMA_NONE;
+			
+			cmd->transfersize = req->data_len;
+			cmd->allowed = 3;
+			cmd->timeout_per_command = req->timeout;

most of this could probably be done in the midlayer always instead of the upper drivers.

Quoted 2 lines

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would make

life

Quoted 1 line

easier for the callers.

Yes ... and it should probably do bio bouncing as well, since there's nothing special we have to do on completion to clean it up.

I also think we might need a blk_rq_kern_iovec call that would take a vector of user I/O's and map it to a multiple bio request. This would

Quoting James Bottomley <[email protected]>:

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would

make

Quoted 5 lines

life

Quoted 1 line

easier for the callers.

Yes ... and it should probably do bio bouncing as well, since there's nothing special we have to do on completion to clean it up.

ok. I just made it like the existing blk_rq_map_user which made the caller do
those things.

Quoted 3 lines

I also think we might need a blk_rq_kern_iovec call that would take a vector of user I/O's and map it to a multiple bio request. This would

Does it need to be a multiple bio request? A single bio should be able to handle
a request's segments and sectors limits.

Will the user assure that the iovec will fit in a single request to handle a case where the iovec is greater than the phys or hw segment limits though?

Quoting Michael Christie <[email protected]>:

Quoted 24 lines

Quoting James Bottomley <[email protected]>:

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would

make

Quoted 5 lines

life

Quoted 1 line

easier for the callers.

Yes ... and it should probably do bio bouncing as well, since there's nothing special we have to do on completion to clean it up.

ok. I just made it like the existing blk_rq_map_user which made the caller do
those things.

Quoted 3 lines

I also think we might need a blk_rq_kern_iovec call that would take a vector of user I/O's and map it to a multiple bio request. This would

Does it need to be a multiple bio request? A single bio should be able to handle
a request's segments and sectors limits.

Quoted 3 lines

Will the user assure that the iovec will fit in a single request to handle a case where the iovec is greater than the phys or hw segment limits though?

scsi_do/wait_req could do the checking and submit mutliple requests for sg I mean.

No, it can't do that. If the user submits one SCSI command, it must result in one SCSI command to the device. Otherwise the effect is not what the user wants. (You can split a multiple block read/write but this does not apply to all commands.)

Michael's question is important. The number of sg segments a HBA supports determines the maximum SCSI data size. In some cases (e.g., tape reads and writes with multimegabyte blocks) using page size (e.g., 4 kB) segments does not allow large enough data size. One solution has been to have a kernel space buffer that consists of segments spanning several pages. As far as I know, the current bio code requires page size segments. It is possible to use chained bios with multimegabyte buffers but the user should be sure that the split segments will be merged before the request reaches the HBA so that the request fits the HBA sg segment limit.

Quoting James Bottomley <[email protected]>:

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would make

Quoted 5 lines

life

Quoted 1 line

easier for the callers.

Yes ... and it should probably do bio bouncing as well, since there's nothing special we have to do on completion to clean it up.

ok. I just made it like the existing blk_rq_map_user which made the caller do those things.

Quoted 2 lines

I also think we might need a blk_rq_kern_iovec call that would take a vector of user I/O's and map it to a multiple bio request. This would

Does it need to be a multiple bio request? A single bio should be able to handle a request's segments and sectors limits.

Will the user assure that the iovec will fit in a single request to handle a case where the iovec is greater than the phys or hw segment limits though?

Quoted 15 lines · Michael Christie

Quoting James Bottomley <[email protected]>:

Quoted 11 lines · Christoph Hellwig

Quoted 3 lines

shouldn't blk_rq_map_kern handle a 0 buffer and do nothing more than blk_get_request? It's not exactly a criticial fastpath and that would make

life

Quoted 1 line

easier for the callers.

Yes ... and it should probably do bio bouncing as well, since there's nothing special we have to do on completion to clean it up.

Quoted 2 lines

ok. I just made it like the existing blk_rq_map_user which made the caller do those things.

I agree with Christoph, makes it nicer for blk_rq_map_user as well. I'll change the latter if you fix the former.

The following patches should enable the use of scatter lists for all REQ_BLOCK_PC requests and cleanup some code duplication or dangerous memory allocations in the dm-multipath hw handlers and block/scsi_ioctl.c.

REQ_BLOCK_PC scatter list usage only required converting the old sg_scsi_ioctl code to do bio backed requests since the current block layer SG_IO code will always use requests with bios. But while converting the old ioctl and removing some dangerous (GFP_KERNEL in failover path) memory allocations from a dm-multipath hw_handler (and while updating the LSI one) I was able to seperate some common code into two new functions: blk_rq_map_kern() and bio_map_kern. These functions are similar to their blk/bio*user cousins where they allocate requests and bios and setup the data pointers except they work on kernel buffers.

The goal is next convert the scsi 'special' requests to use these functions, so scsi will be able to use block layer functions for scatter lists setup for all requests. And then hopefully one day we will not need those stinking "if (sc->use_sg)" paths all over our scsi drivers.

I've slowly been doing the same thing in other places in the kernel and this bit has been talked about between James and I for at least a year or two.

Quoted 3 lines · Jens Axboe

I've slowly been doing the same thing in other places in the kernel and this bit has been talked about between James and I for at least a year or two.

Yes, it's long been a dream of mine to eliminate at least struct scsi_request and just use block struct request for everything in the SCSI layer.

The most pleasing aspect of this will be getting rid of the dma mapping duplications in st and sg.

A "char_uld" library that st, sg, ch and the proposed OSD ULDs could tap into, would cut the duplication. As you can see, Kai and I have shared implementations in this area but had no mechanism for sharing the actual code.

An idea I had was to flag what mechanism inserted a command onto a request queue and when a blk_pc_request() follows a blk_pc_request() then use FIFO order for the second one.

Also, I really don't think OSD should be a character device. It's definitely a block device, it just happens to have a two dimensional address space instead of a one dimensional one.

Really, I think it's cleaner for head or tail insertion to be handled at the time the request is generated. However, the block queues of character taps are special; we certainly don't need all the elevator merging machinery, so perhaps we should have a way of setting them up as noop elevator?