Date:	Mon, 22 Jan 2007 01:04:50 +0200
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Jens Axboe <jens.axboe@...cle.com>,
	Christoph Hellwig <hch@...radead.org>,
	Mike Christie <michaelc@...wisc.edu>
CC:	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	open-iscsi@...glegroups.com, Daniel.E.Messinger@...gate.com,
	Liran Schour <LIRANS@...ibm.com>,
	Benny Halevy <bhalevy@...asas.com>
Subject: [RFC 0/6] bidi support: block, SCSI, and iSCSI layers

Following are 6 (large) patches that introduce support
for bidirectional requests in the kernel.
Since all this is going to interfere with everyone's
work, let us all comment on the implementation, naming,
and future directions. (or forever hold your peace).

The submitted work is against linux-2.6-block tree of
2007/01/15. It compiles with make allmodconfig in both
i386 and x86_64, and it boots and runs on my x86_64
test machines. The target kernel is able to compile a
Linux-kernel, burn and read cd, and pass iSCSI and OSD
tests. OSD tests are the ones who exercise BIDI I/O.

Patches summary:
1. data direction
	- add support for enum dma_data_direction in struct request
	  in preparation to full bidi support.
	- removed rq_data_dir() and added other APIs for querying request's direction.
	- fix usage of rq_data_dir() and peeking at req->cmd_flags & REQ_RW to using
	  new api.
	- clean-up bad usage of DMA_BIDIRECTIONAL and bzero of "black" requests,
	  to use the new blk_rq_init_unqueued_req()

2. request_io_part
	- extract io related fields in struct request into struct request_io_part
	  in preparation to full bidi support.
	- use new API for accessing the new sub-structure

3. bidi request
	- add one more request_io_part member for bidi support in struct request.
	- add block layer API functions for mapping and accessing bidi data buffers
	  and for ending a block request as a whole (end_that_request_block())

4. bidi-scsi
	- add bidi support at the scsi layer
	- new scsi api functions to generate bidi capable requests
	  (scsi_execute_bidi{,_async})
	- new scsi api for accessing scsi_cmnd_buff (similar to request_io_part
	  but not yet implemented as a sub-structure)

5. varlen cdb
	- add support for variable length (or just longer than 16 bytes) SCSI CDBs
	- add scsi device type for OSD devices (will be separated in actual patchset)

6. iscsi bidi varlen
	- add support for bidi and variable length commands at the iscsi layer
	  for the tcp transport. (will also be separated and sent through the open-iscsi project)

--------------------------------------------------------------------------------------------
Developer comments: (long, can be skipped)
1. This part borrows from struct scsi_cmnd use of an enum dma_data_direction member, to keep
everything the same.

1.A. It could be done in a more backward compatible way but it was a good chance for some clean-up
  (see the mess with BIO flags that used to be the same but no longer are). I also believe
  that two ways to describe the same thing will always come to haunt you later.

1.B. Speaking of which, the PCI_DMA_XXX constants are a pain being the same constants but #defined and
  untyped. It confused me in my bidi-bug hunting and it seems I'm not the only one.
  I would suggest boldly inverting enum dma_data_direction from its active-low DMA logic and
  change its name to just enum data_direction. Then, define the PCI_DMA_XXX constants as a new enum,
  and accept that new enum at all the dma_XXX mapping APIs, instead of the current u32. Note
  that this is cardinal now because bidirectional means __different_things__ in struct request
  and in DMA (or memory) mapping. the first means bidirectional access to __same__ buffer, the later
  means __two_sets__ of buffers at one request. (each buffer mapped for uni-directional access).

2. Separation of IO members into two sub structures: There are 2 possible approaches here and each approach
can have a few implementations.

2.A. First approach is to keep everything backward compatible and have a bidi sub-structure on the side.
  This approach can be seen in the attached patch for bidi support in SCSI layer. It keeps the patch to
  a minimum but is hell on readability & maintenance. It also puts a performance penalty on users like iscsi
  who wants to use a generic bidi-api.

2.B. Second approach is what is seen here. Separate all I/O members to a sub-structure, craft an
  API to access each part and a UNI api that wants to access buffers knowing they are uni-
  directional.

3.C. The second approach can be implemented as I did it: one member for uni/bidi-write and second
  for bidi-read or alternatively, using one sub-structure for "read" and another for "write". I hope
  I have built it in  such a way that this choice is merely an "implementation detail" only concerning
  the block layer and hidden behind proper access functions.

3.D. I have currently decided on the first approach for performance reasons, since 99.99% of kernel
  is uni-directional (only exception is proposed iscsi). What will be in the future? I'm not sure,
  If most bus protocols are like iSCSI (and SCSI) where there is a clear separation, and concurrency,
  between write and read states. then second approach will be the logical choice. Or maybe it is all
  exact same programming safe the toggle of a direction bit in a register, then the first approach
  wins. In any way the second approach needs a code change.
Boaz, I don't know what to do with this paragraph... I'm not sure I understand what you mean here...

3.E. One more thing I want to comment on this patch is that most changes are indications of old/badly
  coded drivers. There are plenty of good BIO and block/request APIs for proper IO access and
  advancement. There is no real need in the kernel today to directly access these members (And
  most new code does not). All these drivers need to be cleaned up.

4. SCSI layer: This is mostly the bidi implementation I sent 2 months ago but now simplified
  since the actual bidi support is done in struct request so what's left is more of an API addition. I have
  kept it like this so people can see the "backward compatible" approach for implementing bidi support.
  I must warn here, going the above "request" way, will produce a much bigger patch than all above 4 put together.
  Which is not a bad thing, only that I would like to do it one at a time, sort of sneak it in more slowly.
  (and also let people comment on what they like better).

4.A. The bidi API is just there to serve IBM's OSD driver and is not a must. One can bypass scsi-lib
  and just queue a bidi request in a generic way and if that device happens to be SCSI than it will all
  just work. But on the other hand there is nothing bad in an API that makes everything nice &
  easy.

4.B. TODO: Implementation of bidi residual count.

4.C. TODO: API change (and clean-up) of the done function on the async route. In my opinion the API should
  receive a request pointer and user will extract any info he needs.

5. OSD && Varlen.

5.A. This patch implements support for variable length cdbs and large (>16) vendor specific commands. The
  actual support is at the request level, and is used by the SCSI API. Unlike regular commands
  a user of this fixture needs to keep the command's memory buffer valid until the end of the request execution.
  just like it does with its read/write and sense buffers.

5.B. Includes also a 2 liner to support OSD type SCSI devices.

6. iSCSI bidi support.
  It is given here as an example of a bidi/varlen-cdb supporting SCSI transport and driver.
  The actual patch will go through the open-iscsi project, after the appropriate SCSI and block
  support for bidi will exist in the kernel. At which time it will be broken into 3 separate patches.

6.A. Side note: The iSCSI tests pass only when I comment out line 1643 in libiscsi.c at
  iscsi_conn_start() function. (second if). This is an old issue with iscsi tests and the iscsitarget
  project. I will send a separate e-mail about this to the open-iscsi mailing list.


Boaz

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists