Last 3 patches for bidi support

[1]
I propose a small change to scsi_tgt_lib.c that will make tgt completely neutral to the scsi_data_buffer patch. And will make it all that more ready for bidi, too. TOMO is this OK?

(Can you do without the GFP_KERNEL allocation flag? It could make the code a bit more simple)

Quoted 8 lines · Boaz Harrosh

[1]
I propose a small change to scsi_tgt_lib.c that will make tgt completely neutral to the scsi_data_buffer patch. And will make it all that more ready for bidi, too. TOMO is this OK?

(Can you do without the GFP_KERNEL allocation flag? It could make the code a bit more simple)

GFP_KERNEL is nice for the target layer because it can sleep in that path you changed and it and does not have the "cannot write out pages because it may come back to the same device issues" like an initiator does.

If we ever changed to a softirq instead of the work queue then we would not need the flag since it would have to GFP_ATOMIC, but I am not sure if we have plans to do that anytime soon.

Quoted 14 lines · Mike Christie

Quoted 8 lines · Boaz Harrosh

[1]
I propose a small change to scsi_tgt_lib.c that will make tgt completely neutral to the scsi_data_buffer patch. And will make it all that more ready for bidi, too. TOMO is this OK?

(Can you do without the GFP_KERNEL allocation flag? It could make the code a bit more simple)

GFP_KERNEL is nice for the target layer because it can sleep in that path you changed and it and does not have the "cannot write out pages because it may come back to the same device issues" like an initiator does.

Quoted 3 lines

If we ever changed to a softirq instead of the work queue then we would not need the flag since it would have to GFP_ATOMIC, but I am not sure if we have plans to do that anytime soon.

Yes I understand that, hence the GFP_KERNEL was kept intact in my patch. But I was thinking perhaps it was possible to sleep outside, if the return value was BLKPREP_DEFER, the way the block layer sleeps, just not in allocation stage.

- If we export scsi_init_io()/scsi_release_buffers() instead of scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is much more insulated from scsi_lib changes. As a bonus it will also gain bidi capability when it comes.

Signed-off-by Boaz Harrosh <[email protected]>

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.
- Adjust accessors to new members.
- scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of scsi_cmnd. And work on it.
- Adjust scsi_init_io() and scsi_release_buffers() for above change.
- Fix other parts of scsi_lib/scsi.c to members migration. Use accessors where appropriate.

- fix Documentation about scsi_cmnd in scsi_host.h

- scsi_error.c
* Changed needed members of struct scsi_eh_save.
* Careful considerations in scsi_eh_prep/restore_cmnd.

- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block size so code needs to change once we move to scsi_data_buff implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
* Remove dead code (req_data_dir() != READ && != WRITE)

- tgt: convert libsrp to use scsi_data_buffer
- isd200: This driver still bangs on scsi_cmnd IO members, so need changing

Signed-off-by Boaz Harrosh <[email protected]>

Signed-off-by FUJITA Tomonori <[email protected]>

drivers/scsi/libsrp.c +2 -2

index 5cff020..8a8562a 100644
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 
 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
 				     cmd->tag);

drivers/scsi/scsi.c +1 -1

index 1929488..73d2216 100644
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = cmd->request_bufflen;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)

drivers/scsi/scsi_error.c +10 -18

index ebaca4c..e7b87ea 100644
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	ses->cmd_len = scmd->cmd_len;
 	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	ses->data_direction = scmd->sc_data_direction;
-	ses->bufflen = scmd->request_bufflen;
-	ses->buffer = scmd->request_buffer;
-	ses->use_sg = scmd->use_sg;
-	ses->resid = scmd->resid;
+	ses->sdb = scmd->sdb;
 	ses->result = scmd->result;
 
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
 	if (sense_bytes) {
-		scmd->request_bufflen = min_t(unsigned,
+		scmd->sdb.length = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), sense_bytes);
 		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
