[PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[FUJITA Tomonori]

This patchset removes the majority of scsi_execute_async in st
driver. IOW, this converts st driver to use the block layer functions.

We are in the process of removing scsi_execute_async and
scsi_req_map_sg. scsi_execute_async in sg driver were removed in
2.6.27. Now only st and osst drivers use scsi_execute_async().

st driver uses scsi_execute_async for two purposes, performing sort
management SCSI commands internally and data transfer between user and
kernel space (st_read and st_write). This patchset converts the
former.

The former performs SCSI commands synchronously. It uses a liner
in-kernel buffer (not scatter gather) for data transfer. To perform
such commands, other upper layer drivers (e.g. sd) use
scsi_execute_req (internally uses blk_rq_map_kern and and
blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
helper function similar to scsi_execute_req and replace
scsi_execute_async in st driver (I might modify scsi_execute_req for
st and remove the helper function later).

I'm still working on converting scsi_execute_async for data transfer
between user and kernel space but I want to get this first half be
merged.

Thanks,

[PATCH 01/11] st: move st_request initialization to st_allocate_request form st_do_scsi

[FUJITA Tomonori]

This moves st_request initialization code to st_allocate_request()
form st_do_scsi(). This is a preparation for making
st_allocate_request() usable for everyone, not only st_do_scsi().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index c959bdc..878493d 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -451,9 +451,23 @@ static void st_sleep_done(void *data, char *sense, int result, int resid)
 		complete(SRpnt->waiting);
 }
 
-static struct st_request *st_allocate_request(void)
+static struct st_request *st_allocate_request(struct scsi_tape *stp)
 {
-	return kzalloc(sizeof(struct st_request), GFP_KERNEL);
+	struct st_request *streq;
+
+	streq = kzalloc(sizeof(*streq), GFP_KERNEL);
+	if (streq)
+		streq->stp = stp;
+	else {
+		DEBC(printk(KERN_ERR "%s: Can't get SCSI request.\n",
+			    tape_name(stp)););
+		if (signal_pending(current))
+			stp->buffer->syscall_result = -EINTR;
+		else
+			stp->buffer->syscall_result = -EBUSY;
+	}
+
+	return streq;
 }
 
 static void st_release_request(struct st_request *streq)
@@ -481,18 +495,10 @@ st_do_scsi(struct st_request * SRpnt, struct scsi_tape * STp, unsigned char *cmd
 		return NULL;
 	}
 
-	if (SRpnt == NULL) {
-		SRpnt = st_allocate_request();
-		if (SRpnt == NULL) {
-			DEBC( printk(KERN_ERR "%s: Can't get SCSI request.\n",
-				     tape_name(STp)); );
-			if (signal_pending(current))
-				(STp->buffer)->syscall_result = (-EINTR);
-			else
-				(STp->buffer)->syscall_result = (-EBUSY);
+	if (!SRpnt) {
+		SRpnt = st_allocate_request(STp);
+		if (!SRpnt)
 			return NULL;
-		}
-		SRpnt->stp = STp;
 	}
 
 	/* If async IO, set last_SRpnt. This ptr tells write_behind_check
-- 
1.5.6.5

[PATCH 02/11] st: add st_scsi_kern_execute helper function

[FUJITA Tomonori]

st_scsi_kern_execute is a helper function to perform SCSI commands
synchronously. It supports data transfer with a liner in-kernel buffer
(not scatter gather). st_scsi_kern_execute internally uses
blk_get_request, blk_rq_map_kern, and blk_execute_rq.

The majority of st_do_scsi can be replaced with
st_scsi_kern_execute. This is a preparation for rewriting st_do_scsi
to remove obsolete scsi_execute_async().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 878493d..6179940 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -533,6 +533,55 @@ st_do_scsi(struct st_request * SRpnt, struct scsi_tape * STp, unsigned char *cmd
 	return SRpnt;
 }
 
+static int st_scsi_kern_execute(struct st_request *streq,
+				const unsigned char *cmd, int data_direction,
+				void *buffer, unsigned bufflen, int timeout,
+				int retries)
+{
+	struct scsi_tape *stp = streq->stp;
+	struct request *req;
+	int write = (data_direction == DMA_TO_DEVICE);
+	int ret = 0;
+
+	/* st_do_scsi returns -EBUSY in case of OOM */
+	req = blk_get_request(stp->device->request_queue, write, GFP_KERNEL);
+	if (!req)
+		return -EBUSY;
+
+	if (bufflen) {
+		ret = blk_rq_map_kern(stp->device->request_queue, req,
+				      buffer, bufflen, GFP_KERNEL);
+		if (ret)
+			goto out;
+	}
+
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_QUIET;
+
+	req->cmd_len = COMMAND_SIZE(cmd[0]);
+	memcpy(req->cmd, cmd, req->cmd_len);
+
+	req->sense = streq->sense;
+	req->sense_len = 0;
+
+	req->timeout = timeout;
+	req->retries = retries;
+
+	stp->buffer->cmdstat.have_sense = 0;
+	memcpy(streq->cmd, cmd, sizeof(streq->cmd));
+
+	blk_execute_rq(req->q, NULL, req, 1);
+
+	stp->buffer->cmdstat.midlevel_result = streq->result = req->errors;
+	stp->buffer->cmdstat.residual = req->data_len;
+
+	stp->buffer->syscall_result = st_chk_result(stp, streq);
+
+out:
+	blk_put_request(req);
+
+	return ret;
+}
 
 /* Handle the write-behind checking (waits for completion). Returns -ENOSPC if
    write has been correct but EOM early warning reached, -EIO if write ended in
-- 
1.5.6.5

[PATCH 03/11] st: convert test_ready to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in test_ready (TEST_UNIT_READY) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 6179940..72b4b44 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -899,21 +899,24 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
 	int attentions, waits, max_wait, scode;
 	int retval = CHKRES_READY, new_session = 0;
 	unsigned char cmd[MAX_COMMAND_SIZE];
-	struct st_request *SRpnt = NULL;
+	struct st_request *SRpnt;
 	struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
 
+	SRpnt = st_allocate_request(STp);
+	if (!SRpnt)
+		return STp->buffer->syscall_result;
+
 	max_wait = do_wait ? ST_BLOCK_SECONDS : 0;
 
 	for (attentions=waits=0; ; ) {
 		memset((void *) &cmd[0], 0, MAX_COMMAND_SIZE);
 		cmd[0] = TEST_UNIT_READY;
-		SRpnt = st_do_scsi(SRpnt, STp, cmd, 0, DMA_NONE,
-				   STp->long_timeout, MAX_READY_RETRIES, 1);
 
-		if (!SRpnt) {
-			retval = (STp->buffer)->syscall_result;
+		retval = st_scsi_kern_execute(SRpnt, cmd, DMA_NONE, NULL, 0,
+					      STp->long_timeout,
+					      MAX_READY_RETRIES);
+		if (retval)
 			break;
-		}
 
 		if (cmdstatp->have_sense) {
 
@@ -957,8 +960,8 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
 		break;
 	}
 
-	if (SRpnt != NULL)
-		st_release_request(SRpnt);
+	st_release_request(SRpnt);
+
 	return retval;
 }
 
-- 
1.5.6.5

[PATCH 04/11] st: convert set_location to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in set_location (LOCATE 10) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 72b4b44..cce4358 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3106,10 +3106,14 @@ static int set_location(struct scsi_tape *STp, unsigned int block, int partition
 		timeout = STp->device->timeout;
 	}
 
-	SRpnt = st_do_scsi(NULL, STp, scmd, 0, DMA_NONE,
-			   timeout, MAX_READY_RETRIES, 1);
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
-		return (STp->buffer)->syscall_result;
+		return STp->buffer->syscall_result;
+
+	result = st_scsi_kern_execute(SRpnt, scmd, DMA_NONE, NULL, 0,
+				      timeout, MAX_READY_RETRIES);
+	if (result)
+		goto out;
 
 	STps->drv_block = STps->drv_file = (-1);
 	STps->eof = ST_NOEOF;
@@ -3134,7 +3138,7 @@ static int set_location(struct scsi_tape *STp, unsigned int block, int partition
 			STps->drv_block = STps->drv_file = 0;
 		result = 0;
 	}
-
+out:
 	st_release_request(SRpnt);
 	SRpnt = NULL;
 
-- 
1.5.6.5

[PATCH 05/11] st: convert do_load_unload to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in do_load_unload (START STOP) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index cce4358..61ed6c1 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2534,13 +2534,16 @@ static int do_load_unload(struct scsi_tape *STp, struct file *filp, int load_cod
 		printk(ST_DEB_MSG "%s: Loading tape.\n", name);
 		);
 
-	SRpnt = st_do_scsi(NULL, STp, cmd, 0, DMA_NONE,
-			   timeout, MAX_RETRIES, 1);
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
-		return (STp->buffer)->syscall_result;
+		return STp->buffer->syscall_result;
+
+	retval = st_scsi_kern_execute(SRpnt, cmd, DMA_NONE, NULL, 0, timeout,
+				      MAX_RETRIES);
+	if (retval)
+		goto out;
 
 	retval = (STp->buffer)->syscall_result;
-	st_release_request(SRpnt);
 
 	if (!retval) {	/* SCSI command successful */
 
@@ -2559,6 +2562,8 @@ static int do_load_unload(struct scsi_tape *STp, struct file *filp, int load_cod
 		STps = &(STp->ps[STp->partition]);
 		STps->drv_file = STps->drv_block = (-1);
 	}
+out:
+	st_release_request(SRpnt);
 
 	return retval;
 }
