use the cmd_type of a leading request for scsi_init_sgtable

PATCH In review FUJITA Tomonori 3 participants 5 messages Jan 26, 2008

Subsystems (3)

SCSI Block layer BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

FUJITA Tomonori

author

2008-01-26 00:58 UTC #0 lore raw

PATCH 1/1 use the cmd_type of a leading request for scsi_init_sgtable

This is against the scsi-bidi tree.

We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of a bidi request properly.

An alternative approach is setting the cmd_type of a leading request and its bidi request (*1). But the block layer and scsi-ml don't expect that the leading request and its sub-requests have the different command types.

Note that scsi_debug's XDWRITEREAD_10 support is fine without this patch since req->nr_sectors works for it but req->nr_sectors doesn't work for everyone.

(*1)

http://www.mail-archive.com/[email protected]/msg12669.html

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable

We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of its bidi request properly.

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

---
 drivers/scsi/scsi_lib.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

drivers/scsi/scsi_lib.c +14 -9

index e1c7eeb..61093ea 100644
@@ -1001,8 +1001,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	scsi_end_request(cmd, -EIO, this_count, !result);
 }
 
-static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
-			     gfp_t gfp_mask)
+static int scsi_init_sgtable(struct request *req,
+			     enum rq_cmd_type_bits cmd_type,
+			     struct scsi_data_buffer *sdb, gfp_t gfp_mask)
 {
 	int count;

@@ -1015,12 +1016,12 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
 	}
 
 	req->buffer = NULL;
-	if (blk_pc_request(req))
+	if (cmd_type == REQ_TYPE_BLOCK_PC)
 		sdb->length = req->data_len;
 	else
 		sdb->length = req->nr_sectors << 9;
 
-	/* 
+	/*
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */

@@ -1043,21 +1044,25 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
  */
 int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
-	int error = scsi_init_sgtable(cmd->request, &cmd->sdb, gfp_mask);
+	int error;
+	enum rq_cmd_type_bits cmd_type = cmd->request->cmd_type;
+
+	error = scsi_init_sgtable(cmd->request, cmd_type, &cmd->sdb, gfp_mask);
 	if (error)
 		goto err_exit;
 
 	if (blk_bidi_rq(cmd->request)) {
-		struct scsi_data_buffer *bidi_sdb = kmem_cache_zalloc(
-			scsi_bidi_sdb_cache, GFP_ATOMIC);
+		struct scsi_data_buffer *bidi_sdb;
+
+		bidi_sdb = kmem_cache_zalloc(scsi_bidi_sdb_cache, GFP_ATOMIC);
 		if (!bidi_sdb) {
 			error = BLKPREP_DEFER;
 			goto err_exit;
 		}
 
 		cmd->request->next_rq->special = bidi_sdb;
-		error = scsi_init_sgtable(cmd->request->next_rq, bidi_sdb,
-								    GFP_ATOMIC);
+		error = scsi_init_sgtable(cmd->request->next_rq, cmd_type,
+					  bidi_sdb, GFP_ATOMIC);
 		if (error)
 			goto err_exit;
 	}

Signature · 1 line

1.5.3.4

James Bottomley

maintainer

2008-01-26 02:06 UTC #1 lore raw

Quoted 16 lines · FUJITA Tomonori

This is against the scsi-bidi tree.
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of a bidi request properly.
An alternative approach is setting the cmd_type of a leading request and its bidi request (*1). But the block layer and scsi-ml don't expect that the leading request and its sub-requests have the different command types.
Note that scsi_debug's XDWRITEREAD_10 support is fine without this patch since req->nr_sectors works for it but req->nr_sectors doesn't work for everyone.
(*1)

Quoted 8 lines

http://www.mail-archive.com/[email protected]/msg12669.html
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of its bidi request properly.

This seems to be a very convoluted work around for the fact that we forgot to set the cmd_type on the subordinate request.

Wouldn't this be a better fix?

James

---

block/bsg.c +1 -0

index 69b0a9d..8917c51 100644
@@ -279,6 +279,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 			goto out;
 		}
 		rq->next_rq = next_rq;
+		next_rq->cmd_type = rq->cmd_type;
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);

FUJITA Tomonori

author Acked-by

2008-01-26 02:23 UTC #2 lore raw

Quoted 23 lines · On Fri, 25 Jan 2008 20:05:55 -0600 James Bottomley…