-		                                       scmd->request_bufflen);
-		scmd->request_buffer = &ses->sense_sgl;
+		                                       scmd->sdb.length);
+		scmd->sdb.sglist = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->use_sg = 1;
+		scmd->sdb.sg_count = 1;
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 		scmd->cmnd[0] = REQUEST_SENSE;
-		scmd->cmnd[4] = scmd->request_bufflen;
+		scmd->cmnd[4] = scmd->sdb.length;
 		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	} else {
-		scmd->request_buffer = NULL;
-		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
-		scmd->use_sg = 0;
 		if (cmnd) {
 			memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 			memcpy(scmd->cmnd, cmnd, cmnd_size);

@@ -676,10 +672,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	scmd->cmd_len = ses->cmd_len;
 	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = ses->data_direction;
-	scmd->request_bufflen = ses->bufflen;
-	scmd->request_buffer = ses->buffer;
-	scmd->use_sg = ses->use_sg;
-	scmd->resid = ses->resid;
+	scmd->sdb = ses->sdb;
 	scmd->result = ses->result;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);

@@ -1699,8 +1692,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	memset(&scmd->cmnd, '\0', sizeof(scmd->cmnd));
 
 	scmd->scsi_done		= scsi_reset_provider_done_command;
-	scmd->request_buffer		= NULL;
-	scmd->request_bufflen		= 0;
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
 
 	scmd->cmd_len			= 0;
 

drivers/scsi/scsi_lib.c +7 -8

index a8bf7cb..f904692 100644
@@ -438,7 +438,7 @@ EXPORT_SYMBOL_GPL(scsi_execute_async);
 static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
 {
 	cmd->serial_number = 0;
-	cmd->resid = 0;
+	scsi_set_resid(cmd, 0);
 	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
 	if (cmd->cmd_len == 0)
 		cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);

@@ -737,17 +737,15 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
-                                              gfp_t gfp_mask)
+static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
+                                       unsigned short sg_count, gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
 	unsigned int index;
 	int this, left;
 
-	BUG_ON(!cmd->use_sg);
-
-	left = cmd->use_sg;
+	left = sg_count;
 	ret = prev = NULL;
 	do {
 		this = left;

@@ -801,8 +799,9 @@ static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
 	 * ->use_sg 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;
+	sdb->alloc_sg_count = sdb->sg_count = sg_count;
+	sdb->sglist = ret;
+	return 0;
 enomem:
 	if (ret) {
 		/*

drivers/scsi/scsi_lib.c +14 -22

 }
 
-static void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 {
-	struct scatterlist *sgl = cmd->request_buffer;
+	struct scatterlist *sgl = sdb->sglist;
 	struct scsi_host_sg_pool *sgp;
 
 	/*
 	 * 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 (sdb->alloc_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 = sdb->alloc_sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
 		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
 		while (left && next) {
 			sgl = next;

@@ -862,10 +861,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
 		/*
 		 * Restore original, will be freed below
 		 */
-		sgl = cmd->request_buffer;
+		sgl = sdb->sglist;
 		sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
 	} else
-		sgp = scsi_sg_pools + scsi_sgtable_index(cmd->__use_sg);
+		sgp = scsi_sg_pools + scsi_sgtable_index(sdb->alloc_sg_count);
 
 	mempool_free(sgl, sgp->pool);
 }

@@ -889,15 +888,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
-		scsi_free_sgtable(cmd);
+	if (cmd->sdb.sglist)
+		scsi_free_sgtable(&cmd->sdb);
 
-	/*
-	 * 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;
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 

@@ -932,7 +926,7 @@ EXPORT_SYMBOL(scsi_release_buffers);
 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);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;

@@ -940,8 +934,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	int sense_valid = 0;
 	int sense_deferred = 0;
 
-	scsi_release_buffers(cmd);
-
 	if (result) {
 		sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
 		if (sense_valid)

@@ -964,9 +956,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = scsi_get_resid(cmd);
 	}
 
+	scsi_release_buffers(cmd);
+
 	/*
 	 * Next deal with any sectors which we were able to correctly
 	 * handle.

@@ -974,7 +968,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;

@@ -1106,41 +1099,31 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct request     *req = cmd->request;
 	int		   count;
+	struct scsi_data_buffer *sdb = &cmd->sdb;
 
-	/*
-	 * We used to not use scatter-gather for single segment request,
-	 * but now we do (it makes highmem I/O easier to support without

drivers/scsi/scsi_lib.c +7 -9

 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sdb->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sdb->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, sdb->sglist);
+	if (likely(count <= sdb->sg_count)) {
+		sdb->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, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 

@@ -1199,9 +1182,7 @@ 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;
+		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 		req->buffer = NULL;
 	}
 

drivers/scsi/sd.c +0 -0

index 18343a6..28cf6fe 100644

drivers/scsi/sd.c +2 -2

 	}
 
 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,

@@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	SCpnt->sdb.length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb

@@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
 static int sd_done(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;

drivers/scsi/sr.c +1 -1

index 7702681..6d3bf41 100644
@@ -226,7 +226,7 @@ out:
 static int sr_done(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;

drivers/scsi/sr.c +12 -11

 	}
 
 	{
-		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 != scsi_bufflen(SCpnt)) {
 			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, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 

@@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}
 
-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 
 
 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",

@@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;

drivers/usb/storage/isd200.c +4 -4

index 178e8c2..2d9a32b 100644
@@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
 		sg_init_one(&info->sg, buff, bufflen);
 
 	srb->sc_data_direction = dir;
-	srb->request_buffer = buff ? &info->sg : NULL;
-	srb->request_bufflen = bufflen;
-	srb->use_sg = buff ? 1 : 0;
+	srb->sdb.sglist = buff ? &info->sg : NULL;
+	srb->sdb.length = bufflen;
+	srb->sdb.sg_count = buff ? 1 : 0;
 }
 
 static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
 {
-	srb->request_bufflen = bufflen;
+	srb->sdb.length = bufflen;
 }
 
 

include/scsi/scsi_cmnd.h +0 -0

index a7be605..4e3cf43 100644

include/scsi/scsi_cmnd.h +20 -14

+	struct scatterlist* sglist;
+};
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {

@@ -62,15 +69,10 @@ 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 */
 
 	/* These elements define the operation we ultimately want to perform */
-	unsigned short use_sg;	/* Number of pieces of scatter-gather */
-	unsigned short __use_sg;
-
 	unsigned underflow;	/* Return error if less than
 				   this amount is transferred */
 

@@ -80,10 +82,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 */
 

@@ -114,6 +112,8 @@ struct scsi_cmnd {
 	int result;		/* Status code from lower level driver */
 
 	unsigned char tag;	/* SCSI-II queued command tag */
+
+	struct scsi_data_buffer sdb;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);

@@ -134,18 +134,29 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 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->sdb.sg_count;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.sglist;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.length;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	cmd->sdb.resid = resid;
 }
 
 static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 {
-	return cmd->resid;
+	return cmd->sdb.resid;
 }
 
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\

include/scsi/scsi_eh.h +3 -5

index d21b891..1e08be1 100644
@@ -68,16 +68,14 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 extern int scsi_reset_provider(struct scsi_device *, int);
 
 struct scsi_eh_save {
+	/* saved state */
 	int result;
 	enum dma_data_direction data_direction;
 	unsigned char cmd_len;
 	unsigned char cmnd[MAX_COMMAND_SIZE];
+	struct scsi_data_buffer sdb;
 
-	void *buffer;
-	unsigned bufflen;
-	unsigned short use_sg;
-	int resid;
-
+	/* new command support */
 	struct scatterlist sense_sgl;
 };
 

include/scsi/scsi_host.h +2 -2

index 0fd4746..cb2bcab 100644
@@ -136,9 +136,9 @@ struct scsi_host_template {
 	 * the done callback is invoked.
 	 *
 	 * This is called to inform the LLD to transfer
-	 * cmd->request_bufflen bytes. The cmd->use_sg speciefies the
+	 * scsi_bufflen(cmd) bytes. scsi_sg_count(cmd) speciefies the
 	 * number of scatterlist entried in the command and
-	 * cmd->request_buffer contains the scatterlist.
+	 * scsi_sglist(cmd) returns the scatterlist.
 	 *
 	 * return values: see queuecommand
 	 *

Signature · 7 lines

1.5.3.1


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

Quoted 5 lines · On Tue, 06 Nov 2007 20:16:19 +0200 Boaz Harrosh <[email protected]> wrote:

- If we export scsi_init_io()/scsi_release_buffers() instead of scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is much more insulated from scsi_lib changes. As a bonus it will also gain bidi capability when it comes.

Quoted 1 line

Signed-off-by Boaz Harrosh <[email protected]>

Looks good for me except for this:

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
ERROR: use tabs not spaces
#101: FILE: drivers/scsi/scsi_lib.c:741:
+ gfp_t gfp_mask)$

total: 1 errors, 0 warnings, 144 lines checked

On Tue, 06 Nov 2007 20:19:32 +0200
Boaz Harrosh <[email protected]> wrote:

