support for bidirectional transfers and variable length CDBs

Hi,

Attached are patches for linux-2.6.18, open-iscsi (2.0-711), and ibm-osd-initiator (v3.1.1), sketching a proposal for supporting bi-directional
scsi commands and variable length CDBs. The motivation behind this is having an OSD stack in the kernel for the Object-based pNFS layout driver in the linux kernel that we're working on with CITI.
(part of NFSv4.1, see also
http://www.ietf.org/internet-drafts/draft-ietf-nfsv4-pnfs-obj-02.txt)

The core changes we propose in the scsi layer to support bidi are:
- adding struct scsi_cmnd_sgl for holding a mapping of the caller
buffers as
<use_sg, sglist_len, request_buffer, request_bufflen, request>

- using it in struct scsi_cmnd to map the read buffer of
bidirectional commands.
for backward compatibility and to keep this patch minimalistic we
kept the mapping
of the uni-directional buffers as well as the write buffers of
bi-directional transfers intact;
in the future I think it will be cleaner to hold these in a
scsi_cmnd_sgl of their own.

- allocate another struct request when executing a bidirectional
command and use it to map the bidi read
buffer (or sglist). keep a pointer to it in the scsi_io_context.

Core changes proposed to support variable length CDBs:

- API crafted in the spirit of scsi_execute_async and is a super set of scsi_execute and scsi_execute_async (i.e. the varlen cdb and
bidi_read_buff
are optional)

- use two scsi_cmnd_sgl's in struct scsi_cmnd to map uni/bidi_write
buffers and bidi_read buffers
respectively (or alternatively use one to map write buffers and the
other to map read buffers, this
doesn't matter much id everybody uses scsi_get_{in,out,uni}_buff()
rather than dereferencing
a scsi_cmnd pointer to get to the fields.

- support bidi in struct request. This will save allocating yet
another request for mapping the
bidi_read buffers

Quoted 9 lines · Benny Halevy

Hi,

Attached are patches for linux-2.6.18, open-iscsi (2.0-711), and ibm-osd-initiator (v3.1.1), sketching a proposal for supporting bi-directional scsi commands and variable length CDBs. The motivation behind this is
having an OSD stack in the kernel for the Object-based pNFS layout driver
in the linux kernel that we're working on with CITI.
(part of NFSv4.1, see also http://www.ietf.org/internet-drafts/draft-ietf-nfsv4-pnfs-obj-02.txt)

Offtopic note: unless you opensource all your filesystem code and merge into the kernel there's not chance we'll put in nfs hooks for you. This doesn't affect the scsi bits though as we plan to have bidi commands for other reasons though (which doesn't mean it's legal to use for binary modules either)

Quoted 6 lines

The core changes we propose in the scsi layer to support bidi are:
- adding struct scsi_cmnd_sgl for holding a mapping of the caller
buffers as
<use_sg, sglist_len, request_buffer, request_bufflen, request>

I would rather call this scsi_data_buffer, but that's just a smallish thing. Having this as a separate stuct is a lot nicer than my previous proposal, though.

Quoted 10 lines

- using it in struct scsi_cmnd to map the read buffer of
bidirectional commands.
for backward compatibility and to keep this patch minimalistic we
kept the mapping
of the uni-directional buffers as well as the write buffers of
bi-directional transfers intact;
in the future I think it will be cleaner to hold these in a
scsi_cmnd_sgl of their own.

This on the other hand is not very nice. We should put an in and and an out scsi_data_buffer in struct scsi_cmnd. As we need to revist this part of the scsi midlayer <-> LLDD interface anyway we plan to handle this change as part of the bigger overall transition. Here are the helpers I had envisioned at storage summit in the version where they still translate to the currently existing code, but can be nicely adopted to bidi-capable versions with two sg lists:

size_t scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)

{
	ssize_t nseg = 0;

	if (cmd->use_sg) {
		struct scatterlist *sg =
			(struct scatterlist *) cmd->request_buffer;

		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
		if (unlikely(!nseg))
			return -ENOMEM;
	}

	return nseg;
}

void scsi_dma_unmap(struct device *dev, struct scsi_cmnd *cmd)

{
	if (cmd->use_sg) {
		struct scatterlist *sg =
			(struct scatterlist *) cmd->request_buffer;
		dma_unmap_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
	}
}

and something similar for pio / virtual drivers. Note that the above doesn't handle non-sg commands - we still have one user of them in the error handler, but I'll finally submit the patch to remove it soon. Also note that actually bidi-capable driver still will have to opencode the map and know about the two data buffers. I expect driver actually handling bidi commands to be the absolute minority, though.

Quoted 5 lines

- allocate another struct request when executing a bidirectional
command and use it to map the bidi read
buffer (or sglist). keep a pointer to it in the scsi_io_context.

That's also not the way to go ahead - struct request should support bidi commands the same way scsi should. I'd rather finish the scsi bits first, and until that is done your approach might be fine for (out of tree) debugging.

Quoted 3 lines

Core changes proposed to support variable length CDBs:

This should be an entirely separate patch. (As should your updates to the list of scsi device types and the typo fix in a comment in your patch) I haven't looked at this code in detail but it looks rather messy already ;-) I'd prefer if we could postponed implementing this properly until we've landed bidi support.

Quoted 6 lines

- API crafted in the spirit of scsi_execute_async and is a super set of scsi_execute and scsi_execute_async (i.e. the varlen cdb and
bidi_read_buff
are optional)

We might aswell change scsi_execute_async to just support bidi and varlen commands in the end. There's very few user and having yet another API to submit scsi commands doesn't sound very helpfull.

Quoted 9 lines

- use two scsi_cmnd_sgl's in struct scsi_cmnd to map uni/bidi_write
buffers and bidi_read buffers
respectively (or alternatively use one to map write buffers and the
other to map read buffers, this
doesn't matter much id everybody uses scsi_get_{in,out,uni}_buff()
rather than dereferencing
a scsi_cmnd pointer to get to the fields.

Quoted 4 lines

- support bidi in struct request. This will save allocating yet
another request for mapping the
bidi_read buffers

As per comments above I think we should do this from the beginning.

As for the further todo I'd suggest the following:

- I submit the patch to get rid of the remaining non-sg I/O ASAP
- you submit your patch to add the osd type and the addition device type descriptions
- you submit your patch that fixes the scsi_execute comment typo
- you submit your patch to add the VARLEN_CDB opcode
- you start converting as many as possible drivers to the above wrapper API (I already had a big FC card vendor signed up for this, but no updates so far..)

And then once we have almost all direct references to the request_buffer & friends gone you submit a new bidi patch based on the comments in this mail.

Thanks for working on this.

Quoted 2 lines

though.

I'm not sure I understand exactly when these helpers are to be used. Can you point me at more info please?
Does this solve the dependency of scsi on struct request for buffer mapping? (i.e. will this be used in the scsi_init_io() path instead of calling blk_rq_map_kern()
or the calls to the block layer in scsi_merge_bio()?)

Quoted 4 lines

We might aswell change scsi_execute_async to just support bidi and varlen commands in the end. There's very few user and having yet another API to submit scsi commands doesn't sound very helpfull.

cool. and what about scsi_execute? are you ok with changing it too or would you
rather add a new synchronous API call for bidi. The reason I'm asking is that
it doesn't currently allocate a scsi_io_context and therefore it's more efficient
than the async api.

Quoted 6 lines

- you submit your patch that fixes the scsi_execute comment typo
- you submit your patch to add the VARLEN_CDB opcode
- you start converting as many as possible drivers to the above wrapper API (I already had a big FC card vendor signed up for this, but no updates so far..)

fine with me, if ok with them too, please send them my way so they can send us whatever they've done already.

This should be an entirely separate patch. (As should your updates to the list of scsi device types and the typo fix in a comment in your patch)

I haven't looked at this code in detail but it looks rather messy already ;-) I'd prefer if we could postponed implementing this properly until we've landed bidi support.

Quoted 5 lines

Quoted 5 lines

- you submit your patch that fixes the scsi_execute comment typo
- you submit your patch to add the VARLEN_CDB opcode
- you start converting as many as possible drivers to the above wrapper API (I already had a big FC card vendor signed up for this, but no updates so far..)