Quoted 22 lines · FUJITA Tomonori
This is against the scsi-bidi tree.
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of a bidi request properly.
An alternative approach is setting the cmd_type of a leading request and its bidi request (*1). But the block layer and scsi-ml don't expect that the leading request and its sub-requests have the different command types.
Note that scsi_debug's XDWRITEREAD_10 support is fine without this patch since req->nr_sectors works for it but req->nr_sectors doesn't work for everyone.
(*1)
http://www.mail-archive.com/[email protected]/msg12669.html
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable

Quoted 7 lines

Quoted 2 lines
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of its bidi request properly.
This seems to be a very convoluted work around for the fact that we forgot to set the cmd_type on the subordinate request.
Wouldn't this be a better fix?

I'm fine with this. I have no big preference in this issue.

Acked-by FUJITA Tomonori <[email protected]>

I just thought that the approach (the block layer and scsi-ml look at only the type of a leading request) show better how the block layer and scsi-ml see a leading request and its subordinate requests (all the requests need to have the same command type).

Quoted 22 lines

James
---
block/bsg.c +1 -1
index 69b0a9d..8917c51 100644
@@ -279,6 +279,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 			goto out;
 		}
 		rq->next_rq = next_rq;
+		next_rq->cmd_type = rq->cmd_type;
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
 
 
-
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

FUJITA Tomonori

author

2008-01-26 02:28 UTC #3 lore raw

Quoted 36 lines · On Sat, 26 Jan 2008 11:22:47 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 23 lines · On Fri, 25 Jan 2008 20:05:55 -0600 James Bottomley…
Quoted 22 lines · FUJITA Tomonori
This is against the scsi-bidi tree.
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of a bidi request properly.
An alternative approach is setting the cmd_type of a leading request and its bidi request (*1). But the block layer and scsi-ml don't expect that the leading request and its sub-requests have the different command types.
Note that scsi_debug's XDWRITEREAD_10 support is fine without this patch since req->nr_sectors works for it but req->nr_sectors doesn't work for everyone.
(*1)
http://www.mail-archive.com/[email protected]/msg12669.html
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable
Quoted 7 lines
Quoted 2 lines
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of its bidi request properly.
This seems to be a very convoluted work around for the fact that we forgot to set the cmd_type on the subordinate request.
Wouldn't this be a better fix?
I'm fine with this. I have no big preference in this issue.

Quoted 7 lines

Acked-by FUJITA Tomonori <[email protected]>
I just thought that the approach (the block layer and scsi-ml look at only the type of a leading request) show better how the block layer and scsi-ml see a leading request and its subordinate requests (all the requests need to have the same command type).

I meant that the block layer and scsi-ml use its subordinate requests just to hook data buffer.

Anyway, I'm fine with your patch.

Pete Wyckoff

2008-01-26 02:34 UTC #4 lore raw

In reply to James Bottomley

[email protected] wrote on Fri, 25 Jan 2008 20:05 -0600:

Quoted 42 lines · FUJITA Tomonori

Quoted 16 lines
This is against the scsi-bidi tree.
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of a bidi request properly.
An alternative approach is setting the cmd_type of a leading request and its bidi request (*1). But the block layer and scsi-ml don't expect that the leading request and its sub-requests have the different command types.
Note that scsi_debug's XDWRITEREAD_10 support is fine without this patch since req->nr_sectors works for it but req->nr_sectors doesn't work for everyone.
(*1)
Quoted 8 lines
http://www.mail-archive.com/[email protected]/msg12669.html
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable
We need to use the cmd_type of a leading request for scsi_init_sgtable to set up scsi_data_buffer:length of its bidi request properly.
This seems to be a very convoluted work around for the fact that we forgot to set the cmd_type on the subordinate request.
Wouldn't this be a better fix?
James
---
block/bsg.c +0 -0
index 69b0a9d..8917c51 100644

block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 285

Quoted 7 lines

block/bsg.c +1 -0

@@ -279,6 +279,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 			goto out;
 		}
 		rq->next_rq = next_rq;
+		next_rq->cmd_type = rq->cmd_type;
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);

Agree. I did essentially the same thing in:

http://marc.info/?l=linux-scsi&m=119835555623270

Tomo, you may want to have a look at all the bsg patches I sent back in Dec 2007. Boaz has the minimum required for bidi in his tree. There are a few more here too, if you want to see what we need for OSD:

http://git.osc.edu/?p=linux.git;a=summary

		-- Pete