-- 
1.5.6.5

[PATCH 06/11] st: convert cross_eof to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in cross_eof (SPACE) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 61ed6c1..4bfdc96 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -654,6 +654,7 @@ static int cross_eof(struct scsi_tape * STp, int forward)
 {
 	struct st_request *SRpnt;
 	unsigned char cmd[MAX_COMMAND_SIZE];
+	int ret;
 
 	cmd[0] = SPACE;
 	cmd[1] = 0x01;		/* Space FileMarks */
@@ -667,19 +668,25 @@ static int cross_eof(struct scsi_tape * STp, int forward)
         DEBC(printk(ST_DEB_MSG "%s: Stepping over filemark %s.\n",
 		   tape_name(STp), forward ? "forward" : "backward"));
 
-	SRpnt = st_do_scsi(NULL, STp, cmd, 0, DMA_NONE,
-			   STp->device->timeout, MAX_RETRIES, 1);
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
-		return (STp->buffer)->syscall_result;
+		return STp->buffer->syscall_result;
 
-	st_release_request(SRpnt);
-	SRpnt = NULL;
+	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_NONE, NULL, 0,
+				   STp->device->timeout, MAX_RETRIES);
+	if (ret)
+		goto out;
+
+	ret = STp->buffer->syscall_result;
 
 	if ((STp->buffer)->cmdstat.midlevel_result != 0)
 		printk(KERN_ERR "%s: Stepping over filemark %s failed.\n",
 		   tape_name(STp), forward ? "forward" : "backward");
 
-	return (STp->buffer)->syscall_result;
+out:
+	st_release_request(SRpnt);
+
+	return ret;
 }
 
 
-- 
1.5.6.5

[PATCH 07/11] st: convert st_flush to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in st_flush (WRITE FILEMARKS) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 4bfdc96..43651cd 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -1311,10 +1311,17 @@ static int st_flush(struct file *filp, fl_owner_t id)
 		cmd[0] = WRITE_FILEMARKS;
 		cmd[4] = 1 + STp->two_fm;
 
-		SRpnt = st_do_scsi(NULL, STp, cmd, 0, DMA_NONE,
-				   STp->device->timeout, MAX_WRITE_RETRIES, 1);
+		SRpnt = st_allocate_request(STp);
 		if (!SRpnt) {
-			result = (STp->buffer)->syscall_result;
+			result = STp->buffer->syscall_result;
+			goto out;
+		}
+
+		result = st_scsi_kern_execute(SRpnt, cmd, DMA_NONE, NULL, 0,
+					      STp->device->timeout,
+					      MAX_WRITE_RETRIES);
+		if (result) {
+			st_release_request(SRpnt);
 			goto out;
 		}
 
-- 
1.5.6.5

[PATCH 08/11] st: convert check_tape to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in check_tape (READ_BLOCK_LIMITS and
MODE_SENSE) with st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 43651cd..cc085bc 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -1045,16 +1045,24 @@ static int check_tape(struct scsi_tape *STp, struct file *filp)
 		}
 	}
 