Hmm, checkpatch.pl complains reasonably:

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
ERROR: use tabs not spaces
#177: FILE: drivers/scsi/scsi_error.c:629:
+^I^I scmd->sdb.length);$

ERROR: use tabs not spaces
#237: FILE: drivers/scsi/scsi_lib.c:741:
+ unsigned short sg_count, gfp_t gfp_mask)$

WARNING: no space between function name and open parenthesis '(' #487: FILE: drivers/scsi/sr.c:377:
+ scsi_for_each_sg (SCpnt, sg, sg_count, i)

ERROR: "foo* bar" should be "foo *bar"
#563: FILE: include/scsi/scsi_cmnd.h:20:
+ struct scatterlist* sglist;

total: 3 errors, 1 warnings, 482 lines checked

drivers/scsi/scsi_lib.c enomem: · line 823

Quoted 6 lines

drivers/scsi/scsi_lib.c +1 -1

@@ -821,24 +820,24 @@ enomem:
 
 		mempool_free(prev, sgp->pool);
 	}
-	return NULL;
+	return -1;

I think that -ENOMEM is better. The other functions in scsi_lib.c (even static functions) use proper error values.

drivers/scsi/scsi_lib.c int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask) · line 1104

Quoted 6 lines

drivers/scsi/scsi_lib.c +1 -5

-	/*
-	 * If sg table allocation fails, requeue request later.
-	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (unlikely(!cmd->request_buffer)) {
+	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {

IIRC, preferable style is:

	ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
	if (ret) {

Quoted 42 lines

drivers/scsi/scsi_lib.c +7 -9

 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sdb->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sdb->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, sdb->sglist);
+	if (likely(count <= sdb->sg_count)) {
+		sdb->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, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);

@@ -1199,9 +1182,7 @@ 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;
+		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 		req->buffer = NULL;
 	}
 

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 453

Quoted 11 lines

drivers/scsi/sd.c +0 -3

index 18343a6..28cf6fe 100644
@@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} 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);
-		goto out;

This should go to the bidi patch?

Quoted 2 lines

drivers/scsi/sd.c +0 -0

 	}
 

Quoted 33 lines

drivers/scsi/sd.c +2 -2

 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,

@@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	SCpnt->sdb.length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb

@@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
 static int sd_done(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;

drivers/scsi/sr.c +1 -1

index 7702681..6d3bf41 100644
@@ -226,7 +226,7 @@ out:
 static int sr_done(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;

@@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 373

Quoted 6 lines

drivers/scsi/sr.c +0 -3

 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;

Ditto.

Quoted 79 lines

drivers/scsi/sr.c +12 -11

 	}
 
 	{
-		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 != scsi_bufflen(SCpnt)) {
 			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, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 

@@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}
 
-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 
 
 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",

@@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;

drivers/usb/storage/isd200.c +4 -4

index 178e8c2..2d9a32b 100644
@@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
 		sg_init_one(&info->sg, buff, bufflen);
 
 	srb->sc_data_direction = dir;
-	srb->request_buffer = buff ? &info->sg : NULL;
-	srb->request_bufflen = bufflen;
-	srb->use_sg = buff ? 1 : 0;
+	srb->sdb.sglist = buff ? &info->sg : NULL;
+	srb->sdb.length = bufflen;
+	srb->sdb.sg_count = buff ? 1 : 0;
 }
 
 static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
 {
-	srb->request_bufflen = bufflen;
+	srb->sdb.length = bufflen;
 }
 
 

include/scsi/scsi_cmnd.h +0 -0

index a7be605..4e3cf43 100644

include/scsi/scsi_cmnd.h struct scatterlist; · line 19

Quoted 8 lines

include/scsi/scsi_cmnd.h +5 -0

@@ -12,6 +12,13 @@ struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
+struct scsi_data_buffer {
+	unsigned length;
+	int resid;
+	unsigned short sg_count;
+	unsigned short alloc_sg_count;

sg_count and alloc_sg_count are a bit lengthy?

My suggestion is using nr_* like the block layer (nr_sg and nr_alloc_sg ?).

Quoted 16 lines

include/scsi/scsi_cmnd.h +2 -2

+	struct scatterlist* sglist;
+};
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {

@@ -62,15 +69,10 @@ 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 */
 

