[RFC 0/7] scsi_sgtable implementation

[RFC 0/7] scsi_sgtable implementation

[Boaz Harrosh]

 This is a proposed implementation of the scsi sg tables
 solution to scsi-ml in preparations for support of
 bidirectional scsi commands.

 A complete bidirectional solution on top of these patches
 can be found in the usual place:
 http://www.bhalevy.com/open-osd/download/sgtable_bidi_varlen

 It would be best if these patches can be accepted into Morton's
 tree as soon as the open window for 2.6.23. So it can be tested
 as much as possible before actually getting accepted into
 scsi-misc and eventually into the kernel. The reason I say
 that is because there is a small but scary difference between
 scsi-ml with accessors over old code and accessors over new
 sg-tables code. And that is the absence of the IO members in
 the case of a DMA_NONE transfer. An un-careful coding can trip
 on it. OK I admit the chances are slim but exist.

 The code is kept backward compatible with unconverted drivers
 all the way up to the last patch. So it can be run and tested
 with some drivers unconverted. Applying the last patch will
 remove compatibility.

 [patch 1/8] stex driver BROKEN
   Once patch 2/8 is applied this driver will no longer compile.
   So here I comment out some code. The maintainer of this driver
   said that driver will be fixed soon enough in the proper manner.

 [patch 2/8] Restrict scsi accessors access to read-only
   This patch should be accepted now. Even into 2.6.23.
   The rest of the patches are dependent on it.

 [patch 3/8] libata-scsi don't set max_phys_segments higher than scsi-ml
   Also this one is relevant now. But once scsi_sgtable is used sata
   will stop working without this one.

 [patch 4/8] RFC scsi-ml: scsi_sgtable implementation
   This is the actual patch for review. Please see comments
   inside and than fire away ...
   (With old code so not to let diff make a mess)

 [patch 5/8] Remove old code from scsi_lib.c
   For easier viewing old code is removed in second patch

 [patch 6/8] RFC scsi_error.c move to sg_table implementation
   Logically belongs to 4/8. Kept separate for easier review.

 [patch 7/8] sd.c and sr.c move to scsi_sgtable implementaion
   And these too.

 [patch 8/8] Remove compatibility with unconverted drivers
   Once this patch is applied any unconverted files will not
   compile.


Free Life
Boaz

[RFC 1/8] stex driver BROKEN

[Boaz Harrosh]

  I just comment out the code, but now user-mode is broken
  what to do ??
---
 drivers/scsi/stex.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index adda296..935e2a6 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -719,8 +719,8 @@ static void stex_ys_commands(struct st_hba *hba,
 
 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
-		scsi_bufflen(ccb->cmd) =
-			le32_to_cpu(*(__le32 *)&resp->variable[0]);
+/*		scsi_bufflen(ccb->cmd) =
+			le32_to_cpu(*(__le32 *)&resp->variable[0]);*/
 		return;
 	}
 
-- 
1.5.2.2.249.g45fd

[RFC 2/8] Restrict scsi accessors access to read-only

[Boaz Harrosh]

- Users of scsi_cmnd might not change sg_count bufflen or sglist.
  So reflect that in code.
---
 include/scsi/scsi_cmnd.h |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 53e1705..aaf8282 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -138,9 +138,20 @@ extern void scsi_free_sgtable(struct scatterlist *, int);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->use_sg;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return (struct scatterlist *)cmd->request_buffer;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->request_bufflen;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-- 
1.5.2.2.249.g45fd

[RFC 3/8] libata-scsi don't set max_phys_segments higher than scsi-ml

[Boaz Harrosh]

- The set can actually be removed completely as suggested by Tomo

Boaz
---
 drivers/ata/libata-scsi.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index c228df2..13603a0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -800,8 +800,6 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
 
 	ata_scsi_sdev_config(sdev);
 
-	blk_queue_max_phys_segments(sdev->request_queue, LIBATA_MAX_PRD);
-
 	sdev->manage_start_stop = 1;
 
 	if (dev)
-- 
1.5.2.2.249.g45fd

[RFC 4/8] scsi-ml: scsi_sgtable implementation

[Boaz Harrosh]

  As proposed by James Bottomley all I/O members of struc scsi_cmnd
  and the resid member, which need to be duplicated for bidirectional
  transfers. Can be allocated together with the sg-list they are
  pointing to. This way when bidi comes the all structure can be duplicated
  with minimal change to code, and with no extra baggage when bidi is not
  used. The resulting code is the use of a new mechanism called scsi_sgtable.
  (Old code will be removed in next patch for easier reviewing)

  scsi_cmnd.h
  - define a new scsi_sgtable structure that will hold IO descriptors + the
    actual scattergather array.
  - Hold a pointer to the scsi_sgtable in scsi_cmnd.
  - Deprecate old, now unnecessary, IO members of scsi_cmnd. These members are
    kept for compatibility with unconverted drivers, still lurking around in
    the code tree. Last patch in the series removes them completely.
  - Modify data accessors to now use new members of scsi_sgtable.

  scsi_lib.c
  - Allocate scsi_sgtable of different sizes in sg-pools instead of the
    old sg arrays. The code Automatically calculates at compile time the
    maximum size sg-table that will fit in a memory-page and will allocate
    pools of (BASE_2-1) size, up to that maximum size.
  - scsi_lib is converted to use the new scsi_sgtable, in stead of the old
    members and sg-arrays.
  - The old scsi_{alloc,free}_sgtable() is no longer exported. This will break scsi_sgt
    which will need to be converted to new implementation.
  - Special code is inserted to initialize the old compatibility members from
    the new structures. This code will be removed.
  - Some extra prints for the duration of the stability period. Once
    debugging phase is finished these prints should be removed.

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c  |  168 +++++++++++++++++++++++++++++-----------------
 include/scsi/scsi_cmnd.h |   40 ++++++-----
 2 files changed, 128 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 70454b4..a5505ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -29,40 +29,32 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
-
-#define SG_MEMPOOL_NR		ARRAY_SIZE(scsi_sg_pools)
 #define SG_MEMPOOL_SIZE		2
 
 struct scsi_host_sg_pool {
 	size_t		size;
-	char		*name; 
 	struct kmem_cache	*slab;
 	mempool_t	*pool;
 };
 
-#if (SCSI_MAX_PHYS_SEGMENTS < 32)
-#error SCSI_MAX_PHYS_SEGMENTS is too small
-#endif
+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +
+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};
 
-#define SP(x) { x, "sgpool-" #x } 
-static struct scsi_host_sg_pool scsi_sg_pools[] = {
-	SP(8),
-	SP(16),
-	SP(32),
-#if (SCSI_MAX_PHYS_SEGMENTS > 32)
-	SP(64),
-#if (SCSI_MAX_PHYS_SEGMENTS > 64)
-	SP(128),
-#if (SCSI_MAX_PHYS_SEGMENTS > 128)
-	SP(256),
-#if (SCSI_MAX_PHYS_SEGMENTS > 256)
-#error SCSI_MAX_PHYS_SEGMENTS is too large
-#endif
-#endif
-#endif
-#endif
-}; 	
-#undef SP
+static struct scsi_host_sg_pool scsi_sg_pools[SG_MEMPOOL_NR];
 
 static void scsi_run_queue(struct request_queue *q);
 
@@ -701,6 +693,46 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 	return NULL;
 }
 
+static unsigned scsi_sgtable_index(unsigned nents)
+{
+	int i, size;
+
+	for (i = 0, size = 8; i < SG_MEMPOOL_NR-1; i++, size <<= 1)
+		if (size > nents)
+			return i;
+
+	if (SCSI_MAX_SG_SEGMENTS >= nents)
+		return SG_MEMPOOL_NR-1;
+
+	printk(KERN_ERR "scsi: bad segment count=%d\n", nents);
+	BUG();
+	return -1;
+}
+
+static void _scsi_free_sgtable(struct scsi_sgtable *sgt)
+{
+	mempool_free(sgt, scsi_sg_pools[sgt->sg_pool].pool);
+}
+
+static struct scsi_sgtable *_scsi_alloc_sgtable(int sg_count, gfp_t gfp_mask)
+{
+	struct scsi_host_sg_pool *sgp;
+	struct scsi_sgtable *sgt;
+	unsigned int index;
+
+	index = scsi_sgtable_index(sg_count);
+	sgp = scsi_sg_pools + index;
+
+	sgt = mempool_alloc(sgp->pool, gfp_mask);
+	if (unlikely(!sgt))
+		return NULL;
+
+	memset(sgt, 0, SG_TABLE_SIZEOF(sgp->size));
+	sgt->sg_pool = index;
+	sgt->sg_count = sg_count;
+	return sgt;
+}
+
 struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
@@ -775,15 +807,15 @@ EXPORT_SYMBOL(scsi_free_sgtable);
  */
 static void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
-		scsi_free_sgtable(cmd->request_buffer, cmd->sglist_len);
+	if (cmd->sgtable)
+		_scsi_free_sgtable(cmd->sgtable);
 
-	/*
-	 * Zero these out.  They now point to freed memory, and it is
-	 * dangerous to hang onto the pointers.
-	 */
+	cmd->sgtable = NULL;
+
+	/*FIXME: make code backward compatible with old system */
 	cmd->request_buffer = NULL;
 	cmd->request_bufflen = 0;
+	cmd->use_sg = 0;
 }
 
 /*
@@ -817,13 +849,14 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = cmd->request_bufflen;
+	int this_count = scsi_bufflen(cmd);
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;
+	int resid = scsi_get_resid(cmd);
 
 	scsi_release_buffers(cmd);
 
@@ -849,7 +882,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = resid;
 	}
 
 	/*
@@ -859,7 +892,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, "
 				      "%d bytes done.\n",
 				      req->nr_sectors, good_bytes));
-	SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n", cmd->use_sg));
 
 	if (clear_errors)
 		req->errors = 0;
@@ -991,44 +1023,43 @@ EXPORT_SYMBOL(scsi_io_completion);
 static int scsi_init_io(struct scsi_cmnd *cmd)
 {
 	struct request     *req = cmd->request;
-	struct scatterlist *sgpnt;
 	int		   count;
-
-	/*
-	 * We used to not use scatter-gather for single segment request,
-	 * but now we do (it makes highmem I/O easier to support without
-	 * kmapping pages)
-	 */
-	cmd->use_sg = req->nr_phys_segments;
+	struct scsi_sgtable *sgt;
 
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	sgpnt = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
-	if (unlikely(!sgpnt)) {
+	sgt = _scsi_alloc_sgtable(req->nr_phys_segments, GFP_ATOMIC);
+	if (unlikely(!sgt)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
 	req->buffer = NULL;
-	cmd->request_buffer = (char *) sgpnt;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sgt->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sgt->length = req->nr_sectors << 9;
 
+	cmd->sgtable = sgt;
 	/* 
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
-	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
+	count = blk_rq_map_sg(req->q, req, sgt->sglist);
+	if (likely(count <= sgt->sg_count)) {
+		sgt->sg_count = count;
+		
+		/*FIXME: make code backward compatible with old system */
+		cmd->request_buffer = sgt->sglist;
+		cmd->request_bufflen = sgt->length;
+		cmd->use_sg = sgt->sg_count;
+
 		return BLKPREP_OK;
 	}
 
 	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
+	printk(KERN_ERR "counted %d, received %d\n", count, scsi_sg_count(cmd));
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 
@@ -1084,7 +1115,7 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
 	 * successfully. Since this is a REQ_BLOCK_PC command the
 	 * caller should check the request's errors value
 	 */
-	scsi_io_completion(cmd, cmd->request_bufflen);
+	scsi_io_completion(cmd, scsi_bufflen(cmd));
 }
 
 static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