+	SRpnt = st_allocate_request(STp);
+	if (!SRpnt) {
+		retval = STp->buffer->syscall_result;
+		goto err_out;
+	}
+
 	if (STp->omit_blklims)
 		STp->min_block = STp->max_block = (-1);
 	else {
 		memset((void *) &cmd[0], 0, MAX_COMMAND_SIZE);
 		cmd[0] = READ_BLOCK_LIMITS;
 
-		SRpnt = st_do_scsi(SRpnt, STp, cmd, 6, DMA_FROM_DEVICE,
-				   STp->device->timeout, MAX_READY_RETRIES, 1);
-		if (!SRpnt) {
-			retval = (STp->buffer)->syscall_result;
+		retval = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
+					      STp->buffer->b_data, 6,
+					      STp->device->timeout,
+					      MAX_READY_RETRIES);
+		if (retval) {
+			st_release_request(SRpnt);
 			goto err_out;
 		}
 
@@ -1078,10 +1086,12 @@ static int check_tape(struct scsi_tape *STp, struct file *filp)
 	cmd[0] = MODE_SENSE;
 	cmd[4] = 12;
 
-	SRpnt = st_do_scsi(SRpnt, STp, cmd, 12, DMA_FROM_DEVICE,
-			STp->device->timeout, MAX_READY_RETRIES, 1);
-	if (!SRpnt) {
-		retval = (STp->buffer)->syscall_result;
+	retval = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
+				      STp->buffer->b_data, 12,
+				      STp->device->timeout,
+				      MAX_READY_RETRIES);
+	if (retval) {
+		st_release_request(SRpnt);
 		goto err_out;
 	}
 
-- 
1.5.6.5

[PATCH 09/11] st: convert read_mode_page to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in read_mode_page (MODE_SENSE) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index cc085bc..91d1249 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2393,7 +2393,8 @@ static int st_set_options(struct scsi_tape *STp, long options)
 static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
 {
 	unsigned char cmd[MAX_COMMAND_SIZE];
-	struct st_request *SRpnt = NULL;
+	struct st_request *SRpnt;
+	int ret;
 
 	memset(cmd, 0, MAX_COMMAND_SIZE);
 	cmd[0] = MODE_SENSE;
@@ -2402,10 +2403,15 @@ static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
 	cmd[2] = page;
 	cmd[4] = 255;
 
-	SRpnt = st_do_scsi(SRpnt, STp, cmd, cmd[4], DMA_FROM_DEVICE,
-			   STp->device->timeout, 0, 1);
-	if (SRpnt == NULL)
-		return (STp->buffer)->syscall_result;
+	SRpnt = st_allocate_request(STp);
+	if (!SRpnt)
+		return STp->buffer->syscall_result;
+
+	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
+				   STp->buffer->b_data, cmd[4],
+				   STp->device->timeout, MAX_RETRIES);
+	if (ret)
+		return ret;
 
 	st_release_request(SRpnt);
 
-- 
1.5.6.5

[PATCH 10/11] st: convert write_mode_page to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in write_mode_page (MODE_SELECT) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 91d1249..6fbfe33 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2423,9 +2423,9 @@ static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
    in the buffer is correctly formatted. The long timeout is used if slow is non-zero. */
 static int write_mode_page(struct scsi_tape *STp, int page, int slow)
 {
-	int pgo;
+	int pgo, timeout, ret = 0;
 	unsigned char cmd[MAX_COMMAND_SIZE];
-	struct st_request *SRpnt = NULL;
+	struct st_request *SRpnt;
 
 	memset(cmd, 0, MAX_COMMAND_SIZE);
 	cmd[0] = MODE_SELECT;
@@ -2439,14 +2439,20 @@ static int write_mode_page(struct scsi_tape *STp, int page, int slow)
 	(STp->buffer)->b_data[MH_OFF_DEV_SPECIFIC] &= ~MH_BIT_WP;
 	(STp->buffer)->b_data[pgo + MP_OFF_PAGE_NBR] &= MP_MSK_PAGE_NBR;
 
-	SRpnt = st_do_scsi(SRpnt, STp, cmd, cmd[4], DMA_TO_DEVICE,
-			   (slow ? STp->long_timeout : STp->device->timeout), 0, 1);
-	if (SRpnt == NULL)
-		return (STp->buffer)->syscall_result;
+	SRpnt = st_allocate_request(STp);
+	if (!SRpnt)
+		return ret;
+
+	timeout = slow ? STp->long_timeout : STp->device->timeout;
+
+	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_TO_DEVICE,
+				   STp->buffer->b_data, cmd[4], timeout, 0);
+	if (!ret)
+		ret = STp->buffer->syscall_result;
 
 	st_release_request(SRpnt);
 
-	return (STp->buffer)->syscall_result;
+	return ret;
 }
 
 
-- 
1.5.6.5

[PATCH 11/11] st: convert get_location to use st_scsi_kern_execute

[FUJITA Tomonori]

This replaces st_do_scsi in get_location (READ_POSITION) with
st_scsi_kern_execute.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/st.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 6fbfe33..f026398 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3042,10 +3042,16 @@ static int get_location(struct scsi_tape *STp, unsigned int *block, int *partiti
 		if (!logical && !STp->scsi2_logical)
 			scmd[1] = 1;
 	}