include/scsi/scsi_cmnd.h struct scsi_cmnd { · line 78

Quoted 7 lines

include/scsi/scsi_cmnd.h +0 -3

 	/* These elements define the operation we ultimately want to perform */
-	unsigned short use_sg;	/* Number of pieces of scatter-gather */
-	unsigned short __use_sg;
-
 	unsigned underflow;	/* Return error if less than
 				   this amount is transferred */
 

Quoted 14 lines

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 453

Quoted 11 lines

drivers/scsi/sd.c +0 -3

index 18343a6..28cf6fe 100644
@@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} 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);
-		goto out;

This should go to the bidi patch?

Quoted 2 lines

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 452

Quoted 2 lines

drivers/scsi/sd.c +0 -0

 	}
 

This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer for that matter. It could be in it's own patch, but surly it will not go into the bidi patch. I will submit a new patch just for that code. Independent of these. (I was hoping to save the extra effort)

Quoted 12 lines · FUJITA Tomonori

Quoted 6 lines · On Tue, 06 Nov 2007 20:16:19 +0200 Boaz Harrosh <[email protected]> wrote:

- If we export scsi_init_io()/scsi_release_buffers() instead of scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is much more insulated from scsi_lib changes. As a bonus it will also gain bidi capability when it comes.

Signed-off-by Boaz Harrosh <[email protected]>

Looks good for me except for this:

Quoted 4 lines

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
ERROR: use tabs not spaces
#101: FILE: drivers/scsi/scsi_lib.c:741:
+ gfp_t gfp_mask)$

Come on Tomo, tabs should be used for nesting, not for decoration. This way no matter what's your tab expansion setup is the code will look correct and will make sense. The number of space

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block size so code needs to change once we move to scsi_data_buff implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
* Remove dead code (req_data_dir() != READ && != WRITE)

drivers/scsi/sr.c +10 -12

index 7702681..6d3bf41 100644
@@ -226,7 +226,7 @@ out:
 static int sr_done(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;

@@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;
 	}
 
 	{
-		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 != scsi_bufflen(SCpnt)) {
 			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, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 

Quoted 15 lines · On Thu, 08 Nov 2007 11:24:36 +0200 Boaz Harrosh <[email protected]> wrote:

Quoted 15 lines

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 453

Quoted 11 lines

drivers/scsi/sd.c +0 -3

index 18343a6..28cf6fe 100644
@@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} 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);
-		goto out;

This should go to the bidi patch?

Quoted 2 lines

drivers/scsi/sd.c +0 -0

 	}
 

Quoted 4 lines

This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer for that matter. It could be in it's own patch, but surly it will not go into the bidi patch. I will submit a new patch just for that code. Independent of these. (I was hoping to save the extra effort)

Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not hit that code. sd only accetps READ and WRITE requests. It prevents funcy requests like BIDI from accidentally comming.

James, Jens please note the question below
It is something that bothers me about sr.c

Quoted 3 lines · Boaz Harrosh

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

<snip>

Quoted 9 lines

- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block size so code needs to change once we move to scsi_data_buff implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
* Remove dead code (req_data_dir() != READ && != WRITE)

<snip>

Quoted 47 lines

drivers/scsi/sr.c +10 -12

index 7702681..6d3bf41 100644
@@ -226,7 +226,7 @@ out:
 static int sr_done(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;

@@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;
 	}
 
 	{
-		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 != scsi_bufflen(SCpnt)) {
 			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, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 396

Quoted 8 lines

drivers/scsi/sr.c +1 -1

@@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}

Here we check I/O is "large-block" aligned. Both start and size

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 399

Quoted 4 lines

drivers/scsi/sr.c +1 -1

-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 

Number of "large-blocks"

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 414

Quoted 10 lines

drivers/scsi/sr.c +1 -1

 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",

@@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 

Here is my problem:
In the case that the transfer is bigger than 0xffff * s_size (512/1024/2048) we modify ->request_bufflen. Now this has two bad effects, the way I understand it, please fix me in my misunderstanding.

1. Later in sr_done doing return good_bytes=cmd->request_bufflen will only complete the cut-out bytes. Meaning possible BIO leak, since the original request_bufflen was lost. (not all bytes are completed)

2. What mechanics will re-send, or even knows, that not the complete request was transfered? The way I understand it, a cmnd->resid must be set, in this case. Now the normal cmnd->resid is not enough because it will be written by drivers, sr needs to stash a resid somewhere and add it to cmnd->resid in sr_done(). But ...