@@ -1113,9 +1144,7 @@ static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
 
-		cmd->request_bufflen = 0;
-		cmd->request_buffer = NULL;
-		cmd->use_sg = 0;
+		cmd->sgtable = NULL;
 		req->buffer = NULL;
 	}
 
@@ -1576,7 +1605,7 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 		return NULL;
 
 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_PHYS_SEGMENTS);
+	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
@@ -1655,9 +1684,15 @@ void scsi_unblock_requests(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unblock_requests);
 
+const char* sg_names[] = {
+	"sgtable-7", "sgtable-15", "sgtable-31", "sgtable-63",
+	"sgtable-127", "sgtable-255", "sgtable-511"
+};
+
 int __init scsi_init_queue(void)
 {
 	int i;
+	unsigned size;
 
 	scsi_io_context_cache = kmem_cache_create("scsi_io_context",
 					sizeof(struct scsi_io_context),
@@ -1667,25 +1702,32 @@ int __init scsi_init_queue(void)
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < SG_MEMPOOL_NR; i++) {
+	for (i = 0, size = 8; i < SG_MEMPOOL_NR; i++, size <<= 1) {
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
-		int size = sgp->size * sizeof(struct scatterlist);
-
-		sgp->slab = kmem_cache_create(sgp->name, size, 0,
-				SLAB_HWCACHE_ALIGN, NULL, NULL);
+		sgp->size = size-1;
+		sgp->slab = kmem_cache_create(sg_names[i],
+				SG_TABLE_SIZEOF(sgp->size), 0, 0, NULL, NULL);
 		if (!sgp->slab) {
 			printk(KERN_ERR "SCSI: can't init sg slab %s\n",
-					sgp->name);
+					sg_names[i]);
 		}
 
 		sgp->pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE,
 						     sgp->slab);
 		if (!sgp->pool) {
 			printk(KERN_ERR "SCSI: can't init sg mempool %s\n",
-					sgp->name);
+					sg_names[i]);
 		}
 	}
 
+	/* FIXME: Here for the debugging phase only */
+	printk(KERN_ERR
+		"SCSI: max_sg_count=%d SG_MEMPOOL_NR=%d page=%ld "
+		"so_sgtable=%Zd so_scaterlist=%Zd\n",
+		SCSI_MAX_SG_SEGMENTS, SG_MEMPOOL_NR, PAGE_SIZE,
+		sizeof(struct scsi_sgtable), sizeof(struct scatterlist)
+	);
+
 	return 0;
 }
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index aaf8282..d408d93 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -11,6 +11,16 @@ struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
+struct scsi_sgtable {
+	unsigned length;
+	int resid;
+	short sg_count;
+	short sg_pool;
+	struct scatterlist sglist[0];
+};
+
+#define SG_TABLE_SIZEOF(sg_count) ((sg_count)*sizeof(struct scatterlist) \
+                                   + sizeof(struct scsi_sgtable))
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
@@ -64,15 +74,11 @@ struct scsi_cmnd {
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16
 	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned request_bufflen;	/* Actual request size */
 
 	struct timer_list eh_timeout;	/* Used to time out the command. */
-	void *request_buffer;		/* Actual requested buffer */
+	struct scsi_sgtable *sgtable;
 
 	/* These elements define the operation we ultimately want to perform */
-	unsigned short use_sg;	/* Number of pieces of scatter-gather */
-	unsigned short sglist_len;	/* size of malloc'd scatter-gather list */
-
 	unsigned underflow;	/* Return error if less than
 				   this amount is transferred */
 
@@ -82,10 +88,6 @@ struct scsi_cmnd {
 				   reconnects.   Probably == sector
 				   size */
 
-	int resid;		/* Number of bytes requested to be
-				   transferred less actual number
-				   transferred (0 if not supported) */
-
 	struct request *request;	/* The command we are
 				   	   working on */
 
@@ -117,6 +119,12 @@ struct scsi_cmnd {
 
 	unsigned char tag;	/* SCSI-II queued command tag */
 	unsigned long pid;	/* Process ID, starts at 0. Unique per host. */
+
+	unsigned short sglist_len;
+	unsigned short __deprecated use_sg;
+	unsigned __deprecated request_bufflen;
+	void __deprecated *request_buffer;
+	int __deprecated resid;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
@@ -132,35 +140,33 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
-extern void scsi_free_sgtable(struct scatterlist *, int);
-
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
 static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
 {
-	return cmd->use_sg;
+	return cmd->sgtable ? cmd->sgtable->sg_count : 0;
 }
 
 static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
 {
-	return (struct scatterlist *)cmd->request_buffer;
+	return cmd->sgtable ? cmd->sgtable->sglist : 0;
 }
 
 static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
 {
-	return cmd->request_bufflen;
+	return cmd->sgtable ? cmd->sgtable->length : 0;
 }
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	if (cmd->sgtable)
+		cmd->sgtable->resid = resid;
 }
 
 static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 {
-	return cmd->resid;
+	return cmd->sgtable ? cmd->sgtable->resid : 0;
 }
 
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
-- 
1.5.2.2.249.g45fd

[RFC 5/8] Remove old code from scsi_lib.c

[Boaz Harrosh]

   For easier viewing old code is removed in this patch.
   Also SCSI_MAX_PHYS_SEGMENTS is no longer used. Remove it.

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c  |   55 ----------------------------------------------
 include/scsi/scsi.h      |    7 ------
 include/scsi/scsi_cmnd.h |    1 -
 3 files changed, 0 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a5505ec..56829bf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -733,61 +733,6 @@ static struct scsi_sgtable *_scsi_alloc_sgtable(int sg_count, gfp_t gfp_mask)
 	return sgt;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
-	struct scsi_host_sg_pool *sgp;
-	struct scatterlist *sgl;
-
-	BUG_ON(!cmd->use_sg);
-
-	switch (cmd->use_sg) {
-	case 1 ... 8:
-		cmd->sglist_len = 0;
-		break;
-	case 9 ... 16:
-		cmd->sglist_len = 1;
-		break;
-	case 17 ... 32:
-		cmd->sglist_len = 2;
-		break;
-#if (SCSI_MAX_PHYS_SEGMENTS > 32)
-	case 33 ... 64:
-		cmd->sglist_len = 3;
-		break;
-#if (SCSI_MAX_PHYS_SEGMENTS > 64)
-	case 65 ... 128:
-		cmd->sglist_len = 4;
-		break;
-#if (SCSI_MAX_PHYS_SEGMENTS  > 128)
-	case 129 ... 256:
-		cmd->sglist_len = 5;
-		break;
-#endif
-#endif
-#endif
-	default:
-		return NULL;
-	}
-
-	sgp = scsi_sg_pools + cmd->sglist_len;
-	sgl = mempool_alloc(sgp->pool, gfp_mask);
-	return sgl;
-}
-
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scatterlist *sgl, int index)
-{
-	struct scsi_host_sg_pool *sgp;
-
-	BUG_ON(index >= SG_MEMPOOL_NR);
-
-	sgp = scsi_sg_pools + index;
-	mempool_free(sgl, sgp->pool);
-}
-
-EXPORT_SYMBOL(scsi_free_sgtable);
-
 /*
  * Function:    scsi_release_buffers()
  *
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 9f8f80a..702fcfe 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -11,13 +11,6 @@
 #include <linux/types.h>
 
 /*
- *	The maximum sg list length SCSI can cope with
- *	(currently must be a power of 2 between 32 and 256)
- */
-#define SCSI_MAX_PHYS_SEGMENTS	MAX_PHYS_SEGMENTS
-
-
-/*
  *	SCSI command lengths
  */
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index d408d93..4b87c0f 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -120,7 +120,6 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 	unsigned long pid;	/* Process ID, starts at 0. Unique per host. */
 
-	unsigned short sglist_len;
 	unsigned short __deprecated use_sg;
 	unsigned __deprecated request_bufflen;
 	void __deprecated *request_buffer;
-- 
1.5.2.2.249.g45fd

[RFC 6/8] scsi_error.c move to scsi_sgtable implementation

[Boaz Harrosh]

 - Careful considerations in scsi_send_eh_cmnd. Everything
   is kept on the stack as before.
 - This code is backward compatible with unconverted drivers.
   Compatibility will be removed in last patch.

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |   46 ++++++++++++++++++++++++++------------------
 1 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9adb64a..1105011 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -613,13 +613,10 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	unsigned long flags;
-	struct scatterlist sgl;
 	unsigned char old_cmnd[MAX_COMMAND_SIZE];
 	enum dma_data_direction old_data_direction;
-	unsigned short old_use_sg;
 	unsigned char old_cmd_len;
-	unsigned old_bufflen;
-	void *old_buffer;
+	struct scsi_sgtable *old_sgtable;
 	int rtn;
 
 	/*
@@ -629,31 +626,39 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	 * we will need to restore these values prior to running the actual
 	 * command.
 	 */
-	old_buffer = scmd->request_buffer;
-	old_bufflen = scmd->request_bufflen;
+	old_sgtable = scmd->sgtable;
 	memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	old_data_direction = scmd->sc_data_direction;
 	old_cmd_len = scmd->cmd_len;