-	SRpnt = st_do_scsi(NULL, STp, scmd, 20, DMA_FROM_DEVICE,
-			STp->device->timeout, MAX_READY_RETRIES, 1);
+
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
-		return (STp->buffer)->syscall_result;
+		return STp->buffer->syscall_result;
+
+	result = st_scsi_kern_execute(SRpnt, scmd, DMA_FROM_DEVICE,
+				      STp->buffer->b_data, 20,
+				      STp->device->timeout, MAX_READY_RETRIES);
+	if (result)
+		goto out;
 
 	if ((STp->buffer)->syscall_result != 0 ||
 	    (STp->device->scsi_level >= SCSI_2 &&
@@ -3073,6 +3079,7 @@ static int get_location(struct scsi_tape *STp, unsigned int *block, int *partiti
                 DEBC(printk(ST_DEB_MSG "%s: Got tape pos. blk %d part %d.\n", name,
                             *block, *partition));
 	}
+out:
 	st_release_request(SRpnt);
 	SRpnt = NULL;
 
-- 
1.5.6.5

Re: [PATCH 02/11] st: add st_scsi_kern_execute helper function

[Boaz Harrosh]

FUJITA Tomonori wrote:
> st_scsi_kern_execute is a helper function to perform SCSI commands
> synchronously. It supports data transfer with a liner in-kernel buffer
> (not scatter gather). st_scsi_kern_execute internally uses
> blk_get_request, blk_rq_map_kern, and blk_execute_rq.
> 
> The majority of st_do_scsi can be replaced with
> st_scsi_kern_execute. This is a preparation for rewriting st_do_scsi
> to remove obsolete scsi_execute_async().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/st.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 878493d..6179940 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -533,6 +533,55 @@ st_do_scsi(struct st_request * SRpnt, struct scsi_tape * STp, unsigned char *cmd
>  	return SRpnt;
>  }
>  
> +static int st_scsi_kern_execute(struct st_request *streq,
> +				const unsigned char *cmd, int data_direction,
> +				void *buffer, unsigned bufflen, int timeout,
> +				int retries)
> +{
> +	struct scsi_tape *stp = streq->stp;
> +	struct request *req;
> +	int write = (data_direction == DMA_TO_DEVICE);
> +	int ret = 0;
> +
> +	/* st_do_scsi returns -EBUSY in case of OOM */
> +	req = blk_get_request(stp->device->request_queue, write, GFP_KERNEL);
> +	if (!req)
> +		return -EBUSY;
> +
> +	if (bufflen) {
> +		ret = blk_rq_map_kern(stp->device->request_queue, req,
> +				      buffer, bufflen, GFP_KERNEL);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req->cmd_flags |= REQ_QUIET;
> +
> +	req->cmd_len = COMMAND_SIZE(cmd[0]);
> +	memcpy(req->cmd, cmd, req->cmd_len);
> +
> +	req->sense = streq->sense;
> +	req->sense_len = 0;
> +
> +	req->timeout = timeout;
> +	req->retries = retries;
> +
> +	stp->buffer->cmdstat.have_sense = 0;
> +	memcpy(streq->cmd, cmd, sizeof(streq->cmd));
> +
> +	blk_execute_rq(req->q, NULL, req, 1);
> +
> +	stp->buffer->cmdstat.midlevel_result = streq->result = req->errors;
> +	stp->buffer->cmdstat.residual = req->data_len;
> +

TOMO Hi.

would you say that the only reason we cannot use scsi_execute inside st_scsi_kern_execute
is because we don't have residual count returned from scsi_execute ?

> +	stp->buffer->syscall_result = st_chk_result(stp, streq);
> +
> +out:
> +	blk_put_request(req);
> +
> +	return ret;
> +}
>  
>  /* Handle the write-behind checking (waits for completion). Returns -ENOSPC if
>     write has been correct but EOM early warning reached, -EIO if write ended in

The reason I'm asking is because I needed the same from scsi_execute in the passed.

Boaz

Re: [PATCH 09/11] st: convert read_mode_page to use st_scsi_kern_execute

[Boaz Harrosh]

FUJITA Tomonori wrote:
> This replaces st_do_scsi in read_mode_page (MODE_SENSE) with
> st_scsi_kern_execute.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/st.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index cc085bc..91d1249 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -2393,7 +2393,8 @@ static int st_set_options(struct scsi_tape *STp, long options)
>  static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
>  {
>  	unsigned char cmd[MAX_COMMAND_SIZE];
> -	struct st_request *SRpnt = NULL;
> +	struct st_request *SRpnt;
> +	int ret;
>  
>  	memset(cmd, 0, MAX_COMMAND_SIZE);
>  	cmd[0] = MODE_SENSE;
> @@ -2402,10 +2403,15 @@ static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
>  	cmd[2] = page;
>  	cmd[4] = 255;
>  
> -	SRpnt = st_do_scsi(SRpnt, STp, cmd, cmd[4], DMA_FROM_DEVICE,
> -			   STp->device->timeout, 0, 1);
> -	if (SRpnt == NULL)
> -		return (STp->buffer)->syscall_result;
> +	SRpnt = st_allocate_request(STp);
> +	if (!SRpnt)
> +		return STp->buffer->syscall_result;
> +
> +	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
> +				   STp->buffer->b_data, cmd[4],
> +				   STp->device->timeout, MAX_RETRIES);
> +	if (ret)
> +		return ret;

Are you sure you want to return here without st_release_request() ?
All other patches return ret but release first.

>  
>  	st_release_request(SRpnt);
>  

Boaz

Re: [PATCH 02/11] st: add st_scsi_kern_execute helper function

[FUJITA Tomonori]

On Sun, 30 Nov 2008 14:10:56 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> would you say that the only reason we cannot use scsi_execute inside st_scsi_kern_execute
> is because we don't have residual count returned from scsi_execute ?

Yeah, it's one of reasons. I don't want to create dependence. But once
the patchset is merged, I might try to change scsi_execute() for st.

But maybe not since I might change st_scsi_kern_execute to handle
st_read/st_write data transfer (between user and kernel space).

Re: [PATCH 09/11] st: convert read_mode_page to use st_scsi_kern_execute

[FUJITA Tomonori]

On Sun, 30 Nov 2008 14:12:41 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > This replaces st_do_scsi in read_mode_page (MODE_SENSE) with
> > st_scsi_kern_execute.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/scsi/st.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> > index cc085bc..91d1249 100644
> > --- a/drivers/scsi/st.c
> > +++ b/drivers/scsi/st.c
> > @@ -2393,7 +2393,8 @@ static int st_set_options(struct scsi_tape *STp, long options)
> >  static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
> >  {
> >  	unsigned char cmd[MAX_COMMAND_SIZE];
> > -	struct st_request *SRpnt = NULL;
> > +	struct st_request *SRpnt;
> > +	int ret;
> >  
> >  	memset(cmd, 0, MAX_COMMAND_SIZE);
> >  	cmd[0] = MODE_SENSE;
> > @@ -2402,10 +2403,15 @@ static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
> >  	cmd[2] = page;
> >  	cmd[4] = 255;
> >  
> > -	SRpnt = st_do_scsi(SRpnt, STp, cmd, cmd[4], DMA_FROM_DEVICE,
> > -			   STp->device->timeout, 0, 1);
> > -	if (SRpnt == NULL)
> > -		return (STp->buffer)->syscall_result;
> > +	SRpnt = st_allocate_request(STp);
> > +	if (!SRpnt)
> > +		return STp->buffer->syscall_result;
> > +
> > +	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
> > +				   STp->buffer->b_data, cmd[4],
> > +				   STp->device->timeout, MAX_RETRIES);
> > +	if (ret)
> > +		return ret;
> 
> Are you sure you want to return here without st_release_request() ?
> All other patches return ret but release first.

Ah, thanks a lot.

It should do something like this.

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index cc085bc..cf8861f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2393,7 +2393,8 @@ static int st_set_options(struct scsi_tape *STp, long options)
 static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
 {
 	unsigned char cmd[MAX_COMMAND_SIZE];
-	struct st_request *SRpnt = NULL;
+	struct st_request *SRpnt;
+	int ret;
 
 	memset(cmd, 0, MAX_COMMAND_SIZE);
 	cmd[0] = MODE_SENSE;
@@ -2402,14 +2403,16 @@ static int read_mode_page(struct scsi_tape *STp, int page, int omit_block_descs)
 	cmd[2] = page;
 	cmd[4] = 255;
 
-	SRpnt = st_do_scsi(SRpnt, STp, cmd, cmd[4], DMA_FROM_DEVICE,
-			   STp->device->timeout, 0, 1);
-	if (SRpnt == NULL)
-		return (STp->buffer)->syscall_result;
+	SRpnt = st_allocate_request(STp);
+	if (!SRpnt)
+		return STp->buffer->syscall_result;
 
+	ret = st_scsi_kern_execute(SRpnt, cmd, DMA_FROM_DEVICE,
+				   STp->buffer->b_data, cmd[4],
+				   STp->device->timeout, MAX_RETRIES);
 	st_release_request(SRpnt);
 
-	return (STp->buffer)->syscall_result;
+	return ret ? : STp->buffer->syscall_result;
 }
 
 

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Vladislav Bolkhovitin]

FUJITA Tomonori wrote:
> This patchset removes the majority of scsi_execute_async in st
> driver. IOW, this converts st driver to use the block layer functions.
> 
> We are in the process of removing scsi_execute_async and
> scsi_req_map_sg. scsi_execute_async in sg driver were removed in
> 2.6.27. Now only st and osst drivers use scsi_execute_async().
> 
> st driver uses scsi_execute_async for two purposes, performing sort
> management SCSI commands internally and data transfer between user and
> kernel space (st_read and st_write). This patchset converts the
> former.
> 
> The former performs SCSI commands synchronously. It uses a liner
> in-kernel buffer (not scatter gather) for data transfer. To perform
> such commands, other upper layer drivers (e.g. sd) use
> scsi_execute_req (internally uses blk_rq_map_kern and and
> blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
> helper function similar to scsi_execute_req and replace
> scsi_execute_async in st driver (I might modify scsi_execute_req for
> st and remove the helper function later).
> 
> I'm still working on converting scsi_execute_async for data transfer
> between user and kernel space but I want to get this first half be
> merged.

May I ask a possibly stupid question? Is amount of overall code in Linux 
SCSI subsystem after your conversion is fully done and 
scsi_execute_async() with scsi_req_map_sg() completely removed going to 
get smaller or bigger?

 From diffstats of your patches seems it's is going to get considerably 
bigger (171 insertion vs 61 deletion). And this is only in one user of 
scsi_execute_async() of 4. What's the point then in scsi_execute_async() 
removal if the resulting code is going to get bigger, not smaller? And, 
frankly, it doesn't look as it's going to get clearer too..

P.S. Scsi_execute_async() is just ~50 lines long, scsi_req_map_sg() is 
just ~80 lines long and both look like nice helper functions, which are 
not worse than st_scsi_kern_execute().

Regards,
Vlad

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Boaz Harrosh]

Vladislav Bolkhovitin wrote:
> FUJITA Tomonori wrote:
>> This patchset removes the majority of scsi_execute_async in st
>> driver. IOW, this converts st driver to use the block layer functions.
>>
>> We are in the process of removing scsi_execute_async and
>> scsi_req_map_sg. scsi_execute_async in sg driver were removed in
>> 2.6.27. Now only st and osst drivers use scsi_execute_async().
>>
>> st driver uses scsi_execute_async for two purposes, performing sort
>> management SCSI commands internally and data transfer between user and
>> kernel space (st_read and st_write). This patchset converts the
>> former.
>>
>> The former performs SCSI commands synchronously. It uses a liner
>> in-kernel buffer (not scatter gather) for data transfer. To perform
>> such commands, other upper layer drivers (e.g. sd) use
>> scsi_execute_req (internally uses blk_rq_map_kern and and
>> blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
>> helper function similar to scsi_execute_req and replace
>> scsi_execute_async in st driver (I might modify scsi_execute_req for
>> st and remove the helper function later).
>>
>> I'm still working on converting scsi_execute_async for data transfer
>> between user and kernel space but I want to get this first half be
>> merged.
> 
> May I ask a possibly stupid question? Is amount of overall code in Linux 
> SCSI subsystem after your conversion is fully done and 
> scsi_execute_async() with scsi_req_map_sg() completely removed going to 
> get smaller or bigger?
> 
>  From diffstats of your patches seems it's is going to get considerably 
> bigger (171 insertion vs 61 deletion). And this is only in one user of 
> scsi_execute_async() of 4. What's the point then in scsi_execute_async() 
> removal if the resulting code is going to get bigger, not smaller? And, 
> frankly, it doesn't look as it's going to get clearer too..
> 
> P.S. Scsi_execute_async() is just ~50 lines long, scsi_req_map_sg() is 
> just ~80 lines long and both look like nice helper functions, which are 
> not worse than st_scsi_kern_execute().
> 
> Regards,
> Vlad
> 

No! you are not looking at the full picture. See previous conversion of
sg.c driver. What you see here is only the rq_map_kern  side which is
fine. But the ugliness starts with when you need sg-lists.

With Scsi_execute_async you go from:
	memory=>sg-lists=>bios=>sg-lists=>driver
going directly to blk you save the allocation of the first sg-list and
translation to BIOs. And the ugly self-made preparation of the sg-list
by each driver again and again. Not talking about correctness. Like when
the block layer takes care of alignment, draining, sizes and all that funny
stuff all kind of drivers need (sa*cough*ta)

The net effect is a very big gain. both in code and specially in resources
and performance.

Note that all users of block layer have user-memory, kernel-pointer, page*.
Only target driver happens to be on the other side and actually hold an sg-list
at hand. That's because, in principle, it is down-side-up.

Boaz

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Vladislav Bolkhovitin]

Boaz Harrosh wrote:
> Vladislav Bolkhovitin wrote:
>> FUJITA Tomonori wrote:
>>> This patchset removes the majority of scsi_execute_async in st
>>> driver. IOW, this converts st driver to use the block layer functions.
>>>
>>> We are in the process of removing scsi_execute_async and
>>> scsi_req_map_sg. scsi_execute_async in sg driver were removed in
>>> 2.6.27. Now only st and osst drivers use scsi_execute_async().
>>>
>>> st driver uses scsi_execute_async for two purposes, performing sort
>>> management SCSI commands internally and data transfer between user and
>>> kernel space (st_read and st_write). This patchset converts the
>>> former.
>>>
>>> The former performs SCSI commands synchronously. It uses a liner
>>> in-kernel buffer (not scatter gather) for data transfer. To perform
>>> such commands, other upper layer drivers (e.g. sd) use
>>> scsi_execute_req (internally uses blk_rq_map_kern and and
>>> blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
>>> helper function similar to scsi_execute_req and replace
>>> scsi_execute_async in st driver (I might modify scsi_execute_req for
>>> st and remove the helper function later).
>>>
>>> I'm still working on converting scsi_execute_async for data transfer
>>> between user and kernel space but I want to get this first half be
>>> merged.
>> May I ask a possibly stupid question? Is amount of overall code in Linux 
>> SCSI subsystem after your conversion is fully done and 
>> scsi_execute_async() with scsi_req_map_sg() completely removed going to 
>> get smaller or bigger?
>>
>>  From diffstats of your patches seems it's is going to get considerably 
>> bigger (171 insertion vs 61 deletion). And this is only in one user of 
>> scsi_execute_async() of 4. What's the point then in scsi_execute_async() 
>> removal if the resulting code is going to get bigger, not smaller? And, 
>> frankly, it doesn't look as it's going to get clearer too..
>>
>> P.S. Scsi_execute_async() is just ~50 lines long, scsi_req_map_sg() is 
>> just ~80 lines long and both look like nice helper functions, which are 
>> not worse than st_scsi_kern_execute().
>>
>> Regards,
>> Vlad
>>
> 
> No! you are not looking at the full picture. See previous conversion of
> sg.c driver. What you see here is only the rq_map_kern  side which is
> fine. But the ugliness starts with when you need sg-lists.
> 
> With Scsi_execute_async you go from:
> 	memory=>sg-lists=>bios=>sg-lists=>driver
> going directly to blk you save the allocation of the first sg-list and
> translation to BIOs. And the ugly self-made preparation of the sg-list
> by each driver again and again. Not talking about correctness. Like when
> the block layer takes care of alignment, draining, sizes and all that funny
> stuff all kind of drivers need (sa*cough*ta)
> 
> The net effect is a very big gain. both in code and specially in resources
> and performance.
> 
> Note that all users of block layer have user-memory, kernel-pointer, page*.
> Only target driver happens to be on the other side and actually hold an sg-list
> at hand. That's because, in principle, it is down-side-up.

I see. If it's about performance, it should be done, no doubts. But I 
have another likely stupid question, answer for which I have been going 
to find out for long time, but always have no time. Maybe, you can help 
me, BTW, please? Why is block layer involved in SCSI commands execution 
for non-block cases, like st and sg? Sg isn't intended to be actively 
used for data transfers with block devices, for them there are sd and 
bsg interfaces, right? Wouldn't for them path memory=>sg-lists=>driver 
via SCSI mid-layer be simpler and all those alignment, draining, sizes, 
etc. issues can't be handled by some library, which both SCSI and block 
subsystems use? I have feeling, maybe wrong, that currently performance 
for non-block cases is sacrificed for block case.

Thanks,
Vlad

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Boaz Harrosh]

Vladislav Bolkhovitin wrote:
> Boaz Harrosh wrote:
>> Vladislav Bolkhovitin wrote:
>>> FUJITA Tomonori wrote:
>>>> This patchset removes the majority of scsi_execute_async in st
>>>> driver. IOW, this converts st driver to use the block layer functions.
>>>>
>>>> We are in the process of removing scsi_execute_async and
>>>> scsi_req_map_sg. scsi_execute_async in sg driver were removed in
>>>> 2.6.27. Now only st and osst drivers use scsi_execute_async().
>>>>
>>>> st driver uses scsi_execute_async for two purposes, performing sort
>>>> management SCSI commands internally and data transfer between user and
>>>> kernel space (st_read and st_write). This patchset converts the
>>>> former.
>>>>
>>>> The former performs SCSI commands synchronously. It uses a liner
>>>> in-kernel buffer (not scatter gather) for data transfer. To perform
>>>> such commands, other upper layer drivers (e.g. sd) use
>>>> scsi_execute_req (internally uses blk_rq_map_kern and and
>>>> blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
>>>> helper function similar to scsi_execute_req and replace
>>>> scsi_execute_async in st driver (I might modify scsi_execute_req for
>>>> st and remove the helper function later).
>>>>
>>>> I'm still working on converting scsi_execute_async for data transfer
>>>> between user and kernel space but I want to get this first half be
>>>> merged.
>>> May I ask a possibly stupid question? Is amount of overall code in Linux 
>>> SCSI subsystem after your conversion is fully done and 
>>> scsi_execute_async() with scsi_req_map_sg() completely removed going to 
>>> get smaller or bigger?
>>>
>>>  From diffstats of your patches seems it's is going to get considerably 
>>> bigger (171 insertion vs 61 deletion). And this is only in one user of 
>>> scsi_execute_async() of 4. What's the point then in scsi_execute_async() 
>>> removal if the resulting code is going to get bigger, not smaller? And, 
>>> frankly, it doesn't look as it's going to get clearer too..
>>>
>>> P.S. Scsi_execute_async() is just ~50 lines long, scsi_req_map_sg() is 
>>> just ~80 lines long and both look like nice helper functions, which are 
>>> not worse than st_scsi_kern_execute().
>>>
>>> Regards,
>>> Vlad
>>>
>> No! you are not looking at the full picture. See previous conversion of
>> sg.c driver. What you see here is only the rq_map_kern  side which is
>> fine. But the ugliness starts with when you need sg-lists.
>>
>> With Scsi_execute_async you go from:
>> 	memory=>sg-lists=>bios=>sg-lists=>driver
>> going directly to blk you save the allocation of the first sg-list and
>> translation to BIOs. And the ugly self-made preparation of the sg-list
>> by each driver again and again. Not talking about correctness. Like when
>> the block layer takes care of alignment, draining, sizes and all that funny
>> stuff all kind of drivers need (sa*cough*ta)
>>
>> The net effect is a very big gain. both in code and specially in resources
>> and performance.
>>
>> Note that all users of block layer have user-memory, kernel-pointer, page*.
>> Only target driver happens to be on the other side and actually hold an sg-list
>> at hand. That's because, in principle, it is down-side-up.
> 
> I see. If it's about performance, it should be done, no doubts. But I 
> have another likely stupid question, answer for which I have been going 
> to find out for long time, but always have no time. Maybe, you can help 
> me, BTW, please? Why is block layer involved in SCSI commands execution 
> for non-block cases, like st and sg? Sg isn't intended to be actively 
> used for data transfers with block devices, for them there are sd and 
> bsg interfaces, right? Wouldn't for them path memory=>sg-lists=>driver 
> via SCSI mid-layer be simpler and all those alignment, draining, sizes, 
> etc. issues can't be handled by some library, which both SCSI and block 
> subsystems use? I have feeling, maybe wrong, that currently performance 
> for non-block cases is sacrificed for block case.
> 
> Thanks,
> Vlad
> 

Hi Vlad, it used to be like that before 2.6.17. And there was one big mess
and lots of bugs in regard to alignment, draining, sizes, bounce buffers
device masks, emergency pools, to name a few.

So there is a library that takes care of that, the block layer. The block
layer is very thin in regard to BLOCK_PC commands. It's just that library
you are talking about and a queue to submit commands. Not using it would
be a duplication of code, and would lead to the mess it used to be.

However perhaps there is one optimization that could be preformed at
block layer, which is as it is because of historical reasons. The BIOs
could hold sg-lists instead of biovecs, which are exactly half of an
sg. The reason it is like that is, because the majority of block devices
at the time did not use sg-lists to map DMA buffers, hell 2/3 of them did
not do scatter-gather DMA at all. So it was left like that.
Note that sg-lists are 2 times bigger then biovecs but if the big majority
go through sg-list anyway, in the life span of a request, then it might
be advantageous. But, there is always a but, we will need to take care of
cases where in a retry or split requests the original bio is always kept
intact, the sg-list which is a copy is destroyed and remade if needed.

There you have my $0.017
Boaz

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Kai Makisara]

On Sun, 30 Nov 2008, FUJITA Tomonori wrote:

> This patchset removes the majority of scsi_execute_async in st
> driver. IOW, this converts st driver to use the block layer functions.
> 
> We are in the process of removing scsi_execute_async and
> scsi_req_map_sg. scsi_execute_async in sg driver were removed in
> 2.6.27. Now only st and osst drivers use scsi_execute_async().
> 
> st driver uses scsi_execute_async for two purposes, performing sort
> management SCSI commands internally and data transfer between user and
> kernel space (st_read and st_write). This patchset converts the
> former.
> 
> The former performs SCSI commands synchronously. It uses a liner
> in-kernel buffer (not scatter gather) for data transfer. To perform
> such commands, other upper layer drivers (e.g. sd) use
> scsi_execute_req (internally uses blk_rq_map_kern and and
> blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
> helper function similar to scsi_execute_req and replace
> scsi_execute_async in st driver (I might modify scsi_execute_req for
> st and remove the helper function later).
> 
> I'm still working on converting scsi_execute_async for data transfer
> between user and kernel space but I want to get this first half be
> merged.
> 
I have looked at the patches but it is a little difficult to say much 
without seeing the other half. I think there should be only one function 
in st to handle scsi. If you extend st_scsi_kern_execute to handle also 
read and write (as you later indicate you may do) I think that is a good 
direction.

I tested the patches in 2.6.28-rc7 and noticed a couple of problems. The 
patches do not convert st_int_ioctl. I added simple conversion (at the end 
of this message) but after this, the driver did not pass my simple tests. 
Looking at st_scsi_kern_execute(), it seems that it does not copy the 
sense data to struct st_request.

Thanks,
Kai

--- linux-t1/drivers/scsi/st.c.org	2008-12-02 20:14:11.000000000 +0200
+++ linux-t1/drivers/scsi/st.c	2008-12-02 20:34:49.000000000 +0200
@@ -2872,12 +2872,14 @@ static int st_int_ioctl(struct scsi_tape
 		return (-ENOSYS);
 	}
 
-	SRpnt = st_do_scsi(NULL, STp, cmd, datalen, direction,
-			   timeout, MAX_RETRIES, 1);
+
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
-		return (STp->buffer)->syscall_result;
+		return STp->buffer->syscall_result;
 
-	ioctl_result = (STp->buffer)->syscall_result;
+	ioctl_result = st_scsi_kern_execute(SRpnt, cmd, direction,
+					    STp->buffer->b_data, datalen,
+					    timeout, MAX_RETRIES);
 
 	if (!ioctl_result) {	/* SCSI command successful */
 		st_release_request(SRpnt);

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[FUJITA Tomonori]

On Tue, 2 Dec 2008 21:08:19 +0200 (EET)
Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:

> On Sun, 30 Nov 2008, FUJITA Tomonori wrote:
> 
> > This patchset removes the majority of scsi_execute_async in st
> > driver. IOW, this converts st driver to use the block layer functions.
> > 
> > We are in the process of removing scsi_execute_async and
> > scsi_req_map_sg. scsi_execute_async in sg driver were removed in
> > 2.6.27. Now only st and osst drivers use scsi_execute_async().
> > 
> > st driver uses scsi_execute_async for two purposes, performing sort
> > management SCSI commands internally and data transfer between user and
> > kernel space (st_read and st_write). This patchset converts the
> > former.
> > 
> > The former performs SCSI commands synchronously. It uses a liner
> > in-kernel buffer (not scatter gather) for data transfer. To perform
> > such commands, other upper layer drivers (e.g. sd) use
> > scsi_execute_req (internally uses blk_rq_map_kern and and
> > blk_execute_rq). scsi_execute_req is not fully fit for st so I adds a
> > helper function similar to scsi_execute_req and replace
> > scsi_execute_async in st driver (I might modify scsi_execute_req for
> > st and remove the helper function later).
> > 
> > I'm still working on converting scsi_execute_async for data transfer
> > between user and kernel space but I want to get this first half be
> > merged.
> > 
> I have looked at the patches but it is a little difficult to say much 
> without seeing the other half. I think there should be only one function 
> in st to handle scsi. If you extend st_scsi_kern_execute to handle also 
> read and write (as you later indicate you may do) I think that is a good 
> direction.

First, thanks a lot for taking a look at the patchset!

Yeah, I would extend st_scsi_kern_execute to handle st_read and
st_write (and remove st_do_scsi).

Or I would replace st_scsi_kern_execute with scsi_execute in
scsi_lib.c (then there is no homegrown function to perform internal
management SCSI commands in st driver).


> I tested the patches in 2.6.28-rc7 and noticed a couple of problems. The 
> patches do not convert st_int_ioctl.

Yeah, I left it alone since I didn't finish testing st_int_ioctl. But
you are right, st_int_ioctl does in-kernel data transfer so it needs
to be converted with st_scsi_kern_execute.


> I added simple conversion (at the end 
> of this message) but after this, the driver did not pass my simple tests. 
> Looking at st_scsi_kern_execute(), it seems that it does not copy the 
> sense data to struct st_request.

I think that blk_execute_rq copies the sense data for us. So we don't
need to worry about it.

Without your patch, that is, with only my patchset, the driver passed
your tests?

If so, can you try this patch to convert st_int_ioctl?


Do you prefer me to change st_scsi_kern_execute to set -EBUSY to
syscall_result if st_scsi_kern_execute internally fails (that is,
blk_rq_map_kern or blk_get_request fails)?

Note that scsi_execute_async (which st_do_scsi uses) could fail
internally. scsi_execute_async and st_scsi_kern_execute uses the same
functions that could fail, blk_rq_map_kern or blk_get_request


diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 02c3138..a70401f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2872,12 +2872,15 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 		return (-ENOSYS);
 	}
 
-	SRpnt = st_do_scsi(NULL, STp, cmd, datalen, direction,
-			   timeout, MAX_RETRIES, 1);
+	SRpnt = st_allocate_request(STp);
 	if (!SRpnt)
 		return (STp->buffer)->syscall_result;
 
-	ioctl_result = (STp->buffer)->syscall_result;
+	ioctl_result = st_scsi_kern_execute(SRpnt, cmd, direction,
+					    STp->buffer->b_data, datalen,
+					    timeout, MAX_RETRIES);
+	if (!ioctl_result)
+		ioctl_result = (STp->buffer)->syscall_result;
 
 	if (!ioctl_result) {	/* SCSI command successful */
 		st_release_request(SRpnt);

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Kai Makisara]

On Wed, 3 Dec 2008, FUJITA Tomonori wrote:

> On Tue, 2 Dec 2008 21:08:19 +0200 (EET)
> Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:
> 
...
> > I added simple conversion (at the end 
> > of this message) but after this, the driver did not pass my simple tests. 
> > Looking at st_scsi_kern_execute(), it seems that it does not copy the 
> > sense data to struct st_request.
> 
> I think that blk_execute_rq copies the sense data for us. So we don't
> need to worry about it.
> 
Now that I looked more carefully, it does that.

> Without your patch, that is, with only my patchset, the driver passed
> your tests?
> 
Yes.

> If so, can you try this patch to convert st_int_ioctl?
> 
I replaced my patch with yours. Now the tests pass. I did some tests with 
debugging enabled and those showed that the sense data is returned 
correctly.

> 
> Do you prefer me to change st_scsi_kern_execute to set -EBUSY to
> syscall_result if st_scsi_kern_execute internally fails (that is,
> blk_rq_map_kern or blk_get_request fails)?
> 
I am not sure that -EBUSY is a valid error return any more. Should the 
error be -ENOMEM if blk_get_request fails and otherwise return what 
blk_rq_map_kern returns?

> Note that scsi_execute_async (which st_do_scsi uses) could fail
> internally. scsi_execute_async and st_scsi_kern_execute uses the same
> functions that could fail, blk_rq_map_kern or blk_get_request
> 
Yes. I am not sure that -EBUSY is correct return value there.

Thanks,
Kai

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[FUJITA Tomonori]

On Wed, 3 Dec 2008 21:40:53 +0200 (EET)
Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:

> > If so, can you try this patch to convert st_int_ioctl?
> > 
> I replaced my patch with yours. Now the tests pass. I did some tests with 
> debugging enabled and those showed that the sense data is returned 
> correctly.

Great, then can I get your ACK on this patchset?


> > Do you prefer me to change st_scsi_kern_execute to set -EBUSY to
> > syscall_result if st_scsi_kern_execute internally fails (that is,
> > blk_rq_map_kern or blk_get_request fails)?
> > 
> I am not sure that -EBUSY is a valid error return any more. Should the 
> error be -ENOMEM if blk_get_request fails and otherwise return what 
> blk_rq_map_kern returns?
> 
> > Note that scsi_execute_async (which st_do_scsi uses) could fail
> > internally. scsi_execute_async and st_scsi_kern_execute uses the same
> > functions that could fail, blk_rq_map_kern or blk_get_request
> > 
> Yes. I am not sure that -EBUSY is correct return value there.

I think that it's the right thing to return an error value to user
space that scsi_execute_async returns but st_do_scsi uses has returned
-EBUSY for any failure for long time so it might be safer to keep the
current behavior.

I'll change st_scsi_kern_execute to set -EBUSY in case of internal
failure in the next submission if you prefer.


Thanks,

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[Kai Makisara]

On Thu, 4 Dec 2008, FUJITA Tomonori wrote:

> On Wed, 3 Dec 2008 21:40:53 +0200 (EET)
> Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:
> 
> > > If so, can you try this patch to convert st_int_ioctl?
> > > 
> > I replaced my patch with yours. Now the tests pass. I did some tests with 
> > debugging enabled and those showed that the sense data is returned 
> > correctly.
> 
> Great, then can I get your ACK on this patchset?
> 
Yes, you can add
Acked-by: Kai Makisara <Kai.Makisara@kolumbus.fi>

> 
> > > Do you prefer me to change st_scsi_kern_execute to set -EBUSY to
> > > syscall_result if st_scsi_kern_execute internally fails (that is,
> > > blk_rq_map_kern or blk_get_request fails)?
> > > 
> > I am not sure that -EBUSY is a valid error return any more. Should the 
> > error be -ENOMEM if blk_get_request fails and otherwise return what 
> > blk_rq_map_kern returns?
> > 
> > > Note that scsi_execute_async (which st_do_scsi uses) could fail
> > > internally. scsi_execute_async and st_scsi_kern_execute uses the same
> > > functions that could fail, blk_rq_map_kern or blk_get_request
> > > 
> > Yes. I am not sure that -EBUSY is correct return value there.
> 
> I think that it's the right thing to return an error value to user
> space that scsi_execute_async returns but st_do_scsi uses has returned
> -EBUSY for any failure for long time so it might be safer to keep the
> current behavior.
> 
Retaining the old behaviour is always a safe solution but sometimes it is 
time to improve code. Note that scsi_execute_async() only returns 
(DRIVER_ERROR << 24) in case of error. This does not allow using different 
error codes for different error reasons. With st_scsi_kern_execute it is 
possible.

> I'll change st_scsi_kern_execute to set -EBUSY in case of internal
> failure in the next submission if you prefer.
> 
I don't like returning -EBUSY always now that we have a choice. However, 
if you think the conservative solution of returning -EBUSY is better, I 
don't object.

Thanks,
Kai

Re: [PATCH 00/11] st: remove scsi_execute_async usage (the first half)

[FUJITA Tomonori]

On Thu, 4 Dec 2008 22:23:06 +0200 (EET)
Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:

> On Thu, 4 Dec 2008, FUJITA Tomonori wrote:
> 
> > On Wed, 3 Dec 2008 21:40:53 +0200 (EET)
> > Kai Makisara <Kai.Makisara@kolumbus.fi> wrote:
> > 
> > > > If so, can you try this patch to convert st_int_ioctl?
> > > > 
> > > I replaced my patch with yours. Now the tests pass. I did some tests with 
> > > debugging enabled and those showed that the sense data is returned 
> > > correctly.
> > 
> > Great, then can I get your ACK on this patchset?
> > 
> Yes, you can add
> Acked-by: Kai Makisara <Kai.Makisara@kolumbus.fi>

Thanks!


> > > > Do you prefer me to change st_scsi_kern_execute to set -EBUSY to
> > > > syscall_result if st_scsi_kern_execute internally fails (that is,
> > > > blk_rq_map_kern or blk_get_request fails)?
> > > > 
> > > I am not sure that -EBUSY is a valid error return any more. Should the 
> > > error be -ENOMEM if blk_get_request fails and otherwise return what 
> > > blk_rq_map_kern returns?
> > > 
> > > > Note that scsi_execute_async (which st_do_scsi uses) could fail
> > > > internally. scsi_execute_async and st_scsi_kern_execute uses the same
> > > > functions that could fail, blk_rq_map_kern or blk_get_request
> > > > 
> > > Yes. I am not sure that -EBUSY is correct return value there.
> > 
> > I think that it's the right thing to return an error value to user
> > space that scsi_execute_async returns but st_do_scsi uses has returned
> > -EBUSY for any failure for long time so it might be safer to keep the
> > current behavior.
> > 
> Retaining the old behaviour is always a safe solution but sometimes it is 
> time to improve code. Note that scsi_execute_async() only returns 
> (DRIVER_ERROR << 24) in case of error.

Ah, right, I overlooked it.


> This does not allow using different 
> error codes for different error reasons. With st_scsi_kern_execute it is 
> possible.
> 
> > I'll change st_scsi_kern_execute to set -EBUSY in case of internal
> > failure in the next submission if you prefer.
> > 
> I don't like returning -EBUSY always now that we have a choice. However, 
> if you think the conservative solution of returning -EBUSY is better, I 
> don't object.

Thanks, I see.

I simplified st_scsi_kern_execute by using scsi_execute. scsi_execute
returns DRIVER_ERROR << 24 for internal failures like
scsi_execute_async does. I changed st_scsi_kern_execute to return
-EBUSY for now as st_do_scsi does.

st_scsi_kern_execute would be changed again in the second half so this
might be changed. But I think if we change return code, it would be
better to do it in a separate patch (explicitly).