Quoted 12 lines · On Thu, 08 Nov 2007 10:32:56 +0200 Benny Halevy <[email protected]> wrote:

Quoted 11 lines · FUJITA Tomonori

Quoted 6 lines · On Tue, 06 Nov 2007 20:16:19 +0200 Boaz Harrosh <[email protected]> wrote:

- If we export scsi_init_io()/scsi_release_buffers() instead of scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is much more insulated from scsi_lib changes. As a bonus it will also gain bidi capability when it comes.

Signed-off-by Boaz Harrosh <[email protected]>

Looks good for me except for this:

Quoted 7 lines

Quoted 4 lines

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
ERROR: use tabs not spaces
#101: FILE: drivers/scsi/scsi_lib.c:741:
+ gfp_t gfp_mask)$

Come on Tomo, tabs should be used for nesting, not for decoration. This way no matter what's your tab expansion setup is the code will look correct and will make sense. The number of space

I've never heard about that rule. I use tabs and minimum spaces for decoration.

But it's just about the style. The patch is fine by me if you like to use only spaces there.

Thanks,

Quoted 101 lines · Boaz Harrosh

James, Jens please note the question below
It is something that bothers me about sr.c

Quoted 3 lines · Boaz Harrosh

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

<snip>

Quoted 8 lines

- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block size so code needs to change once we move to scsi_data_buff implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
* Remove dead code (req_data_dir() != READ && != WRITE)

<snip>

Quoted 47 lines

drivers/scsi/sr.c +10 -12

index 7702681..6d3bf41 100644
@@ -226,7 +226,7 @@ out:
 static int sr_done(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;

@@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;
 	}
 
 	{
-		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 != scsi_bufflen(SCpnt)) {
 			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, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 396

Quoted 8 lines

drivers/scsi/sr.c +1 -1

@@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}

Here we check I/O is "large-block" aligned. Both start and size

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 399

Quoted 4 lines

drivers/scsi/sr.c +1 -1

-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 

Number of "large-blocks"

drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 414

Quoted 10 lines

drivers/scsi/sr.c +1 -1

 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",

@@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 

Here is my problem:
In the case that the transfer is bigger than 0xffff * s_size (512/1024/2048) we modify ->request_bufflen. Now this has two bad effects, the way I understand it, please fix me in my misunderstanding.

1. Later in sr_done doing return good_bytes=cmd->request_bufflen will only complete the cut-out bytes. Meaning possible BIO leak, since the original request_bufflen was lost. (not all bytes are completed)

Quoted 6 lines

2. What mechanics will re-send, or even knows, that not the complete request was transfered? The way I understand it, a cmnd->resid must be set, in this case. Now the normal cmnd->resid is not enough because it will be written by drivers, sr needs to stash a resid somewhere and add it to cmnd->resid in sr_done(). But ...

It's not lost, the request will be requeued in scsi_end_request(). The original state is in the request structure, and end_that_request_chunk() will return not-done when you complete this first part.

Quoted 26 lines · FUJITA Tomonori

Quoted 12 lines · On Thu, 08 Nov 2007 10:32:56 +0200 Benny Halevy <[email protected]> wrote:

Quoted 11 lines · FUJITA Tomonori

Quoted 6 lines · On Tue, 06 Nov 2007 20:16:19 +0200 Boaz Harrosh <[email protected]> wrote:

- If we export scsi_init_io()/scsi_release_buffers() instead of scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is much more insulated from scsi_lib changes. As a bonus it will also gain bidi capability when it comes.

Signed-off-by Boaz Harrosh <[email protected]>

Looks good for me except for this:

Quoted 7 lines

Quoted 4 lines

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
ERROR: use tabs not spaces
#101: FILE: drivers/scsi/scsi_lib.c:741:
+ gfp_t gfp_mask)$

Come on Tomo, tabs should be used for nesting, not for decoration. This way no matter what's your tab expansion setup is the code will look correct and will make sense. The number of space

I've never heard about that rule. I use tabs and minimum spaces for decoration.

Quoted 4 lines

But it's just about the style. The patch is fine by me if you like to use only spaces there.

Thanks,

Thanks Tomo, I'm resending with the way you like it. You are the maintainer and you should be comfortable with the code.

I do need your Signed-off-by on this. Since you are the maintainer. Do you want that we push this through James, or through your tree?