-	old_use_sg = scmd->use_sg;
 
 	memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 	memcpy(scmd->cmnd, cmnd, cmnd_size);
 
 	if (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
+		struct {
+			struct scsi_sgtable sgtable;
+			struct scatterlist sgl;
+		} new_sgtable;
+
+		sg_init_one(new_sgtable.sgtable.sglist, scmd->sense_buffer,
 			    sizeof(scmd->sense_buffer));
 
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
-		scmd->request_buffer = &sgl;
-		scmd->use_sg = 1;
+		new_sgtable.sgtable.length = new_sgtable.sgtable.sglist[0].length;
+		new_sgtable.sgtable.resid = 0;
+		new_sgtable.sgtable.sg_count = 1;
+		new_sgtable.sgtable.sg_pool = -1;	/* invalid */
+		scmd->sgtable = &new_sgtable.sgtable;
 	} else {
-		scmd->request_buffer = NULL;
-		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
-		scmd->use_sg = 0;
+		scmd->sgtable = NULL;
 	}
 
+	/*FIXME: make code backward compatible with old system */
+	cmd->request_buffer = scsi_sglist(scmd);
+	cmd->request_bufflen = scsi_bufflen(scmd);
+	cmd->use_sg = scsi_sg_count(scmd);
+
 	scmd->underflow = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
@@ -714,13 +719,17 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	/*
 	 * Restore original data
 	 */
-	scmd->request_buffer = old_buffer;
-	scmd->request_bufflen = old_bufflen;
+	scmd->sgtable = old_sgtable;
 	memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = old_data_direction;
 	scmd->cmd_len = old_cmd_len;
-	scmd->use_sg = old_use_sg;
 	scmd->result = old_result;
+
+	/*FIXME: make code backward compatible with old system */
+	cmd->request_buffer = scsi_sglist(scmd);
+	cmd->request_bufflen = scsi_bufflen(scmd);
+	cmd->use_sg = scsi_sg_count(scmd);
+
 	return rtn;
 }
 
@@ -1673,8 +1682,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
     
 	scmd->scsi_done		= scsi_reset_provider_done_command;
 	scmd->done			= NULL;
-	scmd->request_buffer		= NULL;
-	scmd->request_bufflen		= 0;
+	scmd->sgtable			= NULL;
 
 	scmd->cmd_len			= 0;
 
-- 
1.5.2.2.249.g45fd

[RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation

[Boaz Harrosh]

  - sd and sr would adjust IO size to align on device's block
    size. So code needs to change once we move to scsi_sgtable
    implementation. (Not compatible with un-converted drivers)
  - Convert code to use scsi_for_each_sg
  - Use Data accessors wherever is appropriate.

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/sd.c |   10 ++++------
 drivers/scsi/sr.c |   27 ++++++++++++---------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 448d316..459fd23 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -338,7 +338,7 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 	struct request *rq = SCpnt->request;
 	struct gendisk *disk = rq->rq_disk;
 	sector_t block = rq->sector;
-	unsigned int this_count = SCpnt->request_bufflen >> 9;
+	unsigned int this_count = scsi_bufflen(SCpnt) >> 9;
 	unsigned int timeout = sdp->timeout;
 
 	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
@@ -418,9 +418,6 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_6;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
-		return 0;
 	}
 
 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
@@ -480,7 +477,8 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	BUG_ON(!SCpnt->sgtable);
+	SCpnt->sgtable->length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -892,7 +890,7 @@ static struct block_device_operations sd_fops = {
 static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
- 	unsigned int xfer_size = SCpnt->request_bufflen;
+ 	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
  	u64 bad_lba;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index f9a52af..ed61ca9 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -218,7 +218,7 @@ int sr_media_change(struct cdrom_device_info *cdi, int slot)
 static void rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+	int this_count = scsi_bufflen(SCpnt);
 	int good_bytes = (result == 0 ? this_count : 0);
 	int block_sectors = 0;
 	long error_sector;
@@ -345,23 +345,20 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
 	} else if (rq_data_dir(SCpnt->request) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(SCpnt->request, "Unknown sr command");
-		return 0;
 	}
 
 	{
-		struct scatterlist *sg = SCpnt->request_buffer;
-		int i, size = 0;
-		for (i = 0; i < SCpnt->use_sg; i++)
-			size += sg[i].length;
+		struct scatterlist *sg;
+		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
+		scsi_for_each_sg (SCpnt, sg, sg_count, i)
+			size += sg->length;
 
-		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
+		if (size != SCpnt->sgtable->length) {
 			scmd_printk(KERN_ERR, SCpnt,
 				"mismatch count %d, bytes %d\n",
-				size, SCpnt->request_bufflen);
-			if (SCpnt->request_bufflen > size)
-				SCpnt->request_bufflen = size;
+				size, SCpnt->sgtable->length);
+			if (SCpnt->sgtable->length > size)
+				SCpnt->sgtable->length = size;
 		}
 	}
 
@@ -369,12 +366,12 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)SCpnt->request->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (SCpnt->sgtable->length % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		return 0;
 	}
 
-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (SCpnt->sgtable->length >> 9) / (s_size >> 9);
 
 
 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
@@ -388,7 +385,7 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sgtable->length = this_count * s_size;
 	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
-- 
1.5.2.2.249.g45fd

[RFC 8/8] Remove compatibility with unconverted drivers

[Boaz Harrosh]

   Old IO members are removed from struct scsi_cmnd

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |   10 ----------
 drivers/scsi/scsi_lib.c   |   11 -----------
 include/scsi/scsi_cmnd.h  |    5 -----
 3 files changed, 0 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 1105011..936dfd1 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -654,11 +654,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 		scmd->sgtable = NULL;
 	}
 
-	/*FIXME: make code backward compatible with old system */
-	cmd->request_buffer = scsi_sglist(scmd);
-	cmd->request_bufflen = scsi_bufflen(scmd);
-	cmd->use_sg = scsi_sg_count(scmd);
-
 	scmd->underflow = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
@@ -725,11 +720,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	scmd->cmd_len = old_cmd_len;
 	scmd->result = old_result;
 
-	/*FIXME: make code backward compatible with old system */
-	cmd->request_buffer = scsi_sglist(scmd);
-	cmd->request_bufflen = scsi_bufflen(scmd);
-	cmd->use_sg = scsi_sg_count(scmd);
-
 	return rtn;
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 56829bf..dcd8826 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -756,11 +756,6 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 		_scsi_free_sgtable(cmd->sgtable);
 
 	cmd->sgtable = NULL;
-
-	/*FIXME: make code backward compatible with old system */
-	cmd->request_buffer = NULL;
-	cmd->request_bufflen = 0;
-	cmd->use_sg = 0;
 }
 
 /*
@@ -994,12 +989,6 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	count = blk_rq_map_sg(req->q, req, sgt->sglist);
 	if (likely(count <= sgt->sg_count)) {
 		sgt->sg_count = count;
-		
-		/*FIXME: make code backward compatible with old system */
-		cmd->request_buffer = sgt->sglist;
-		cmd->request_bufflen = sgt->length;
-		cmd->use_sg = sgt->sg_count;
-
 		return BLKPREP_OK;
 	}
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 4b87c0f..c13d47f 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -119,11 +119,6 @@ struct scsi_cmnd {
 
 	unsigned char tag;	/* SCSI-II queued command tag */
 	unsigned long pid;	/* Process ID, starts at 0. Unique per host. */
-
-	unsigned short __deprecated use_sg;
-	unsigned __deprecated request_bufflen;
-	void __deprecated *request_buffer;
-	int __deprecated resid;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
-- 
1.5.2.2.249.g45fd

RE: [RFC 1/8] stex driver BROKEN

[Lin Yu]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Boaz Harrosh
> Sent: Thursday, July 05, 2007 6:43 AM
> To: James Bottomley; FUJITA Tomonori; Andrew Morton; linux-scsi
> Subject: [RFC 1/8] stex driver BROKEN
> 
> 
> 
>   I just comment out the code, but now user-mode is broken
>   what to do ??
> ---
>  drivers/scsi/stex.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> index adda296..935e2a6 100644
> --- a/drivers/scsi/stex.c
> +++ b/drivers/scsi/stex.c
> @@ -719,8 +719,8 @@ static void stex_ys_commands(struct st_hba *hba,
>  
>  	if (ccb->cmd->cmnd[0] == MGT_CMD &&
>  		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
> -		scsi_bufflen(ccb->cmd) =
> -			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> +/*		scsi_bufflen(ccb->cmd) =
> +			le32_to_cpu(*(__le32 *)&resp->variable[0]);*/
>  		return;
>  	}
>  
> -- 
> 1.5.2.2.249.g45fd
> 
> 
> -

NAK

A patch for this issue just sent.



Ed Lin
 

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Boaz Harrosh]

Boaz Harrosh wrote:
>   - The old scsi_{alloc,free}_sgtable() is no longer exported. This will break scsi_sgt
>     which will need to be converted to new implementation.

Hi Tomo.

Just for completion of the RFC I would like to have A quick draft patch
that converts stgt to the proposed scsi_sgtable implementation. If you
are busy, I have looked, it should not be too difficult for me to do it.
Do you want that I try? or maybe you could do something quick?

I think a start would be

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index f5a1dda..eecfe70 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -157,6 +157,9 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
+extern struct scsi_sgtable *scsi_alloc_sgtable(int sg_count, gfp_t gfp_mask);
+extern void scsi_free_sgtable(struct scsi_sgtable *sgt);
+
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Mike Christie]

Boaz Harrosh wrote:
> +/*
> + * Should fit within a single page.
> + */
> +enum { SCSI_MAX_SG_SEGMENTS =
> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> +	sizeof(struct scatterlist)) };
> +
> +enum { SG_MEMPOOL_NR =
> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> +	(SCSI_MAX_SG_SEGMENTS >= 511)
> +};
>  

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
some other arch, we were going over a page when doing 
SCSI_MAX_PHYS_SEGMENTS of 256 right?

What happened to Jens's scatter list chaining and how does this relate 
to it then?

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[FUJITA Tomonori]

AF:
From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 05 Jul 2007 16:43:42 +0300

>   As proposed by James Bottomley all I/O members of struc scsi_cmnd
>   and the resid member, which need to be duplicated for bidirectional
>   transfers. Can be allocated together with the sg-list they are
>   pointing to. This way when bidi comes the all structure can be duplicated
>   with minimal change to code, and with no extra baggage when bidi is not
>   used. The resulting code is the use of a new mechanism called scsi_sgtable.
>   (Old code will be removed in next patch for easier reviewing)

I think that it would be better to push this with Jens' sglist and
bidi support together to mainline. Changing the scsi-ml sg code and
testing it again and again doesn't sound nice.

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[FUJITA Tomonori]

From: Mike Christie <michaelc@cs.wisc.edu>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

> Boaz Harrosh wrote:
> > +/*
> > + * Should fit within a single page.
> > + */
> > +enum { SCSI_MAX_SG_SEGMENTS =
> > +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> > +	sizeof(struct scatterlist)) };
> > +
> > +enum { SG_MEMPOOL_NR =
> > +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> > +	(SCSI_MAX_SG_SEGMENTS >= 511)
> > +};
> >  
> 
> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
> some other arch, we were going over a page when doing 
> SCSI_MAX_PHYS_SEGMENTS of 256 right?

Seems that 170 with x86 and 127 with x86_64.


> What happened to Jens's scatter list chaining and how does this relate 
> to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I
think.

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Boaz Harrosh]

FUJITA Tomonori wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
> Date: Thu, 12 Jul 2007 14:09:44 -0500
> 
>> Boaz Harrosh wrote:
>>> +/*
>>> + * Should fit within a single page.
>>> + */
>>> +enum { SCSI_MAX_SG_SEGMENTS =
>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
>>> +	sizeof(struct scatterlist)) };
>>> +
>>> +enum { SG_MEMPOOL_NR =
>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
>>> +};
>>>  
>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
>> some other arch, we were going over a page when doing 
>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
> 
> Seems that 170 with x86 and 127 with x86_64.
> 

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is
automatically calculated in compile time. Arch people
don't have the headache "did I break SCSI-ml?". 
For example observe the current bug with i386 
CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big
pages can gain even more.

> 
>> What happened to Jens's scatter list chaining and how does this relate 
>> to it then?
> 
> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
> want. We can remove the above code.
> 
> We need to push this and Jens' sglist together in one merge window, I
> think.

No Tomo the above does not go away. What goes away is maybe:

 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);

I'm working on a convergence patches that will do scsi_sg_pools cleanup
which is common to both our patches, than scsi_sgtable, and than 
sg-chaining on top of that. I hope it gets accepted. 
The sg-chaining is much much simpler over scsi_sgtables.

If both sg_chaining and scsi_sgtables gets into the same merge window
it could be grate. It will need some testing period.

Boaz

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Jens Axboe]

On Wed, Jul 18 2007, Boaz Harrosh wrote:
> FUJITA Tomonori wrote:
> > From: Mike Christie <michaelc@cs.wisc.edu>
> > Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
> > Date: Thu, 12 Jul 2007 14:09:44 -0500
> > 
> >> Boaz Harrosh wrote:
> >>> +/*
> >>> + * Should fit within a single page.
> >>> + */
> >>> +enum { SCSI_MAX_SG_SEGMENTS =
> >>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> >>> +	sizeof(struct scatterlist)) };
> >>> +
> >>> +enum { SG_MEMPOOL_NR =
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> >>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
> >>> +};
> >>>  
> >> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
> >> some other arch, we were going over a page when doing 
> >> SCSI_MAX_PHYS_SEGMENTS of 256 right?
> > 
> > Seems that 170 with x86 and 127 with x86_64.
> > 
> 
> with scsi_sgtable we get one less than now
> 
> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
> --------------------------|-------------------------|---------------------------
> x86_64                    | 127                     |32
> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
> i386 other                | 255                     |16
> 
> What's nice about this code is that now finally it is
> automatically calculated in compile time. Arch people
> don't have the headache "did I break SCSI-ml?". 
> For example observe the current bug with i386 
> CONFIG_HIGHMEM64G=y.
> 
> The same should be done with BIO's. Than ARCHs with big
> pages can gain even more.
> 
> > 
> >> What happened to Jens's scatter list chaining and how does this relate 
> >> to it then?
> > 
> > With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
> > want. We can remove the above code.
> > 
> > We need to push this and Jens' sglist together in one merge window, I
> > think.
> 
> No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use
chaining to get us where we want.

>  	blk_queue_max_hw_segments(q, shost->sg_tablesize);
> -	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
>  	blk_queue_max_sectors(q, shost->max_sectors);
> 
> I'm working on a convergence patches that will do scsi_sg_pools cleanup
> which is common to both our patches, than scsi_sgtable, and than 
> sg-chaining on top of that. I hope it gets accepted. 
> The sg-chaining is much much simpler over scsi_sgtables.

Sorry, I don't follow this paragraph at all. What is the scsi_sgtables
change you are referring to? And how does it make sg chaining so much
simpler?

I guess my problem is that I don't know what problem this scsi_sgtables
you refer to is fixing?

-- 
Jens Axboe

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Boaz Harrosh]

Jens Axboe wrote:
> On Wed, Jul 18 2007, Boaz Harrosh wrote:
>> FUJITA Tomonori wrote:
>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
>>> Date: Thu, 12 Jul 2007 14:09:44 -0500
>>>
>>>> Boaz Harrosh wrote:
>>>>> +/*
>>>>> + * Should fit within a single page.
>>>>> + */
>>>>> +enum { SCSI_MAX_SG_SEGMENTS =
>>>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
>>>>> +	sizeof(struct scatterlist)) };
>>>>> +
>>>>> +enum { SG_MEMPOOL_NR =
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
>>>>> +};
>>>>>  
>>>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
>>>> some other arch, we were going over a page when doing 
>>>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
>>> Seems that 170 with x86 and 127 with x86_64.
>>>
>> with scsi_sgtable we get one less than now
>>
>> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
>> --------------------------|-------------------------|---------------------------
>> x86_64                    | 127                     |32
>> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
>> i386 other                | 255                     |16
>>
>> What's nice about this code is that now finally it is
>> automatically calculated in compile time. Arch people
>> don't have the headache "did I break SCSI-ml?". 
>> For example observe the current bug with i386 
>> CONFIG_HIGHMEM64G=y.
>>
>> The same should be done with BIO's. Than ARCHs with big
>> pages can gain even more.
>>
>>>> What happened to Jens's scatter list chaining and how does this relate 
>>>> to it then?
>>> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
>>> want. We can remove the above code.
>>>
>>> We need to push this and Jens' sglist together in one merge window, I
>>> think.
>> No Tomo the above does not go away. What goes away is maybe:
> 
> It does go away, since we can just set it to some safe value and use
> chaining to get us where we want.
In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist
anymore.

The new SCSI_MAX_SG_SEGMENTS (your name by the way) is right there
and is calculated to maximize allocation in one page.
(I guess the right name is SCSI_MAX_PHYS_SEGMENTS_IN_A_PAGE)
which will be needed in both your patches and mine.

> 
>>  	blk_queue_max_hw_segments(q, shost->sg_tablesize);
>> -	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
>>  	blk_queue_max_sectors(q, shost->max_sectors);
>>
The reporting above is not needed and can be what ever block layer
considers safe/optimized.

>> I'm working on a convergence patches that will do scsi_sg_pools cleanup
>> which is common to both our patches, than scsi_sgtable, and than 
>> sg-chaining on top of that. I hope it gets accepted. 
>> The sg-chaining is much much simpler over scsi_sgtables.
> 
> Sorry, I don't follow this paragraph at all. What is the scsi_sgtables
> change you are referring to? And how does it make sg chaining so much
> simpler?
> 
> I guess my problem is that I don't know what problem this scsi_sgtables
> you refer to is fixing?
> 

scsi_sgtable is a solution proposed by James Bottomley where all I/O members 
of struct scsi_cmnd and the resid member, which need to be duplicated for 
bidirectional transfers, can be allocated together with the sg-list they are
pointing to. This way when bidi comes, the all structure can be duplicated
with minimal change to code, and with no extra baggage when bidi is not
used.
This was the all motivation for the data accessors and cleanup, swiping through
the entire scsi tree. So when implementation changes drivers do not change with
them. Now meanwhile moving over drivers code we (well Tomo mostly) removed the
old !use_sg code path, and also abstracted the 2 major hot spots of above
usages with scsi_dma_{un,}map, and the scsi_for_each_sg. Actually that one
was changed from the original definition to match you macro.

Since scsi_sgtable is an encapsulation of the actual scatterlist array together
with the sg_count bufflen and pool_index, it gives code a nice clean OO touch,
and makes handling very easy, thats all. It only simplifies things at the scsi-ml
level.

I hope that helps a bit. Thanks
Boaz

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Jens Axboe]

On Wed, Jul 18 2007, Boaz Harrosh wrote:
> Jens Axboe wrote:
> > On Wed, Jul 18 2007, Boaz Harrosh wrote:
> >> FUJITA Tomonori wrote:
> >>> From: Mike Christie <michaelc@cs.wisc.edu>
> >>> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
> >>> Date: Thu, 12 Jul 2007 14:09:44 -0500
> >>>
> >>>> Boaz Harrosh wrote:
> >>>>> +/*
> >>>>> + * Should fit within a single page.
> >>>>> + */
> >>>>> +enum { SCSI_MAX_SG_SEGMENTS =
> >>>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> >>>>> +	sizeof(struct scatterlist)) };
> >>>>> +
> >>>>> +enum { SG_MEMPOOL_NR =
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> >>>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
> >>>>> +};
> >>>>>  
> >>>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
> >>>> some other arch, we were going over a page when doing 
> >>>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
> >>> Seems that 170 with x86 and 127 with x86_64.
> >>>
> >> with scsi_sgtable we get one less than now
> >>
> >> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
> >> --------------------------|-------------------------|---------------------------
> >> x86_64                    | 127                     |32
> >> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
> >> i386 other                | 255                     |16
> >>
> >> What's nice about this code is that now finally it is
> >> automatically calculated in compile time. Arch people
> >> don't have the headache "did I break SCSI-ml?". 
> >> For example observe the current bug with i386 
> >> CONFIG_HIGHMEM64G=y.
> >>
> >> The same should be done with BIO's. Than ARCHs with big
> >> pages can gain even more.
> >>
> >>>> What happened to Jens's scatter list chaining and how does this relate 
> >>>> to it then?
> >>> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
> >>> want. We can remove the above code.
> >>>
> >>> We need to push this and Jens' sglist together in one merge window, I
> >>> think.
> >> No Tomo the above does not go away. What goes away is maybe:
> > 
> > It does go away, since we can just set it to some safe value and use
> > chaining to get us where we want.
>
> In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist
> anymore.

Sure, I could just kill it as well. The point is that it's a parallel
development, there's nothing in your patch that helps the sg chaining
whatsoever. The only "complex" thing in the SCSI layer for sg chaining,
is chaining when allocating and walking that chain on freeing. That's
it!

> The new SCSI_MAX_SG_SEGMENTS (your name by the way) is right there
> and is calculated to maximize allocation in one page.

Yes, it's still a good idea to make sure you get good packing in the
page, it's of course cheaper to have less chaining. But it's not
critical in the same way as before, when it could impact IO layout and
performance.

> (I guess the right name is SCSI_MAX_PHYS_SEGMENTS_IN_A_PAGE)
> which will be needed in both your patches and mine.