[1]
I propose a small change to scsi_tgt_lib.c that will make tgt completely neutral to the scsi_data_buffer patch. And will make it all that more ready for bidi, too. TOMO is this OK?

(Can you do without the GFP_KERNEL allocation flag? It could make the code a bit more simple)

[2]
scsi_data_buffer patch.
As requested by TOMO the all patch is squashed into one, 600 and some lines. TOMO if it makes you happy, OK, here it is.

There is one hunk from libsrp.c that I really hate. and from what I understand of the code, only the "request_bufflen =" is really used, All users of "request_buffer = addr" pass NULL, as far as I can see. If you would like to make me happy, and it is at all possible, please clean it.

[3]
Once scsi_data_buffer is in, then scsi bidi support can be applied. (Block bidi was already merged 3 kernels ago). It makes no changes to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib. So I don't see any reason to wait. please all review.

If ACKed by all parties and applied for inclusion for 2.6.25, then they will have a long time to sit in MM and collect compilation breakage reports from ARCHs we never compiled.

I wish these make everybody happy this time

Boaz Harrosh

At the block level bidi request uses req->next_rq pointer for a second bidi_read request.
At Scsi-midlayer a second scsi_data_buffer structure is used for the bidi_read part. This bidi scsi_data_buffer is put on request->next_rq->special. Struct scsi_cmnd is not changed.

- Define scsi_bidi_cmnd() to return true if it is a bidi request and a second sgtable was allocated.

- Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer from this command This API is to isolate users from the mechanics of bidi.

- Define scsi_end_bidi_request() to do what scsi_end_request() does but for a bidi request. This is necessary because bidi commands are a bit tricky here. (See comments in body)

/*
+ * Bidi commands Must be complete as a whole, both sides at once.
+ * If part of the bytes were written and lld returned
+ * scsi_in()->resid and/or scsi_out()->resid this information will be left
+ * in req->data_len and req->next_rq->data_len. The upper-layer driver can
+ * decide what to do with this information.
+ */

drivers/scsi/scsi_lib.c +11 -0