That would be better name.

> >>  	blk_queue_max_hw_segments(q, shost->sg_tablesize);
> >> -	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
> >>  	blk_queue_max_sectors(q, shost->max_sectors);
> >>
> The reporting above is not needed and can be what ever block layer
> considers safe/optimized.

You still need to set it, you can't just ignore it. Whether you redefine
SCSI_MAX_SG_SEGMENTS or use (unsigned short) -1 doesn't really matter a
whole lot.

> >> I'm working on a convergence patches that will do scsi_sg_pools cleanup
> >> which is common to both our patches, than scsi_sgtable, and than 
> >> sg-chaining on top of that. I hope it gets accepted. 
> >> The sg-chaining is much much simpler over scsi_sgtables.
> > 
> > Sorry, I don't follow this paragraph at all. What is the scsi_sgtables
> > change you are referring to? And how does it make sg chaining so much
> > simpler?
> > 
> > I guess my problem is that I don't know what problem this scsi_sgtables
> > you refer to is fixing?
> > 
> 
> scsi_sgtable is a solution proposed by James Bottomley where all I/O
> members of struct scsi_cmnd and the resid member, which need to be
> duplicated for bidirectional transfers, can be allocated together with
> the sg-list they are pointing to. This way when bidi comes, the all
> structure can be duplicated with minimal change to code, and with no
> extra baggage when bidi is not used.  This was the all motivation for
> the data accessors and cleanup, swiping through the entire scsi tree.
> So when implementation changes drivers do not change with them. Now
> meanwhile moving over drivers code we (well Tomo mostly) removed the
> old !use_sg code path, and also abstracted the 2 major hot spots of
> above usages with scsi_dma_{un,}map, and the scsi_for_each_sg.
> Actually that one was changed from the original definition to match
> you macro.
> 
> Since scsi_sgtable is an encapsulation of the actual scatterlist array
> together with the sg_count bufflen and pool_index, it gives code a
> nice clean OO touch, and makes handling very easy, thats all. It only
> simplifies things at the scsi-ml level.

So it's a pre-requisite for bidi support, it has no bearing on sg
chaining. The only thing they have in common is that the touch some of
the same code, that does not make them dependent or related beyond that
necessarily.

-- 
Jens Axboe

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Benny Halevy]

Jens Axboe wrote:
> On Wed, Jul 18 2007, Boaz Harrosh wrote:
>> Jens Axboe wrote:
>>> On Wed, Jul 18 2007, Boaz Harrosh wrote:
>>>> FUJITA Tomonori wrote:
>>>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>>>> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
>>>>> Date: Thu, 12 Jul 2007 14:09:44 -0500
>>>>>
>>>>>> Boaz Harrosh wrote:
>>>>>>> +/*
>>>>>>> + * Should fit within a single page.
>>>>>>> + */
>>>>>>> +enum { SCSI_MAX_SG_SEGMENTS =
>>>>>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
>>>>>>> +	sizeof(struct scatterlist)) };
>>>>>>> +
>>>>>>> +enum { SG_MEMPOOL_NR =
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
>>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
>>>>>>> +};
>>>>>>>  
>>>>>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
>>>>>> some other arch, we were going over a page when doing 
>>>>>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
>>>>> Seems that 170 with x86 and 127 with x86_64.
>>>>>
>>>> with scsi_sgtable we get one less than now
>>>>
>>>> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
>>>> --------------------------|-------------------------|---------------------------
>>>> x86_64                    | 127                     |32
>>>> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
>>>> i386 other                | 255                     |16
>>>>
>>>> What's nice about this code is that now finally it is
>>>> automatically calculated in compile time. Arch people
>>>> don't have the headache "did I break SCSI-ml?". 
>>>> For example observe the current bug with i386 
>>>> CONFIG_HIGHMEM64G=y.
>>>>
>>>> The same should be done with BIO's. Than ARCHs with big
>>>> pages can gain even more.
>>>>
>>>>>> What happened to Jens's scatter list chaining and how does this relate 
>>>>>> to it then?
>>>>> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
>>>>> want. We can remove the above code.
>>>>>
>>>>> We need to push this and Jens' sglist together in one merge window, I
>>>>> think.
>>>> No Tomo the above does not go away. What goes away is maybe:
>>> It does go away, since we can just set it to some safe value and use
>>> chaining to get us where we want.
>> In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist
>> anymore.
> 
> Sure, I could just kill it as well. The point is that it's a parallel
> development, there's nothing in your patch that helps the sg chaining
> whatsoever. The only "complex" thing in the SCSI layer for sg chaining,
> is chaining when allocating and walking that chain on freeing. That's
> it!

It seems like having the pool index in the sgtable structure simplifies
the implementation a bit for allocation and freeing of linked sgtables.
Boaz will send an example tomorrow (hopefully) showing how the merged
code looks like.

> 
>> The new SCSI_MAX_SG_SEGMENTS (your name by the way) is right there
>> and is calculated to maximize allocation in one page.
> 
> Yes, it's still a good idea to make sure you get good packing in the
> page, it's of course cheaper to have less chaining. But it's not
> critical in the same way as before, when it could impact IO layout and
> performance.
> 
>> (I guess the right name is SCSI_MAX_PHYS_SEGMENTS_IN_A_PAGE)
>> which will be needed in both your patches and mine.
> 
> That would be better name.
> 
>>>>  	blk_queue_max_hw_segments(q, shost->sg_tablesize);
>>>> -	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
>>>>  	blk_queue_max_sectors(q, shost->max_sectors);
>>>>
>> The reporting above is not needed and can be what ever block layer
>> considers safe/optimized.
> 
> You still need to set it, you can't just ignore it. Whether you redefine
> SCSI_MAX_SG_SEGMENTS or use (unsigned short) -1 doesn't really matter a
> whole lot.
> 
>>>> I'm working on a convergence patches that will do scsi_sg_pools cleanup
>>>> which is common to both our patches, than scsi_sgtable, and than 
>>>> sg-chaining on top of that. I hope it gets accepted. 
>>>> The sg-chaining is much much simpler over scsi_sgtables.
>>> Sorry, I don't follow this paragraph at all. What is the scsi_sgtables
>>> change you are referring to? And how does it make sg chaining so much
>>> simpler?
>>>
>>> I guess my problem is that I don't know what problem this scsi_sgtables
>>> you refer to is fixing?
>>>
>> scsi_sgtable is a solution proposed by James Bottomley where all I/O
>> members of struct scsi_cmnd and the resid member, which need to be
>> duplicated for bidirectional transfers, can be allocated together with
>> the sg-list they are pointing to. This way when bidi comes, the all
>> structure can be duplicated with minimal change to code, and with no
>> extra baggage when bidi is not used.  This was the all motivation for
>> the data accessors and cleanup, swiping through the entire scsi tree.
>> So when implementation changes drivers do not change with them. Now
>> meanwhile moving over drivers code we (well Tomo mostly) removed the
>> old !use_sg code path, and also abstracted the 2 major hot spots of
>> above usages with scsi_dma_{un,}map, and the scsi_for_each_sg.
>> Actually that one was changed from the original definition to match
>> you macro.
>>
>> Since scsi_sgtable is an encapsulation of the actual scatterlist array
>> together with the sg_count bufflen and pool_index, it gives code a
>> nice clean OO touch, and makes handling very easy, thats all. It only
>> simplifies things at the scsi-ml level.
> 
> So it's a pre-requisite for bidi support, it has no bearing on sg
> chaining. The only thing they have in common is that the touch some of
> the same code, that does not make them dependent or related beyond that
> necessarily.
> 

Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation

[Jens Axboe]

On Wed, Jul 18 2007, Benny Halevy wrote:
> Jens Axboe wrote:
> > On Wed, Jul 18 2007, Boaz Harrosh wrote:
> >> Jens Axboe wrote:
> >>> On Wed, Jul 18 2007, Boaz Harrosh wrote:
> >>>> FUJITA Tomonori wrote:
> >>>>> From: Mike Christie <michaelc@cs.wisc.edu>
> >>>>> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
> >>>>> Date: Thu, 12 Jul 2007 14:09:44 -0500
> >>>>>
> >>>>>> Boaz Harrosh wrote:
> >>>>>>> +/*
> >>>>>>> + * Should fit within a single page.
> >>>>>>> + */
> >>>>>>> +enum { SCSI_MAX_SG_SEGMENTS =
> >>>>>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> >>>>>>> +	sizeof(struct scatterlist)) };
> >>>>>>> +
> >>>>>>> +enum { SG_MEMPOOL_NR =
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
> >>>>>>> +};
> >>>>>>>  
> >>>>>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
> >>>>>> some other arch, we were going over a page when doing 
> >>>>>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
> >>>>> Seems that 170 with x86 and 127 with x86_64.
> >>>>>
> >>>> with scsi_sgtable we get one less than now
> >>>>
> >>>> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
> >>>> --------------------------|-------------------------|---------------------------
> >>>> x86_64                    | 127                     |32
> >>>> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
> >>>> i386 other                | 255                     |16
> >>>>
> >>>> What's nice about this code is that now finally it is
> >>>> automatically calculated in compile time. Arch people
> >>>> don't have the headache "did I break SCSI-ml?". 
> >>>> For example observe the current bug with i386 
> >>>> CONFIG_HIGHMEM64G=y.
> >>>>
> >>>> The same should be done with BIO's. Than ARCHs with big
> >>>> pages can gain even more.
> >>>>
> >>>>>> What happened to Jens's scatter list chaining and how does this relate 
> >>>>>> to it then?
> >>>>> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
> >>>>> want. We can remove the above code.
> >>>>>
> >>>>> We need to push this and Jens' sglist together in one merge window, I
> >>>>> think.
> >>>> No Tomo the above does not go away. What goes away is maybe:
> >>> It does go away, since we can just set it to some safe value and use
> >>> chaining to get us where we want.
> >> In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist
> >> anymore.
> > 
> > Sure, I could just kill it as well. The point is that it's a parallel
> > development, there's nothing in your patch that helps the sg chaining
> > whatsoever. The only "complex" thing in the SCSI layer for sg chaining,
> > is chaining when allocating and walking that chain on freeing. That's
> > it!
> 
> It seems like having the pool index in the sgtable structure simplifies
> the implementation a bit for allocation and freeing of linked sgtables.
> Boaz will send an example tomorrow (hopefully) showing how the merged
> code looks like.

The index stuff isn't complex, so I don't think you can call that a real
simplification. It's not for free either, there's a size cost to pay.

-- 
Jens Axboe

[PATCH] sgtable over sglist (Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation)

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Wed, 18 Jul 2007 22:17:10 +0200

> On Wed, Jul 18 2007, Benny Halevy wrote:
> > Jens Axboe wrote:
> > > On Wed, Jul 18 2007, Boaz Harrosh wrote:
> > >> Jens Axboe wrote:
> > >>> On Wed, Jul 18 2007, Boaz Harrosh wrote:
> > >>>> FUJITA Tomonori wrote:
> > >>>>> From: Mike Christie <michaelc@cs.wisc.edu>
> > >>>>> Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
> > >>>>> Date: Thu, 12 Jul 2007 14:09:44 -0500
> > >>>>>
> > >>>>>> Boaz Harrosh wrote:
> > >>>>>>> +/*
> > >>>>>>> + * Should fit within a single page.
> > >>>>>>> + */
> > >>>>>>> +enum { SCSI_MAX_SG_SEGMENTS =
> > >>>>>>> +	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
> > >>>>>>> +	sizeof(struct scatterlist)) };
> > >>>>>>> +
> > >>>>>>> +enum { SG_MEMPOOL_NR =
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 7) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 15) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 31) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 63) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 127) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 255) +
> > >>>>>>> +	(SCSI_MAX_SG_SEGMENTS >= 511)
> > >>>>>>> +};
> > >>>>>>>  
> > >>>>>> What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or 
> > >>>>>> some other arch, we were going over a page when doing 
> > >>>>>> SCSI_MAX_PHYS_SEGMENTS of 256 right?
> > >>>>> Seems that 170 with x86 and 127 with x86_64.
> > >>>>>
> > >>>> with scsi_sgtable we get one less than now
> > >>>>
> > >>>> Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
> > >>>> --------------------------|-------------------------|---------------------------
> > >>>> x86_64                    | 127                     |32
> > >>>> i386 CONFIG_HIGHMEM64G=y  | 204                     |20
> > >>>> i386 other                | 255                     |16
> > >>>>
> > >>>> What's nice about this code is that now finally it is
> > >>>> automatically calculated in compile time. Arch people
> > >>>> don't have the headache "did I break SCSI-ml?". 
> > >>>> For example observe the current bug with i386 
> > >>>> CONFIG_HIGHMEM64G=y.
> > >>>>
> > >>>> The same should be done with BIO's. Than ARCHs with big
> > >>>> pages can gain even more.
> > >>>>
> > >>>>>> What happened to Jens's scatter list chaining and how does this relate 
> > >>>>>> to it then?
> > >>>>> With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we
> > >>>>> want. We can remove the above code.
> > >>>>>
> > >>>>> We need to push this and Jens' sglist together in one merge window, I
> > >>>>> think.
> > >>>> No Tomo the above does not go away. What goes away is maybe:
> > >>> It does go away, since we can just set it to some safe value and use
> > >>> chaining to get us where we want.
> > >> In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist
> > >> anymore.
> > > 
> > > Sure, I could just kill it as well. The point is that it's a parallel
> > > development, there's nothing in your patch that helps the sg chaining
> > > whatsoever. The only "complex" thing in the SCSI layer for sg chaining,
> > > is chaining when allocating and walking that chain on freeing. That's
> > > it!
> > 
> > It seems like having the pool index in the sgtable structure simplifies
> > the implementation a bit for allocation and freeing of linked sgtables.
> > Boaz will send an example tomorrow (hopefully) showing how the merged
> > code looks like.
> 
> The index stuff isn't complex, so I don't think you can call that a real
> simplification. It's not for free either, there's a size cost to pay.

I think that the main issue of integrating sgtable and sglist is how
to put scatterlist to scsi_sgtable structure.

If we allocate a scsi_sgtable structure and sglists separately, the
code is pretty simple. But probably it's not the best way from the
perspective of performance.

If we put sglists into the scsi_sgtable structure (like Boaz's patch
does) and allocate and chain sglists only for large I/Os, we would
have the better performance (especially for small I/Os). But we will
have more complicated code.

I wrote my sgtable patch over Jens' sglist with the former way:

master.kernel.org:/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git sgtable


I also put Boaz's scsi_error, sd, and sr sgtable patches into the tree
so you can try it. I've tested this with only normal size I/Os (not
tested sglist code). I don't touch the sglist code much, so hopefully
it works.

I've attached the sgtable patch for review. It's against the
sglist-arch branch in Jens' tree.

---
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] move all the I/O descriptors to a new scsi_sgtable structure

based on Boaz Harrosh <bharrosh@panasas.com> scsi_sgtable patch.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/scsi_lib.c  |   92 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_cmnd.h |   39 +++++++++++++------
 2 files changed, 82 insertions(+), 49 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5fb1048..2fa1852 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -52,6 +52,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 };
 #undef SP
 
+static struct kmem_cache *scsi_sgtable_cache;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -731,16 +733,27 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask,
+					int sg_count)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
+	struct scsi_sgtable *sgt;
 	unsigned int index;
 	int this, left;
 
-	BUG_ON(!cmd->use_sg);
+	sgt = kmem_cache_zalloc(scsi_sgtable_cache, gfp_mask);
+	if (!sgt)
+		return NULL;
+
+	/*
+	 * don't allow subsequent mempool allocs to sleep, it would
+	 * violate the mempool principle.
+	 */
+	gfp_mask &= ~__GFP_WAIT;
+	gfp_mask |= __GFP_HIGH;
 
-	left = cmd->use_sg;
+	left = sg_count;
 	ret = prev = NULL;
 	do {
 		this = left;
@@ -764,7 +777,7 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		 * first loop through, set initial index and return value
 		 */
 		if (!ret) {
-			cmd->sglist_len = index;
+			sgt->sglist_len = index;
 			ret = sgl;
 		}
 
@@ -776,21 +789,18 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		if (prev)
 			sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
 
-		/*
-		 * don't allow subsequent mempool allocs to sleep, it would
-		 * violate the mempool principle.
-		 */
-		gfp_mask &= ~__GFP_WAIT;
-		gfp_mask |= __GFP_HIGH;
 		prev = sgl;
 	} while (left);
 
 	/*
-	 * ->use_sg may get modified after dma mapping has potentially
+	 * ->sg_count may get modified after dma mapping has potentially
 	 * shrunk the number of segments, so keep a copy of it for free.
 	 */
-	cmd->__use_sg = cmd->use_sg;
-	return ret;
+	sgt->sg_count = sg_count;
+	sgt->__sg_count = sg_count;
+	sgt->sglist = ret;
+	cmd->sgtable = sgt;
+	return sgt;
 enomem:
 	if (ret) {
 		/*
@@ -809,6 +819,8 @@ enomem:
 
 		mempool_free(prev, sgp->pool);
 	}
+	kmem_cache_free(scsi_sgtable_cache, sgt);
+
 	return NULL;
 }
 
@@ -816,21 +828,22 @@ EXPORT_SYMBOL(scsi_alloc_sgtable);
 
 void scsi_free_sgtable(struct scsi_cmnd *cmd)
 {
-	struct scatterlist *sgl = cmd->request_buffer;
+	struct scsi_sgtable *sgt = cmd->sgtable;
+	struct scatterlist *sgl = sgt->sglist;
 	struct scsi_host_sg_pool *sgp;
 
-	BUG_ON(cmd->sglist_len >= SG_MEMPOOL_NR);
+	BUG_ON(sgt->sglist_len >= SG_MEMPOOL_NR);
 
 	/*
 	 * if this is the biggest size sglist, check if we have
 	 * chained parts we need to free
 	 */
-	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
+	if (sgt->__sg_count > SCSI_MAX_SG_SEGMENTS) {
 		unsigned short this, left;
 		struct scatterlist *next;
 		unsigned int index;
 
-		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
+		left = sgt->__sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
 		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
 		while (left && next) {
 			sgl = next;
@@ -854,11 +867,12 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
 		/*
 		 * Restore original, will be freed below
 		 */
-		sgl = cmd->request_buffer;
+		sgl = sgt->sglist;
 	}
 
-	sgp = scsi_sg_pools + cmd->sglist_len;
+	sgp = scsi_sg_pools + sgt->sglist_len;
 	mempool_free(sgl, sgp->pool);
+	kmem_cache_free(scsi_sgtable_cache, sgt);
 }
 
 EXPORT_SYMBOL(scsi_free_sgtable);
@@ -882,15 +896,14 @@ EXPORT_SYMBOL(scsi_free_sgtable);
  */
 static void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
+	if (cmd->sgtable)
 		scsi_free_sgtable(cmd);
 
 	/*
 	 * Zero these out.  They now point to freed memory, and it is
 	 * dangerous to hang onto the pointers.
 	 */
-	cmd->request_buffer = NULL;
-	cmd->request_bufflen = 0;
+	cmd->sgtable = NULL;
 }
 
 /*
@@ -924,13 +937,14 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = cmd->request_bufflen;
+	int this_count = scsi_bufflen(cmd);
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;
+	int resid = scsi_get_resid(cmd);
 
 	scsi_release_buffers(cmd);
 
@@ -956,7 +970,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = resid;
 	}
 
 	/*
@@ -1098,6 +1112,7 @@ EXPORT_SYMBOL(scsi_io_completion);
 static int scsi_init_io(struct scsi_cmnd *cmd)
 {
 	struct request     *req = cmd->request;
+	struct scsi_sgtable *sgt;
 	int		   count;
 
 	/*
@@ -1105,35 +1120,36 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	 * but now we do (it makes highmem I/O easier to support without
 	 * kmapping pages)
 	 */
-	cmd->use_sg = req->nr_phys_segments;
+	sgt = scsi_alloc_sgtable(cmd, GFP_ATOMIC, req->nr_phys_segments);
 
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
-	if (unlikely(!cmd->request_buffer)) {
+	if (unlikely(!sgt)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
+	cmd->sgtable = sgt;
+
 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sgt->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sgt->length = req->nr_sectors << 9;
 
 	/* 
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
-	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
+	count = blk_rq_map_sg(req->q, req, cmd->sgtable->sglist);
+	if (likely(count <= sgt->sg_count)) {
+		sgt->sg_count = count;
 		return BLKPREP_OK;
 	}
 
 	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
+	printk(KERN_ERR "counted %d, received %d\n", count, sgt->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 
@@ -1189,7 +1205,7 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
 	 * successfully. Since this is a REQ_BLOCK_PC command the
 	 * caller should check the request's errors value
 	 */
-	scsi_io_completion(cmd, cmd->request_bufflen);
+	scsi_io_completion(cmd, scsi_bufflen(cmd));
 }
 
 static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
@@ -1218,9 +1234,7 @@ static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
 
-		cmd->request_bufflen = 0;
-		cmd->request_buffer = NULL;
-		cmd->use_sg = 0;
+		cmd->sgtable = NULL;
 		req->buffer = NULL;
 	}
 