+void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+{
+	struct request *req = cmd->request;
+
+	end_that_request_chunk(req, 1, req->data_len);
+	req->data_len = scsi_out(cmd)->resid;
+
+	end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
+	req->next_rq->data_len = scsi_in(cmd)->resid;
+
+	scsi_release_buffers(cmd);

Quoted 30 lines

I'm trying to refine the request completion interface between device drivers and the block-layer.
(http://marc.info/?l=linux-kernel&m=118953696504819&w=2)

For that work, I wish to keep the space between end_that_request_chunk() and end_that_request_last() simpler as much as possible so that they can be converted to the interface easily.
So I'd like to ask you whether 3 reorderings below cause problem for you.
1. reorder end_that_request_chunk() calls for req and req->next_rq
2. set data_len before end_that_request_chunk() calls
3. call scsi_release_buffer() after scsi_finalize_request()

void scsi_end_bidi_request(struct scsi_cmnd *cmd)

{
	struct request *req = cmd->request;
	unsigned int dlen = req->data_len;
	unsigned int next_dlen = req->next_rq->data_len;

	req->data_len = scsi_out(cmd)->resid;
	req->next_rq->data_len = scsi_in(cmd)->resid;

	end_that_request_chunk(req->next_rq, 1, next_dlen);
	end_that_request_chunk(req, 1, dlen);

	scsi_finalize_request(cmd, 1);
	scsi_release_buffers(cmd);
}

Quoted 1 line

If they are no problem, I could easily convert it to the new interface.

1. If req->data_len is not affected by end_that_request_chunk() than sure.
2. Please note that "end_that_request_chunk(req->next_rq, 1, next_dlen);" *must* not be paired with a call to end_that_request_last(). It is not a "Queued" request and will break the system. This is the "bidi trick". So end_that_request_chunk() must be exported still.
3. scsi_release_buffers(cmd) must stay where it is, and certainly before scsi_finalize_request() which does a put_command.
4. If you are already touching block-layer. I would be happy if you add a blk_end_request_bidi(struct request *, int write_resid, int read_resid, error ...) ;
That will release both request and request->next_rq, completely and set resid for ULDs. Then scsi_end_bidi_request() can be removed.

I would prefer to leave the code as it is for now and change it to what ever it needs to be in your patchset.
But if you insist What I would suggest is below: (Minus the comments)

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.

Quoted 5 lines · On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <[email protected]> wrote:

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.

drivers/scsi/qla1280.c: In function 'qla1280_done':
drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c: In function 'qla1280_return_status':
drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid' drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen' make[2]: *** [drivers/scsi/qla1280.o] Error 1

It mystfies me how a patch like this can have been floating about in N submissions across M months and nobody has done an allmodconfig build or even a grep to find out what broke.

ho hum. I shall mark qla1280 BROKEN and shall plod onwards.

Quoted 28 lines · On Mon, 12 Nov 2007 22:06:52 -0800 Andrew Morton <[email protected]>…

Quoted 5 lines · On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <[email protected]> wrote:

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.

drivers/scsi/qla1280.c: In function 'qla1280_done':
drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c: In function 'qla1280_return_status':
drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid' drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen' make[2]: *** [drivers/scsi/qla1280.o] Error 1

Quoted 5 lines

It mystfies me how a patch like this can have been floating about in N submissions across M months and nobody has done an allmodconfig build or even a grep to find out what broke.

ho hum. I shall mark qla1280 BROKEN and shall plod onwards.

A patch to fix this is in James' scsi-pending tree. Jes tested and fixed it (thanks !) so it will go to -mm via scsi-misc soon.

Boaz, it's better to send major scsi patches to -mm via scsi-misc to avoid problems like this.

By the way, Andrew, can you add the following patchset to -mm?

http://lkml.org/lkml/2007/10/24/138

It fixes the IOMMUs' problem to merge scatter/gather segments without considering LLDs' restrictions.

Quoted 35 lines · On Tue, 13 Nov 2007 15:40:42 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 32 lines · On Mon, 12 Nov 2007 22:06:52 -0800 Andrew Morton <[email protected]>…

Quoted 5 lines · On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <[email protected]> wrote:

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.

drivers/scsi/qla1280.c: In function 'qla1280_done':
drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c: In function 'qla1280_return_status':
drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid' drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen' make[2]: *** [drivers/scsi/qla1280.o] Error 1

It mystfies me how a patch like this can have been floating about in N submissions across M months and nobody has done an allmodconfig build or even a grep to find out what broke.

Quoted 4 lines

Quoted 1 line

ho hum. I shall mark qla1280 BROKEN and shall plod onwards.

A patch to fix this is in James' scsi-pending tree. Jes tested and fixed it (thanks !) so it will go to -mm via scsi-misc soon.

oh gawd. So we have git-scsi-misc, git-scsi-rc-fixes and now git-scsi-pending?

I hope you fixed imm, ppa and any other broken drivers?

Quoted 4 lines

Boaz, it's better to send major scsi patches to -mm via scsi-misc to avoid problems like this.

Quoted 6 lines

By the way, Andrew, can you add the following patchset to -mm?

http://lkml.org/lkml/2007/10/24/138

It fixes the IOMMUs' problem to merge scatter/gather segments without considering LLDs' restrictions.

hmm, OK, I saved them away to look at after next -mm.

Quoted 27 lines · On Mon, 12 Nov 2007 22:06:52 -0800 Andrew Morton <[email protected]>…

Quoted 5 lines · On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <[email protected]> wrote:

In preparation for bidi we abstract all IO members of scsi_cmnd, that will need to duplicate, into a substructure.

- Group all IO members of scsi_cmnd into a scsi_data_buffer structure.

drivers/scsi/qla1280.c: In function 'qla1280_done':
drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c: In function 'qla1280_return_status':
drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid' drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg' drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer' drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen' drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen' make[2]: *** [drivers/scsi/qla1280.o] Error 1

Quoted 5 lines

It mystfies me how a patch like this can have been floating about in N submissions across M months and nobody has done an allmodconfig build or even a grep to find out what broke.

ho hum. I shall mark qla1280 BROKEN and shall plod onwards.

A patch to fix this is in James' scsi-pending tree. Jes tested and fixed it (thanks !) so it will go to -mm via scsi-misc soon.

Boaz, it's better to send major scsi patches to -mm via scsi-misc to avoid problems like this.

By the way, Andrew, can you add the following patchset to -mm?

http://lkml.org/lkml/2007/10/24/138

It fixes the IOMMUs' problem to merge scatter/gather segments without considering LLDs' restrictions.

Thanks,

From what Christoph said the only way we can get their comments, if any, is by