@@ -1786,6 +1800,10 @@ int __init scsi_init_queue(void)
 		return -ENOMEM;
 	}
 
+	scsi_sgtable_cache = kmem_cache_create("scsi_sgtable",
+					       sizeof(struct scsi_sgtable),
+					       0, 0, NULL);
+
 	for (i = 0; i < SG_MEMPOOL_NR; i++) {
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
 		int size = sgp->size * sizeof(struct scatterlist);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 937b3c4..9ead940 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -11,6 +11,14 @@ struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
+struct scsi_sgtable {
+	unsigned length;
+	int resid;
+	short sg_count;
+	short __sg_count;
+	short sglist_len;
+	struct scatterlist *sglist;
+};
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
@@ -64,10 +72,9 @@ struct scsi_cmnd {
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16
 	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned request_bufflen;	/* Actual request size */
 
 	struct timer_list eh_timeout;	/* Used to time out the command. */
-	void *request_buffer;		/* Actual requested buffer */
+	struct scsi_sgtable *sgtable;
 
 	/* These elements define the operation we ultimately want to perform */
 	unsigned short use_sg;	/* Number of pieces of scatter-gather */
@@ -83,10 +90,6 @@ struct scsi_cmnd {
 				   reconnects.   Probably == sector
 				   size */
 
-	int resid;		/* Number of bytes requested to be
-				   transferred less actual number
-				   transferred (0 if not supported) */
-
 	struct request *request;	/* The command we are
 				   	   working on */
 
@@ -133,24 +136,36 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
+extern struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t, int);
 extern void scsi_free_sgtable(struct scsi_cmnd *);
 
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->sg_count : 0;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->sglist : 0;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->length : 0;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	if (cmd->sgtable)
+		cmd->sgtable->resid = resid;
 }
 
 static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 {
-	return cmd->resid;
+	return cmd->sgtable ? cmd->sgtable->resid : 0;
 }
 
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
-- 
1.5.2.4

Re: [PATCH] sgtable over sglist (Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation)

[Boaz Harrosh]

Comments about this patch embedded inside

FUJITA Tomonori wrote:
> 
> I've attached the sgtable patch for review. It's against the
> sglist-arch branch in Jens' tree.
> 
> ---
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] move all the I/O descriptors to a new scsi_sgtable structure
> 
> based on Boaz Harrosh <bharrosh@panasas.com> scsi_sgtable patch.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/scsi_lib.c  |   92 +++++++++++++++++++++++++++------------------
>  include/scsi/scsi_cmnd.h |   39 +++++++++++++------
>  2 files changed, 82 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5fb1048..2fa1852 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -52,6 +52,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
>  };
>  #undef SP
>  
> +static struct kmem_cache *scsi_sgtable_cache;
> +
One more slab pool to do regular IO

>  static void scsi_run_queue(struct request_queue *q);
>  
>  /*
> @@ -731,16 +733,27 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
>  	return index;
>  }
>  
> -struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> +struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask,
> +					int sg_count)
>  {
>  	struct scsi_host_sg_pool *sgp;
>  	struct scatterlist *sgl, *prev, *ret;
> +	struct scsi_sgtable *sgt;
>  	unsigned int index;
>  	int this, left;
>  
> -	BUG_ON(!cmd->use_sg);
> +	sgt = kmem_cache_zalloc(scsi_sgtable_cache, gfp_mask);
> +	if (!sgt)
> +		return NULL;
One more allocation that can fail for every io. Even if we have
a scsi_cmnd.

> +
> +	/*
> +	 * don't allow subsequent mempool allocs to sleep, it would
> +	 * violate the mempool principle.
> +	 */
> +	gfp_mask &= ~__GFP_WAIT;
> +	gfp_mask |= __GFP_HIGH;
We used to sometime wait for that Large 128 scatterlist full page.
But now this small allocation is probably good. but we no longer
wait for the big allocation below.

>  
> -	left = cmd->use_sg;
> +	left = sg_count;
>  	ret = prev = NULL;
>  	do {
>  		this = left;
> @@ -764,7 +777,7 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
>  		 * first loop through, set initial index and return value
>  		 */
>  		if (!ret) {
> -			cmd->sglist_len = index;
> +			sgt->sglist_len = index;
>  			ret = sgl;
>  		}
>  
> @@ -776,21 +789,18 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
>  		if (prev)
>  			sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
>  
> -		/*
> -		 * don't allow subsequent mempool allocs to sleep, it would
> -		 * violate the mempool principle.
> -		 */
> -		gfp_mask &= ~__GFP_WAIT;
> -		gfp_mask |= __GFP_HIGH;
>  		prev = sgl;
>  	} while (left);
>  
>  	/*
> -	 * ->use_sg may get modified after dma mapping has potentially
> +	 * ->sg_count may get modified after dma mapping has potentially
>  	 * shrunk the number of segments, so keep a copy of it for free.
>  	 */
> -	cmd->__use_sg = cmd->use_sg;
> -	return ret;
> +	sgt->sg_count = sg_count;
> +	sgt->__sg_count = sg_count;
> +	sgt->sglist = ret;
> +	cmd->sgtable = sgt;
We used to set that in scsi_init_io. So this scsi_alloc_sgtable()
can be called twice for bidi. Now we can not. Also the API change
is not friendly for bidi and will have to change. (It used to be good)
If you set it here why do you return it.

> +	return sgt;
>  enomem:
>  	if (ret) {
>  		/*
> @@ -809,6 +819,8 @@ enomem:
>  
>  		mempool_free(prev, sgp->pool);
>  	}
> +	kmem_cache_free(scsi_sgtable_cache, sgt);
> +
>  	return NULL;
>  }
>  
> @@ -816,21 +828,22 @@ EXPORT_SYMBOL(scsi_alloc_sgtable);
>  
>  void scsi_free_sgtable(struct scsi_cmnd *cmd)
>  {
> -	struct scatterlist *sgl = cmd->request_buffer;
> +	struct scsi_sgtable *sgt = cmd->sgtable;
> +	struct scatterlist *sgl = sgt->sglist;
>  	struct scsi_host_sg_pool *sgp;
>  
> -	BUG_ON(cmd->sglist_len >= SG_MEMPOOL_NR);
> +	BUG_ON(sgt->sglist_len >= SG_MEMPOOL_NR);
>  
>  	/*
>  	 * if this is the biggest size sglist, check if we have
>  	 * chained parts we need to free
>  	 */
> -	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
> +	if (sgt->__sg_count > SCSI_MAX_SG_SEGMENTS) {
>  		unsigned short this, left;
>  		struct scatterlist *next;
>  		unsigned int index;
>  
> -		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
> +		left = sgt->__sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
>  		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
>  		while (left && next) {
>  			sgl = next;
> @@ -854,11 +867,12 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
>  		/*
>  		 * Restore original, will be freed below
>  		 */
> -		sgl = cmd->request_buffer;
> +		sgl = sgt->sglist;
>  	}
>  
> -	sgp = scsi_sg_pools + cmd->sglist_len;
> +	sgp = scsi_sg_pools + sgt->sglist_len;
>  	mempool_free(sgl, sgp->pool);
> +	kmem_cache_free(scsi_sgtable_cache, sgt);
>  }
>  
>  EXPORT_SYMBOL(scsi_free_sgtable);
> @@ -882,15 +896,14 @@ EXPORT_SYMBOL(scsi_free_sgtable);
>   */
>  static void scsi_release_buffers(struct scsi_cmnd *cmd)
>  {
> -	if (cmd->use_sg)
> +	if (cmd->sgtable)
>  		scsi_free_sgtable(cmd);
>  
>  	/*
>  	 * Zero these out.  They now point to freed memory, and it is
>  	 * dangerous to hang onto the pointers.
>  	 */
> -	cmd->request_buffer = NULL;
> -	cmd->request_bufflen = 0;
> +	cmd->sgtable = NULL;
>  }
>  
>  /*
> @@ -924,13 +937,14 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  {
>  	int result = cmd->result;
> -	int this_count = cmd->request_bufflen;
> +	int this_count = scsi_bufflen(cmd);
>  	request_queue_t *q = cmd->device->request_queue;
>  	struct request *req = cmd->request;
>  	int clear_errors = 1;
>  	struct scsi_sense_hdr sshdr;
>  	int sense_valid = 0;
>  	int sense_deferred = 0;
> +	int resid = scsi_get_resid(cmd);
>  
>  	scsi_release_buffers(cmd);
>  
> @@ -956,7 +970,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  				req->sense_len = len;
>  			}
>  		}
> -		req->data_len = cmd->resid;
> +		req->data_len = resid;
>  	}
>  
>  	/*
> @@ -1098,6 +1112,7 @@ EXPORT_SYMBOL(scsi_io_completion);
>  static int scsi_init_io(struct scsi_cmnd *cmd)
>  {
>  	struct request     *req = cmd->request;
> +	struct scsi_sgtable *sgt;
>  	int		   count;
>  
>  	/*
> @@ -1105,35 +1120,36 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
>  	 * but now we do (it makes highmem I/O easier to support without
>  	 * kmapping pages)
>  	 */
> -	cmd->use_sg = req->nr_phys_segments;
> +	sgt = scsi_alloc_sgtable(cmd, GFP_ATOMIC, req->nr_phys_segments);
>  
>  	/*
>  	 * If sg table allocation fails, requeue request later.
>  	 */
> -	cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
> -	if (unlikely(!cmd->request_buffer)) {
> +	if (unlikely(!sgt)) {
>  		scsi_unprep_request(req);
>  		return BLKPREP_DEFER;
>  	}
>  
> +	cmd->sgtable = sgt;
> +
Set second time

>  	req->buffer = NULL;
>  	if (blk_pc_request(req))
> -		cmd->request_bufflen = req->data_len;
> +		sgt->length = req->data_len;
>  	else
> -		cmd->request_bufflen = req->nr_sectors << 9;
> +		sgt->length = req->nr_sectors << 9;
>  
>  	/* 
>  	 * Next, walk the list, and fill in the addresses and sizes of
>  	 * each segment.
>  	 */
> -	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
> -	if (likely(count <= cmd->use_sg)) {
> -		cmd->use_sg = count;
> +	count = blk_rq_map_sg(req->q, req, cmd->sgtable->sglist);
> +	if (likely(count <= sgt->sg_count)) {
> +		sgt->sg_count = count;
>  		return BLKPREP_OK;
>  	}
>  
>  	printk(KERN_ERR "Incorrect number of segments after building list\n");
> -	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
> +	printk(KERN_ERR "counted %d, received %d\n", count, sgt->sg_count);
>  	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>  			req->current_nr_sectors);
>  
> @@ -1189,7 +1205,7 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
>  	 * successfully. Since this is a REQ_BLOCK_PC command the
>  	 * caller should check the request's errors value
>  	 */
> -	scsi_io_completion(cmd, cmd->request_bufflen);
> +	scsi_io_completion(cmd, scsi_bufflen(cmd));
>  }
>  
>  static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> @@ -1218,9 +1234,7 @@ static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>  		BUG_ON(req->data_len);
>  		BUG_ON(req->data);
>  
> -		cmd->request_bufflen = 0;
> -		cmd->request_buffer = NULL;
> -		cmd->use_sg = 0;
> +		cmd->sgtable = NULL;
>  		req->buffer = NULL;
>  	}
>  
> @@ -1786,6 +1800,10 @@ int __init scsi_init_queue(void)
>  		return -ENOMEM;
>  	}
>  
> +	scsi_sgtable_cache = kmem_cache_create("scsi_sgtable",
> +					       sizeof(struct scsi_sgtable),
> +					       0, 0, NULL);
> +
Extra pool allocation

>  	for (i = 0; i < SG_MEMPOOL_NR; i++) {
>  		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
>  		int size = sgp->size * sizeof(struct scatterlist);
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 937b3c4..9ead940 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -11,6 +11,14 @@ struct scatterlist;
>  struct Scsi_Host;
>  struct scsi_device;
>  
> +struct scsi_sgtable {
> +	unsigned length;
> +	int resid;
> +	short sg_count;
> +	short __sg_count;
> +	short sglist_len;
> +	struct scatterlist *sglist;
> +};
This is not a table it is Just that plain old scsi_data_buff.
sgtable is meant to say an header immediately followed by an
sg-array. What makes it a table is the: 
"struct scatterlist sglist[0];" At the end

>  
>  /* embedded in scsi_cmnd */
>  struct scsi_pointer {
> @@ -64,10 +72,9 @@ struct scsi_cmnd {
>  	/* These elements define the operation we are about to perform */
>  #define MAX_COMMAND_SIZE	16
>  	unsigned char cmnd[MAX_COMMAND_SIZE];
> -	unsigned request_bufflen;	/* Actual request size */
>  
>  	struct timer_list eh_timeout;	/* Used to time out the command. */
> -	void *request_buffer;		/* Actual requested buffer */
> +	struct scsi_sgtable *sgtable;
>  
>  	/* These elements define the operation we ultimately want to perform */
>  	unsigned short use_sg;	/* Number of pieces of scatter-gather */
> @@ -83,10 +90,6 @@ struct scsi_cmnd {
>  				   reconnects.   Probably == sector
>  				   size */
>  
> -	int resid;		/* Number of bytes requested to be
> -				   transferred less actual number
> -				   transferred (0 if not supported) */
> -
>  	struct request *request;	/* The command we are
>  				   	   working on */
>  
You only removed 3 members but scsi_sgtable as 6

> @@ -133,24 +136,36 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
>  				 size_t *offset, size_t *len);
>  extern void scsi_kunmap_atomic_sg(void *virt);
>  
> -extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
> +extern struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t, int);
>  extern void scsi_free_sgtable(struct scsi_cmnd *);
>  
>  extern int scsi_dma_map(struct scsi_cmnd *cmd);
>  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>  
> -#define scsi_sg_count(cmd) ((cmd)->use_sg)
> -#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
> -#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
> +static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sgtable ? cmd->sgtable->sg_count : 0;
> +}
> +
> +static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sgtable ? cmd->sgtable->sglist : 0;
> +}
> +
> +static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sgtable ? cmd->sgtable->length : 0;
> +}
>  
>  static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
>  {
> -	cmd->resid = resid;
> +	if (cmd->sgtable)
> +		cmd->sgtable->resid = resid;
>  }
>  
>  static inline int scsi_get_resid(struct scsi_cmnd *cmd)
>  {
> -	return cmd->resid;
> +	return cmd->sgtable ? cmd->sgtable->resid : 0;
>  }
>  
>  #define scsi_for_each_sg(cmd, sg, nseg, __i)			\

Please see my scsi_data_buff patches in that other
thread they solve all thses problems.

Boaz

Re: [RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation

[FUJITA Tomonori]

From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation
Date: Thu, 05 Jul 2007 16:44:04 +0300

> 
>   - sd and sr would adjust IO size to align on device's block
>     size. So code needs to change once we move to scsi_sgtable
>     implementation. (Not compatible with un-converted drivers)
>   - Convert code to use scsi_for_each_sg
>   - Use Data accessors wherever is appropriate.
> 
>  Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/sd.c |   10 ++++------
>  drivers/scsi/sr.c |   27 ++++++++++++---------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 448d316..459fd23 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -338,7 +338,7 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>  	struct request *rq = SCpnt->request;
>  	struct gendisk *disk = rq->rq_disk;
>  	sector_t block = rq->sector;
> -	unsigned int this_count = SCpnt->request_bufflen >> 9;
> +	unsigned int this_count = scsi_bufflen(SCpnt) >> 9;
>  	unsigned int timeout = sdp->timeout;
>  
>  	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
> @@ -418,9 +418,6 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>  	} else if (rq_data_dir(rq) == READ) {
>  		SCpnt->cmnd[0] = READ_6;
>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> -	} else {
> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
> -		return 0;
>  	}

Why?


>  	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
> @@ -480,7 +477,8 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>  		SCpnt->cmnd[4] = (unsigned char) this_count;
>  		SCpnt->cmnd[5] = 0;
>  	}
> -	SCpnt->request_bufflen = this_count * sdp->sector_size;
> +	BUG_ON(!SCpnt->sgtable);
> +	SCpnt->sgtable->length = this_count * sdp->sector_size;
>  
>  	/*
>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
> @@ -892,7 +890,7 @@ static struct block_device_operations sd_fops = {
>  static void sd_rw_intr(struct scsi_cmnd * SCpnt)
>  {
>  	int result = SCpnt->result;
> - 	unsigned int xfer_size = SCpnt->request_bufflen;
> + 	unsigned int xfer_size = scsi_bufflen(SCpnt);
>   	unsigned int good_bytes = result ? 0 : xfer_size;
>   	u64 start_lba = SCpnt->request->sector;
>   	u64 bad_lba;
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index f9a52af..ed61ca9 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -218,7 +218,7 @@ int sr_media_change(struct cdrom_device_info *cdi, int slot)
>  static void rw_intr(struct scsi_cmnd * SCpnt)
>  {
>  	int result = SCpnt->result;
> -	int this_count = SCpnt->request_bufflen;
> +	int this_count = scsi_bufflen(SCpnt);
>  	int good_bytes = (result == 0 ? this_count : 0);
>  	int block_sectors = 0;
>  	long error_sector;
> @@ -345,23 +345,20 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
>  	} else if (rq_data_dir(SCpnt->request) == READ) {
>  		SCpnt->cmnd[0] = READ_10;
>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> -	} else {
> -		blk_dump_rq_flags(SCpnt->request, "Unknown sr command");
> -		return 0;
>  	}

ditto.

Re: [RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation

[Benny Halevy]

FUJITA Tomonori wrote:
> From: Boaz Harrosh <bharrosh@panasas.com>
> Subject: [RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation
> Date: Thu, 05 Jul 2007 16:44:04 +0300
> 
>>   - sd and sr would adjust IO size to align on device's block
>>     size. So code needs to change once we move to scsi_sgtable
>>     implementation. (Not compatible with un-converted drivers)
>>   - Convert code to use scsi_for_each_sg
>>   - Use Data accessors wherever is appropriate.
>>
>>  Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  drivers/scsi/sd.c |   10 ++++------
>>  drivers/scsi/sr.c |   27 ++++++++++++---------------
>>  2 files changed, 16 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 448d316..459fd23 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -338,7 +338,7 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>>  	struct request *rq = SCpnt->request;
>>  	struct gendisk *disk = rq->rq_disk;
>>  	sector_t block = rq->sector;
>> -	unsigned int this_count = SCpnt->request_bufflen >> 9;
>> +	unsigned int this_count = scsi_bufflen(SCpnt) >> 9;
>>  	unsigned int timeout = sdp->timeout;
>>  
>>  	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
>> @@ -418,9 +418,6 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>>  	} else if (rq_data_dir(rq) == READ) {
>>  		SCpnt->cmnd[0] = READ_6;
>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -	} else {
>> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
>> -		return 0;
>>  	}
> 
> Why?

This seems to be dead code as rq_data_dir can only be either READ or WRITE.
The else case could be a BUG() but I don't see why it should be handled
gracefully.

> 
> 
>>  	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
>> @@ -480,7 +477,8 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
>>  		SCpnt->cmnd[4] = (unsigned char) this_count;
>>  		SCpnt->cmnd[5] = 0;
>>  	}
>> -	SCpnt->request_bufflen = this_count * sdp->sector_size;
>> +	BUG_ON(!SCpnt->sgtable);
>> +	SCpnt->sgtable->length = this_count * sdp->sector_size;
>>  
>>  	/*
>>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
>> @@ -892,7 +890,7 @@ static struct block_device_operations sd_fops = {
>>  static void sd_rw_intr(struct scsi_cmnd * SCpnt)
>>  {
>>  	int result = SCpnt->result;
>> - 	unsigned int xfer_size = SCpnt->request_bufflen;
>> + 	unsigned int xfer_size = scsi_bufflen(SCpnt);
>>   	unsigned int good_bytes = result ? 0 : xfer_size;
>>   	u64 start_lba = SCpnt->request->sector;
>>   	u64 bad_lba;
>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>> index f9a52af..ed61ca9 100644
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -218,7 +218,7 @@ int sr_media_change(struct cdrom_device_info *cdi, int slot)
>>  static void rw_intr(struct scsi_cmnd * SCpnt)
>>  {
>>  	int result = SCpnt->result;
>> -	int this_count = SCpnt->request_bufflen;
>> +	int this_count = scsi_bufflen(SCpnt);
>>  	int good_bytes = (result == 0 ? this_count : 0);
>>  	int block_sectors = 0;
>>  	long error_sector;
>> @@ -345,23 +345,20 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
>>  	} else if (rq_data_dir(SCpnt->request) == READ) {
>>  		SCpnt->cmnd[0] = READ_10;
>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -	} else {
>> -		blk_dump_rq_flags(SCpnt->request, "Unknown sr command");
>> -		return 0;
>>  	}
> 
> ditto.

ditto :)

> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html