[PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Kiyoshi Ueda]

This patch converts bidi of scsi mid-layer to use blk_end_request().

rq->next_rq represents a pair of bidi requests.
(There are no other use of 'next_rq' of struct request.)
For both requests in the pair, end_that_request_chunk() should be
called before end_that_request_last() is called for one of them.
Since the calls to end_that_request_first()/chunk() and
end_that_request_last() are packaged into blk_end_request(),
the handling of next_rq completion has to be moved into
blk_end_request(), too.

Bidi sets its specific value to rq->data_len before the request is
completed so that upper-layer can read it.
This setting must be between end_that_request_chunk() and
end_that_request_last(), because rq->data_len may be used
in end_that_request_chunk() by blk_trace and so on.
To satisfy the requirement, use blk_end_request_callback() which
is added in PATCH 25 only for the tricky drivers.

If bidi didn't reuse rq->data_len and added new members to request
for the specific value, it could set before end_that_request_chunk()
and use the standard blk_end_request() like below.

void scsi_end_bidi_request(struct scsi_cmnd *cmd)
{
	struct request *req = cmd->request;

	rq->resid = scsi_out(cmd)->resid;
	rq->next_rq->resid = scsi_in(cmd)->resid;

	if (blk_end_request(req, 1, req->data_len))
		BUG();

	scsi_release_buffers(cmd);
	scsi_next_command(cmd);
}

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 block/ll_rw_blk.c       |   18 +++++++++++++
 drivers/scsi/scsi_lib.c |   66 ++++++++++++++++++++++++------------------------
 2 files changed, 52 insertions(+), 32 deletions(-)

Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
===================================================================
--- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
+++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
@@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
 		scsi_run_queue(sdev->request_queue);
 }
 
-static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-	unsigned long flags;
-
-	add_disk_randomness(req->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	if (blk_rq_tagged(req))
-		blk_queue_end_tag(q, req);
-
-	end_that_request_last(req, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_next_command(cmd);
-}
-
 /*
  * Function:    scsi_end_request()
  *
@@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
 EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
+ * Called from blk_end_request_callback() after all DATA in rq and its next_rq
+ * are completed before rq is completed/freed.
+ */
+static int scsi_end_bidi_request_cb(struct request *rq)
+{
+	struct scsi_cmnd *cmd = rq->special;
+
+	rq->data_len = scsi_out(cmd)->resid;
+	rq->next_rq->data_len = scsi_in(cmd)->resid;
+
+	return 0;
+}
+
+/*
  * 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
@@ -931,22 +923,32 @@ void scsi_end_bidi_request(struct scsi_c
 {
 	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);
-
 	/*
 	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
-	 *       in end_that_request_last() then this WARN_ON must be removed.
+	 *       in blk_end_request() then this WARN_ON must be removed.
 	 *       for now, upper-driver must have registered an end_io.
 	 */
 	WARN_ON(!req->end_io);
 
-	scsi_finalize_request(cmd, 1);
+	/*
+	 * blk_end_request() family take care of data completion of next_rq.
+	 *
+	 * req->data_len and req->next_rq->data_len must be set after
+	 * all data are completed, since they may be referenced during
+	 * the data completion process.
+	 * So use the callback feature of blk_end_request() here.
+	 *
+	 * NOTE: If bidi doesn't reuse the data_len field for upper-layer's
+	 *       reference (e.g. adds new members for it to struct request),
+	 *       we can use the standard blk_end_request() interface here.
+	 */
+	if (blk_end_request_callback(req, 1, req->data_len,
+				     scsi_end_bidi_request_cb))
+		/* req has not been completed */
+		BUG();
+
+	scsi_release_buffers(cmd);
+	scsi_next_command(cmd);
 }
 
 /*
Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
===================================================================
--- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
+++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
@@ -3817,6 +3817,12 @@ int blk_end_request(struct request *rq, 
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	add_disk_randomness(rq->rq_disk);
@@ -3840,6 +3846,12 @@ int __blk_end_request(struct request *rq
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	add_disk_randomness(rq->rq_disk);
@@ -3884,6 +3896,12 @@ int blk_end_request_callback(struct requ
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	/* Special feature for tricky drivers */

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Boaz Harrosh]

On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> This patch converts bidi of scsi mid-layer to use blk_end_request().
> 
> rq->next_rq represents a pair of bidi requests.
> (There are no other use of 'next_rq' of struct request.)
> For both requests in the pair, end_that_request_chunk() should be
> called before end_that_request_last() is called for one of them.
> Since the calls to end_that_request_first()/chunk() and
> end_that_request_last() are packaged into blk_end_request(),
> the handling of next_rq completion has to be moved into
> blk_end_request(), too.
> 
> Bidi sets its specific value to rq->data_len before the request is
> completed so that upper-layer can read it.
> This setting must be between end_that_request_chunk() and
> end_that_request_last(), because rq->data_len may be used
> in end_that_request_chunk() by blk_trace and so on.
> To satisfy the requirement, use blk_end_request_callback() which
> is added in PATCH 25 only for the tricky drivers.
> 
> If bidi didn't reuse rq->data_len and added new members to request
> for the specific value, it could set before end_that_request_chunk()
> and use the standard blk_end_request() like below.
> 
> void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> {
> 	struct request *req = cmd->request;
> 
> 	rq->resid = scsi_out(cmd)->resid;
> 	rq->next_rq->resid = scsi_in(cmd)->resid;
> 
> 	if (blk_end_request(req, 1, req->data_len))
> 		BUG();
> 
> 	scsi_release_buffers(cmd);
> 	scsi_next_command(cmd);
> }
> 
> Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> ---
>  block/ll_rw_blk.c       |   18 +++++++++++++
>  drivers/scsi/scsi_lib.c |   66 ++++++++++++++++++++++++------------------------
>  2 files changed, 52 insertions(+), 32 deletions(-)
> 
> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
>  		scsi_run_queue(sdev->request_queue);
>  }
>  
> -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> -{
> -	struct request_queue *q = cmd->device->request_queue;
> -	struct request *req = cmd->request;
> -	unsigned long flags;
> -
> -	add_disk_randomness(req->rq_disk);
> -
> -	spin_lock_irqsave(q->queue_lock, flags);
> -	if (blk_rq_tagged(req))
> -		blk_queue_end_tag(q, req);
> -
> -	end_that_request_last(req, uptodate);
> -	spin_unlock_irqrestore(q->queue_lock, flags);
> -
> -	/*
> -	 * This will goose the queue request function at the end, so we don't
> -	 * need to worry about launching another command.
> -	 */
> -	scsi_next_command(cmd);
> -}
> -
>  /*
>   * Function:    scsi_end_request()
>   *
> @@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
>  EXPORT_SYMBOL(scsi_release_buffers);
>  
>  /*
> + * Called from blk_end_request_callback() after all DATA in rq and its next_rq
> + * are completed before rq is completed/freed.
> + */
> +static int scsi_end_bidi_request_cb(struct request *rq)
> +{
> +	struct scsi_cmnd *cmd = rq->special;
> +
> +	rq->data_len = scsi_out(cmd)->resid;
> +	rq->next_rq->data_len = scsi_in(cmd)->resid;
> +
> +	return 0;
> +}
> +
> +/*
>   * 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
> @@ -931,22 +923,32 @@ void scsi_end_bidi_request(struct scsi_c
>  {
>  	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);
> -
>  	/*
>  	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> -	 *       in end_that_request_last() then this WARN_ON must be removed.
> +	 *       in blk_end_request() then this WARN_ON must be removed.
>  	 *       for now, upper-driver must have registered an end_io.
>  	 */
>  	WARN_ON(!req->end_io);
>  
> -	scsi_finalize_request(cmd, 1);
> +	/*
> +	 * blk_end_request() family take care of data completion of next_rq.
> +	 *
> +	 * req->data_len and req->next_rq->data_len must be set after
> +	 * all data are completed, since they may be referenced during
> +	 * the data completion process.
> +	 * So use the callback feature of blk_end_request() here.
> +	 *
> +	 * NOTE: If bidi doesn't reuse the data_len field for upper-layer's
> +	 *       reference (e.g. adds new members for it to struct request),
> +	 *       we can use the standard blk_end_request() interface here.
> +	 */
> +	if (blk_end_request_callback(req, 1, req->data_len,
> +				     scsi_end_bidi_request_cb))
> +		/* req has not been completed */
> +		BUG();
> +
> +	scsi_release_buffers(cmd);
> +	scsi_next_command(cmd);
>  }
>  
>  /*
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3817,6 +3817,12 @@ int blk_end_request(struct request *rq, 
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	add_disk_randomness(rq->rq_disk);
> @@ -3840,6 +3846,12 @@ int __blk_end_request(struct request *rq
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	add_disk_randomness(rq->rq_disk);
> @@ -3884,6 +3896,12 @@ int blk_end_request_callback(struct requ
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	/* Special feature for tricky drivers */

rq->data_len = scsi_out(cmd)->resid is Not Just a problem of bidi
it is a General problem of scsi residual handling, and user code.

Even today before any bidi. at scsi_lib.c at scsi_io_completion()
we do req->data_len = scsi_get_resid(cmd);
( or: req->data_len = cmd->resid; depends which version you look)
And then call scsi_end_request() which calls __end_that_request_first/last
So it is assumed even today that req->data_len is not touched by
__end_that_request_first/last unless __end_that_request_first returned
that there is more work to do and the command is resubmitted in which
case the resid information is discarded.

So if the regular resid handling is acceptable - Set req->data_len
before the call to __end_that_request_first/last, or blk_end_request()
in your case, then here goes your second client of the _callback and
it can be removed.
But if it is found that req->data_len is touched and the resid information
gets lost, than it should be fixed for the common uni-io case, by - for example
- pass resid to the blk_end_request() function.
(So in any way the _callback can go)

Boaz

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Kiyoshi Ueda]

Hi Boaz,

On Tue, 04 Dec 2007 15:39:12 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> > This patch converts bidi of scsi mid-layer to use blk_end_request().
> > 
> > rq->next_rq represents a pair of bidi requests.
> > (There are no other use of 'next_rq' of struct request.)
> > For both requests in the pair, end_that_request_chunk() should be
> > called before end_that_request_last() is called for one of them.
> > Since the calls to end_that_request_first()/chunk() and
> > end_that_request_last() are packaged into blk_end_request(),
> > the handling of next_rq completion has to be moved into
> > blk_end_request(), too.
> > 
> > Bidi sets its specific value to rq->data_len before the request is
> > completed so that upper-layer can read it.
> > This setting must be between end_that_request_chunk() and
> > end_that_request_last(), because rq->data_len may be used
> > in end_that_request_chunk() by blk_trace and so on.
> > To satisfy the requirement, use blk_end_request_callback() which
> > is added in PATCH 25 only for the tricky drivers.
> > 
> > If bidi didn't reuse rq->data_len and added new members to request
> > for the specific value, it could set before end_that_request_chunk()
> > and use the standard blk_end_request() like below.
> > 
> > void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> > {
> > 	struct request *req = cmd->request;
> > 
> > 	rq->resid = scsi_out(cmd)->resid;
> > 	rq->next_rq->resid = scsi_in(cmd)->resid;
> > 
> > 	if (blk_end_request(req, 1, req->data_len))
> > 		BUG();
> > 
> > 	scsi_release_buffers(cmd);
> > 	scsi_next_command(cmd);
> > }
...
snip
...
>
> rq->data_len = scsi_out(cmd)->resid is Not Just a problem of bidi
> it is a General problem of scsi residual handling, and user code.
> 
> Even today before any bidi. at scsi_lib.c at scsi_io_completion()
> we do req->data_len = scsi_get_resid(cmd);
> ( or: req->data_len = cmd->resid; depends which version you look)
> And then call scsi_end_request() which calls __end_that_request_first/last
> So it is assumed even today that req->data_len is not touched by
> __end_that_request_first/last unless __end_that_request_first returned
> that there is more work to do and the command is resubmitted in which
> case the resid information is discarded.
> 
> So if the regular resid handling is acceptable - Set req->data_len
> before the call to __end_that_request_first/last, or blk_end_request()
> in your case, then here goes your second client of the _callback and
> it can be removed.
> But if it is found that req->data_len is touched and the resid information
> gets lost, than it should be fixed for the common uni-io case, by - for example
> - pass resid to the blk_end_request() function.
> (So in any way the _callback can go)

Thank you for the explanation of scsi's rq->data_len usage.
I see that scsi usually uses rq->data_len for cmd->resid.

I have investigated the possibility of setting data_len before
the call to blk_end_request.
But no matter whether data_len is touched or not, we need a callback
for bidi.  So I would like to go with the current patch.

I explained the reason and some details below.


As far as I can see, rq->data_len is just referenced
by blk_add_trace_rq() in __end_that_request_first(), not modified.
And I don't change any logic around there in the block-layer.
So there shouldn't be any critical problem for scsi residual handing.
(although I'm not sure that scsi expectes cmd->resid to be traced
 by blk_trace.)

Anyway, I see that it is no critical problem for bidi to set cmd->resid
to rq->data_len before blk_end_request() call.
But if I do that, blk_end_request() can't get the next_rq's size
to complete in its code below.

> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;

So I will have to move next_rq completion to bidi and use _callback()
anyway like the following.
---------------------------------------------------------------------
static int dummy_cb(struct request *rq)
{
	return 1;
}

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;
 
	/* Complete only DATA of next_rq using _callback and dummy function */
	if (!blk_end_request_callback(req->next_rq, 1, next_dlen, dummy_cb))
		BUG();
 
	if (blk_end_request(req, 1, dlen))
		BUG();

	scsi_release_buffers(cmd);
	scsi_next_command(cmd);
}
---------------------------------------------------------------------

I prefer the current patch rather than the code like above,
since the code calls blk_end_request twice and looks trickier.
So I'd like to leave the patch but descriptions and comments in it.
Below is the patch, which updated only descriptions and comments.
If you still have some concerns on this, please let me know.



Subject: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

This patch converts bidi of scsi mid-layer to use blk_end_request
interfaces.

rq->next_rq represents a pair of bidi requests.
(There are no other use of 'next_rq' of struct request.)
For both requests in the pair, end_that_request_chunk() should be
called before end_that_request_last() is called for one of them.
Since the calls to end_that_request_first()/chunk() and
end_that_request_last() are packaged into blk_end_request(),
the handling of next_rq completion has to be moved into
blk_end_request(), too.

Since blk_end_request() family don't have any argument for
the completion size of next_rq, they use next_rq->data_len for it.
So bidi has to set the resid after blk_end_request() completes
the data of next_rq.
To satisfy the requirement, bidi uses blk_end_request_callback(),
which is added in PATCH 25 only for the tricky drivers.

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 block/ll_rw_blk.c       |   18 +++++++++++++
 drivers/scsi/scsi_lib.c |   62 +++++++++++++++++++++++-------------------------
 2 files changed, 48 insertions(+), 32 deletions(-)

Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
===================================================================
--- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
+++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
@@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
 		scsi_run_queue(sdev->request_queue);
 }
 
-static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-	unsigned long flags;
-
-	add_disk_randomness(req->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	if (blk_rq_tagged(req))
-		blk_queue_end_tag(q, req);
-
-	end_that_request_last(req, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_next_command(cmd);
-}
-
 /*
  * Function:    scsi_end_request()
  *
@@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
 EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
+ * Called from blk_end_request_callback() after all DATA in rq and its next_rq
+ * are completed before rq is completed/freed.
+ */
+static int scsi_end_bidi_request_cb(struct request *rq)
+{
+	struct scsi_cmnd *cmd = rq->special;
+
+	rq->data_len = scsi_out(cmd)->resid;
+	rq->next_rq->data_len = scsi_in(cmd)->resid;
+
+	return 0;
+}
+
+/*
  * 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
@@ -931,22 +923,28 @@ void scsi_end_bidi_request(struct scsi_c
 {
 	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);
-
 	/*
 	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
-	 *       in end_that_request_last() then this WARN_ON must be removed.
+	 *       in blk_end_request() then this WARN_ON must be removed.
 	 *       for now, upper-driver must have registered an end_io.
 	 */
 	WARN_ON(!req->end_io);
 
-	scsi_finalize_request(cmd, 1);
+	/*
+	 * blk_end_request() family take care of data completion of next_rq.
+	 * blk_end_request() family use next_rq->data_len for 
+	 * the completion data size of next_rq.
+	 * So resid can't be set before the data completion of next_rq
+	 * in blk_end_request().
+	 * To resolve that, use the callback feature of blk_end_request().
+	 */
+	if (blk_end_request_callback(req, 1, req->data_len,
+				     scsi_end_bidi_request_cb))
+		/* req has not been completed */
+		BUG();
+
+	scsi_release_buffers(cmd);
+	scsi_next_command(cmd);
 }
 
 /*
Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
===================================================================
--- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
+++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
@@ -3817,6 +3817,12 @@ int blk_end_request(struct request *rq, 
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	add_disk_randomness(rq->rq_disk);
@@ -3840,6 +3846,12 @@ int __blk_end_request(struct request *rq
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	add_disk_randomness(rq->rq_disk);
@@ -3884,6 +3896,12 @@ int blk_end_request_callback(struct requ
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate,
+					     blk_rq_bytes(rq->next_rq)))
+			return 1;
 	}
 
 	/* Special feature for tricky drivers */

Thanks,
Kiyoshi Ueda

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Boaz Harrosh]

On Thu, Dec 06 2007 at 2:26 +0200, Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> Hi Boaz,
> 
> On Tue, 04 Dec 2007 15:39:12 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
>>> This patch converts bidi of scsi mid-layer to use blk_end_request().
>>>
>>> rq->next_rq represents a pair of bidi requests.
>>> (There are no other use of 'next_rq' of struct request.)
>>> For both requests in the pair, end_that_request_chunk() should be
>>> called before end_that_request_last() is called for one of them.
>>> Since the calls to end_that_request_first()/chunk() and
>>> end_that_request_last() are packaged into blk_end_request(),
>>> the handling of next_rq completion has to be moved into
>>> blk_end_request(), too.
>>>
>>> Bidi sets its specific value to rq->data_len before the request is
>>> completed so that upper-layer can read it.
>>> This setting must be between end_that_request_chunk() and
>>> end_that_request_last(), because rq->data_len may be used
>>> in end_that_request_chunk() by blk_trace and so on.
>>> To satisfy the requirement, use blk_end_request_callback() which
>>> is added in PATCH 25 only for the tricky drivers.
>>>
>>> If bidi didn't reuse rq->data_len and added new members to request
>>> for the specific value, it could set before end_that_request_chunk()
>>> and use the standard blk_end_request() like below.
>>>
>>> void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>>> {
>>> 	struct request *req = cmd->request;
>>>
>>> 	rq->resid = scsi_out(cmd)->resid;
>>> 	rq->next_rq->resid = scsi_in(cmd)->resid;
>>>
>>> 	if (blk_end_request(req, 1, req->data_len))
>>> 		BUG();
>>>
>>> 	scsi_release_buffers(cmd);
>>> 	scsi_next_command(cmd);
>>> }
> ...
> snip
> ...
>> rq->data_len = scsi_out(cmd)->resid is Not Just a problem of bidi
>> it is a General problem of scsi residual handling, and user code.
>>
>> Even today before any bidi. at scsi_lib.c at scsi_io_completion()
>> we do req->data_len = scsi_get_resid(cmd);
>> ( or: req->data_len = cmd->resid; depends which version you look)
>> And then call scsi_end_request() which calls __end_that_request_first/last
>> So it is assumed even today that req->data_len is not touched by
>> __end_that_request_first/last unless __end_that_request_first returned
>> that there is more work to do and the command is resubmitted in which
>> case the resid information is discarded.
>>
>> So if the regular resid handling is acceptable - Set req->data_len
>> before the call to __end_that_request_first/last, or blk_end_request()
>> in your case, then here goes your second client of the _callback and
>> it can be removed.
>> But if it is found that req->data_len is touched and the resid information
>> gets lost, than it should be fixed for the common uni-io case, by - for example
>> - pass resid to the blk_end_request() function.
>> (So in any way the _callback can go)
> 
> Thank you for the explanation of scsi's rq->data_len usage.
> I see that scsi usually uses rq->data_len for cmd->resid.
> 
> I have investigated the possibility of setting data_len before
> the call to blk_end_request.
> But no matter whether data_len is touched or not, we need a callback
> for bidi.  So I would like to go with the current patch.
> 
> I explained the reason and some details below.
> 
> 
> As far as I can see, rq->data_len is just referenced
> by blk_add_trace_rq() in __end_that_request_first(), not modified.
> And I don't change any logic around there in the block-layer.
> So there shouldn't be any critical problem for scsi residual handing.
> (although I'm not sure that scsi expectes cmd->resid to be traced
>  by blk_trace.)
> 
> Anyway, I see that it is no critical problem for bidi to set cmd->resid
> to rq->data_len before blk_end_request() call.
> But if I do that, blk_end_request() can't get the next_rq's size
> to complete in its code below.
> 
>> +		/* Bidi request must be completed as a whole */
>> +		if (blk_bidi_rq(rq) &&
>> +		    __end_that_request_first(rq->next_rq, uptodate,
>> +					     blk_rq_bytes(rq->next_rq)))
>> +			return 1;
> 
> So I will have to move next_rq completion to bidi and use _callback()
> anyway like the following.
> ---------------------------------------------------------------------
> static int dummy_cb(struct request *rq)
> {
> 	return 1;
> }
> 
> 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;
>  
> 	/* Complete only DATA of next_rq using _callback and dummy function */
> 	if (!blk_end_request_callback(req->next_rq, 1, next_dlen, dummy_cb))
> 		BUG();
>  
> 	if (blk_end_request(req, 1, dlen))
> 		BUG();
> 
> 	scsi_release_buffers(cmd);
> 	scsi_next_command(cmd);
> }
> ---------------------------------------------------------------------
> 
> I prefer the current patch rather than the code like above,
> since the code calls blk_end_request twice and looks trickier.
> So I'd like to leave the patch but descriptions and comments in it.
> Below is the patch, which updated only descriptions and comments.
> If you still have some concerns on this, please let me know.
> 
> 
> 
> Subject: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)
> 
> This patch converts bidi of scsi mid-layer to use blk_end_request
> interfaces.
> 
> rq->next_rq represents a pair of bidi requests.
> (There are no other use of 'next_rq' of struct request.)
> For both requests in the pair, end_that_request_chunk() should be
> called before end_that_request_last() is called for one of them.
> Since the calls to end_that_request_first()/chunk() and
> end_that_request_last() are packaged into blk_end_request(),
> the handling of next_rq completion has to be moved into
> blk_end_request(), too.
> 
> Since blk_end_request() family don't have any argument for
> the completion size of next_rq, they use next_rq->data_len for it.
> So bidi has to set the resid after blk_end_request() completes
> the data of next_rq.
> To satisfy the requirement, bidi uses blk_end_request_callback(),
> which is added in PATCH 25 only for the tricky drivers.
> 
> Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> ---
>  block/ll_rw_blk.c       |   18 +++++++++++++
>  drivers/scsi/scsi_lib.c |   62 +++++++++++++++++++++++-------------------------
>  2 files changed, 48 insertions(+), 32 deletions(-)
> 
> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
>  		scsi_run_queue(sdev->request_queue);
>  }
>  
> -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> -{
> -	struct request_queue *q = cmd->device->request_queue;
> -	struct request *req = cmd->request;
> -	unsigned long flags;
> -
> -	add_disk_randomness(req->rq_disk);
> -
> -	spin_lock_irqsave(q->queue_lock, flags);
> -	if (blk_rq_tagged(req))
> -		blk_queue_end_tag(q, req);
> -
> -	end_that_request_last(req, uptodate);
> -	spin_unlock_irqrestore(q->queue_lock, flags);
> -
> -	/*
> -	 * This will goose the queue request function at the end, so we don't
> -	 * need to worry about launching another command.
> -	 */
> -	scsi_next_command(cmd);
> -}
> -
>  /*
>   * Function:    scsi_end_request()
>   *
> @@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
>  EXPORT_SYMBOL(scsi_release_buffers);
>  
>  /*
> + * Called from blk_end_request_callback() after all DATA in rq and its next_rq
> + * are completed before rq is completed/freed.
> + */
> +static int scsi_end_bidi_request_cb(struct request *rq)
> +{
> +	struct scsi_cmnd *cmd = rq->special;
> +
> +	rq->data_len = scsi_out(cmd)->resid;
> +	rq->next_rq->data_len = scsi_in(cmd)->resid;
> +
> +	return 0;
> +}
> +
> +/*
>   * 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
> @@ -931,22 +923,28 @@ void scsi_end_bidi_request(struct scsi_c
>  {
>  	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);
> -
>  	/*
>  	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> -	 *       in end_that_request_last() then this WARN_ON must be removed.
> +	 *       in blk_end_request() then this WARN_ON must be removed.
>  	 *       for now, upper-driver must have registered an end_io.
>  	 */
>  	WARN_ON(!req->end_io);
>  
> -	scsi_finalize_request(cmd, 1);
> +	/*
> +	 * blk_end_request() family take care of data completion of next_rq.
> +	 * blk_end_request() family use next_rq->data_len for 
> +	 * the completion data size of next_rq.
> +	 * So resid can't be set before the data completion of next_rq
> +	 * in blk_end_request().
> +	 * To resolve that, use the callback feature of blk_end_request().
> +	 */
> +	if (blk_end_request_callback(req, 1, req->data_len,
> +				     scsi_end_bidi_request_cb))
> +		/* req has not been completed */
> +		BUG();
> +
> +	scsi_release_buffers(cmd);
> +	scsi_next_command(cmd);
>  }
>  
>  /*
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3817,6 +3817,12 @@ int blk_end_request(struct request *rq, 
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	add_disk_randomness(rq->rq_disk);
> @@ -3840,6 +3846,12 @@ int __blk_end_request(struct request *rq
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	add_disk_randomness(rq->rq_disk);
> @@ -3884,6 +3896,12 @@ int blk_end_request_callback(struct requ
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate,
> +					     blk_rq_bytes(rq->next_rq)))
> +			return 1;
>  	}
>  
>  	/* Special feature for tricky drivers */
> 
> Thanks,
> Kiyoshi Ueda
No I don't like it. The only client left for blk_end_request_callback()
is bidi, and for bidi we can do a much simpler thing. listed below 
are some possible solutions.

1. Take extra parm to blk_end_request like
int blk_end_request(struct request *rq, int error, int nr_bytes, int bidi_bytes)

if the bidi_bytes is not zero than req->next_req is freed like above.

2. My original suggestion of keeping end_that_request_first() exported Just for the
bidi case. Or rename it to:
int blk_end_bidi_req(struct request *rq, int nr_bytes)
{
	return __end_that_request_first(rq->next_rq, 0, /* 0 is the new API for uptodate */
					     nr_bytes);
}

Or some other variation of the 2 above ...
------

And please reconsider that double implementation. (triple above, but I hope
now I have convinced you to drop one). Jense please ?

Boaz

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Kiyoshi Ueda]

Hi Boaz, Jens,

On Thu, 06 Dec 2007 11:24:44 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> > Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> > ===================================================================
> > --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> > +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> > @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
> >  		scsi_run_queue(sdev->request_queue);
> >  }
> >  
> > -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> > -{
> > -	struct request_queue *q = cmd->device->request_queue;
> > -	struct request *req = cmd->request;
> > -	unsigned long flags;
> > -
> > -	add_disk_randomness(req->rq_disk);
> > -
> > -	spin_lock_irqsave(q->queue_lock, flags);
> > -	if (blk_rq_tagged(req))
> > -		blk_queue_end_tag(q, req);
> > -
> > -	end_that_request_last(req, uptodate);
> > -	spin_unlock_irqrestore(q->queue_lock, flags);
> > -
> > -	/*
> > -	 * This will goose the queue request function at the end, so we don't
> > -	 * need to worry about launching another command.
> > -	 */
> > -	scsi_next_command(cmd);
> > -}
> > -
> >  /*
> >   * Function:    scsi_end_request()
> >   *
> > @@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
> >  EXPORT_SYMBOL(scsi_release_buffers);
> >  
> >  /*
> > + * Called from blk_end_request_callback() after all DATA in rq and its next_rq
> > + * are completed before rq is completed/freed.
> > + */
> > +static int scsi_end_bidi_request_cb(struct request *rq)
> > +{
> > +	struct scsi_cmnd *cmd = rq->special;
> > +
> > +	rq->data_len = scsi_out(cmd)->resid;
> > +	rq->next_rq->data_len = scsi_in(cmd)->resid;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> >   * 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
> > @@ -931,22 +923,28 @@ void scsi_end_bidi_request(struct scsi_c
> >  {
> >  	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);
> > -
> >  	/*
> >  	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> > -	 *       in end_that_request_last() then this WARN_ON must be removed.
> > +	 *       in blk_end_request() then this WARN_ON must be removed.
> >  	 *       for now, upper-driver must have registered an end_io.
> >  	 */
> >  	WARN_ON(!req->end_io);
> >  
> > -	scsi_finalize_request(cmd, 1);
> > +	/*
> > +	 * blk_end_request() family take care of data completion of next_rq.
> > +	 * blk_end_request() family use next_rq->data_len for 
> > +	 * the completion data size of next_rq.
> > +	 * So resid can't be set before the data completion of next_rq
> > +	 * in blk_end_request().
> > +	 * To resolve that, use the callback feature of blk_end_request().
> > +	 */
> > +	if (blk_end_request_callback(req, 1, req->data_len,
> > +				     scsi_end_bidi_request_cb))
> > +		/* req has not been completed */
> > +		BUG();
> > +
> > +	scsi_release_buffers(cmd);
> > +	scsi_next_command(cmd);
> >  }
> >  
> >  /*
> > Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> > ===================================================================
> > --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> > +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> > @@ -3817,6 +3817,12 @@ int blk_end_request(struct request *rq, 
> >  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> >  		if (__end_that_request_first(rq, uptodate, nr_bytes))
> >  			return 1;
> > +
> > +		/* Bidi request must be completed as a whole */
> > +		if (blk_bidi_rq(rq) &&
> > +		    __end_that_request_first(rq->next_rq, uptodate,
> > +					     blk_rq_bytes(rq->next_rq)))
> > +			return 1;
> >  	}
> >  
> >  	add_disk_randomness(rq->rq_disk);
> > @@ -3840,6 +3846,12 @@ int __blk_end_request(struct request *rq
> >  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> >  		if (__end_that_request_first(rq, uptodate, nr_bytes))
> >  			return 1;
> > +
> > +		/* Bidi request must be completed as a whole */
> > +		if (blk_bidi_rq(rq) &&
> > +		    __end_that_request_first(rq->next_rq, uptodate,
> > +					     blk_rq_bytes(rq->next_rq)))
> > +			return 1;
> >  	}
> >  
> >  	add_disk_randomness(rq->rq_disk);
> > @@ -3884,6 +3896,12 @@ int blk_end_request_callback(struct requ
> >  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> >  		if (__end_that_request_first(rq, uptodate, nr_bytes))
> >  			return 1;
> > +
> > +		/* Bidi request must be completed as a whole */
> > +		if (blk_bidi_rq(rq) &&
> > +		    __end_that_request_first(rq->next_rq, uptodate,
> > +					     blk_rq_bytes(rq->next_rq)))
> > +			return 1;
> >  	}
> >  
> >  	/* Special feature for tricky drivers */
>
> No I don't like it. The only client left for blk_end_request_callback()
> is bidi,

ide-cd (cdrom_newpc_intr) is another client.
So I can't drop blk_end_request_callback() even if bidi doesn't use it.

> and for bidi we can do a much simpler thing. listed below 
> are some possible solutions.
> 
> 1. Take extra parm to blk_end_request like
> int blk_end_request(struct request *rq, int error, int nr_bytes, int bidi_bytes)
> 
> if the bidi_bytes is not zero than req->next_req is freed like above.

OK, I agree with the interface basically.
I didn't like to have the 'bidi_bytes', since I considered it was
a driver specific argument and I prefer to avoid driver specific
interface like blk_end_request_callback() as much as possible.
However, it can be considered as block-layer generic, since next_rq is
a member of struct request (and other drivers might have bidi support
although scsi is only one user right now).

So I made a tentative patch below, which adds a variant,
blk_end_bidi_request().
Please take a look.


> And please reconsider that double implementation. (triple above, but I hope
> now I have convinced you to drop one). Jense please ?

I have a plan to remove that duplication using the internal-with-flags
implementation something like the patch (blk_end_io) below.

Jens,
If you agree with passing a 'needlock' flag, which is used to decide
whether to get queue's lock, I can remove the last duplication of
__blk_end_request() too.  Do you still disagree?

Thanks,
Kiyoshi Ueda


Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
===================================================================
--- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
+++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
@@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
 		scsi_run_queue(sdev->request_queue);
 }
 
-static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-	unsigned long flags;
-
-	add_disk_randomness(req->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	if (blk_rq_tagged(req))
-		blk_queue_end_tag(q, req);
-
-	end_that_request_last(req, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_next_command(cmd);
-}
-
 /*
  * Function:    scsi_end_request()
  *
@@ -930,23 +908,25 @@ EXPORT_SYMBOL(scsi_release_buffers);
 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;
 
-	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);
-
 	/*
 	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
-	 *       in end_that_request_last() then this WARN_ON must be removed.
+	 *       in blk_end_bidi_request() then this WARN_ON must be removed.
 	 *       for now, upper-driver must have registered an end_io.
 	 */
 	WARN_ON(!req->end_io);
 
-	scsi_finalize_request(cmd, 1);
+	if (blk_end_bidi_request(req, 1, dlen, next_dlen))
+		/* req has not been completed */
+		BUG();
+
+	scsi_release_buffers(cmd);
+	scsi_next_command(cmd);
 }
 
 /*
Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
===================================================================
--- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
+++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
@@ -3792,24 +3792,17 @@ static void complete_request(struct requ
 	if (!list_empty(&rq->queuelist))
 		blkdev_dequeue_request(rq);
 
+	if (blk_bidi_rq(rq) && !rq->end_io)
+		__blk_put_request(rq->next_rq->q, rq->next_rq);
+
 	end_that_request_last(rq, uptodate);
 }
 
-/**
- * blk_end_request - Helper function for drivers to complete the request.
- * @rq:       the request being processed
- * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
- * @nr_bytes: number of bytes to complete
- *
- * Description:
- *     Ends I/O on a number of bytes attached to @rq.
- *     If @rq has leftover, sets it up for the next range of segments.
- *
- * Return:
- *     0 - we are done with this request
- *     1 - still buffers pending for this request
- **/
-int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
+/*
+ * Internal function
+ */
+static int blk_end_io(struct request *rq, int uptodate, int nr_bytes,
+		      int bidi_bytes, int (drv_callback)(struct request *))
 {
 	struct request_queue *q = rq->q;
 	unsigned long flags = 0UL;
@@ -3817,8 +3810,17 @@ int blk_end_request(struct request *rq, 
 	if (blk_fs_request(rq) || blk_pc_request(rq)) {
 		if (__end_that_request_first(rq, uptodate, nr_bytes))
 			return 1;
+
+		/* Bidi request must be completed as a whole */
+		if (blk_bidi_rq(rq) &&
+		    __end_that_request_first(rq->next_rq, uptodate, bidi_bytes))
+			return 1;
 	}
 
+	/* Special feature for tricky drivers */
+	if (drv_callback && drv_callback(rq))
+		return 1;
+
 	add_disk_randomness(rq->rq_disk);
 
 	spin_lock_irqsave(q->queue_lock, flags);
@@ -3827,6 +3829,32 @@ int blk_end_request(struct request *rq, 
 
 	return 0;
 }
+
+int blk_end_bidi_request(struct request *rq, int uptodate, int nr_bytes,
+			 int bidi_bytes)
+{
+	return blk_end_io(rq, uptodate, nr_bytes, bidi_bytes, NULL);
+}
+EXPORT_SYMBOL_GPL(blk_end_bidi_request);
+
+/**
+ * blk_end_request - Helper function for drivers to complete the request.
+ * @rq:       the request being processed
+ * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
+ * @nr_bytes: number of bytes to complete
+ *
+ * Description:
+ *     Ends I/O on a number of bytes attached to @rq.
+ *     If @rq has leftover, sets it up for the next range of segments.
+ *
+ * Return:
+ *     0 - we are done with this request
+ *     1 - still buffers pending for this request
+ **/
+int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
+{
+	return blk_end_io(rq, uptodate, nr_bytes, 0, NULL);
+}
 EXPORT_SYMBOL_GPL(blk_end_request);
 
 /**
@@ -3878,25 +3906,7 @@ EXPORT_SYMBOL_GPL(__blk_end_request);
 int blk_end_request_callback(struct request *rq, int uptodate, int nr_bytes,
 			     int (drv_callback)(struct request *))
 {
-	struct request_queue *q = rq->q;
-	unsigned long flags = 0UL;
-
-	if (blk_fs_request(rq) || blk_pc_request(rq)) {
-		if (__end_that_request_first(rq, uptodate, nr_bytes))
-			return 1;
-	}
-
-	/* Special feature for tricky drivers */
-	if (drv_callback && drv_callback(rq))
-		return 1;
-
-	add_disk_randomness(rq->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	complete_request(rq, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	return 0;
+	return blk_end_io(rq, uptodate, nr_bytes, 0, drv_callback);
 }
 EXPORT_SYMBOL_GPL(blk_end_request_callback);
 

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Boaz Harrosh]

On Thu, Dec 06 2007 at 21:44 +0200, Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> Hi Boaz, Jens,
> 
> On Thu, 06 Dec 2007 11:24:44 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
>>> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
<snip>
>> No I don't like it. The only client left for blk_end_request_callback()
>> is bidi,
> 
> ide-cd (cdrom_newpc_intr) is another client.
> So I can't drop blk_end_request_callback() even if bidi doesn't use it.
> 
It looks like all is needed for the ide-cd case is a flag that says
"don't complete the request". And the call-back is not actually used.
(Inspecting the last: [PATCH 26/28] blk_end_request: changing ide-cd (take 3))
The same exact flag could also help the bidi case. Perhaps have an API
for that?

<snip>
> 
> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
>  		scsi_run_queue(sdev->request_queue);
>  }
>  
> -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> -{
> -	struct request_queue *q = cmd->device->request_queue;
> -	struct request *req = cmd->request;
> -	unsigned long flags;
> -
> -	add_disk_randomness(req->rq_disk);
> -
> -	spin_lock_irqsave(q->queue_lock, flags);
> -	if (blk_rq_tagged(req))
> -		blk_queue_end_tag(q, req);
> -
> -	end_that_request_last(req, uptodate);
> -	spin_unlock_irqrestore(q->queue_lock, flags);
> -
> -	/*
> -	 * This will goose the queue request function at the end, so we don't
> -	 * need to worry about launching another command.
> -	 */
> -	scsi_next_command(cmd);
> -}
> -
>  /*
>   * Function:    scsi_end_request()
>   *
> @@ -930,23 +908,25 @@ EXPORT_SYMBOL(scsi_release_buffers);
>  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;
>  
> -	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);
> -
>  	/*
>  	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> -	 *       in end_that_request_last() then this WARN_ON must be removed.
> +	 *       in blk_end_bidi_request() then this WARN_ON must be removed.
>  	 *       for now, upper-driver must have registered an end_io.
>  	 */
>  	WARN_ON(!req->end_io);
>  
> -	scsi_finalize_request(cmd, 1);
> +	if (blk_end_bidi_request(req, 1, dlen, next_dlen))
> +		/* req has not been completed */
> +		BUG();
> +
> +	scsi_release_buffers(cmd);
> +	scsi_next_command(cmd);
>  }
>  
>  /*
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3792,24 +3792,17 @@ static void complete_request(struct requ
>  	if (!list_empty(&rq->queuelist))
>  		blkdev_dequeue_request(rq);
>  
> +	if (blk_bidi_rq(rq) && !rq->end_io)
> +		__blk_put_request(rq->next_rq->q, rq->next_rq);
> +
>  	end_that_request_last(rq, uptodate);
>  }
>  
> -/**
> - * blk_end_request - Helper function for drivers to complete the request.
> - * @rq:       the request being processed
> - * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> - * @nr_bytes: number of bytes to complete
> - *
> - * Description:
> - *     Ends I/O on a number of bytes attached to @rq.
> - *     If @rq has leftover, sets it up for the next range of segments.
> - *
> - * Return:
> - *     0 - we are done with this request
> - *     1 - still buffers pending for this request
> - **/
> -int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> +/*
> + * Internal function
> + */
> +static int blk_end_io(struct request *rq, int uptodate, int nr_bytes,
> +		      int bidi_bytes, int (drv_callback)(struct request *))
>  {
>  	struct request_queue *q = rq->q;
>  	unsigned long flags = 0UL;
> @@ -3817,8 +3810,17 @@ int blk_end_request(struct request *rq, 
>  	if (blk_fs_request(rq) || blk_pc_request(rq)) {
>  		if (__end_that_request_first(rq, uptodate, nr_bytes))
>  			return 1;
> +
> +		/* Bidi request must be completed as a whole */
> +		if (blk_bidi_rq(rq) &&
> +		    __end_that_request_first(rq->next_rq, uptodate, bidi_bytes))
> +			return 1;
>  	}
>  
> +	/* Special feature for tricky drivers */
> +	if (drv_callback && drv_callback(rq))
> +		return 1;
> +
>  	add_disk_randomness(rq->rq_disk);
>  
>  	spin_lock_irqsave(q->queue_lock, flags);
> @@ -3827,6 +3829,32 @@ int blk_end_request(struct request *rq, 
>  
>  	return 0;
>  }
> +
> +int blk_end_bidi_request(struct request *rq, int uptodate, int nr_bytes,
> +			 int bidi_bytes)
> +{
> +	return blk_end_io(rq, uptodate, nr_bytes, bidi_bytes, NULL);
> +}
> +EXPORT_SYMBOL_GPL(blk_end_bidi_request);
> +
> +/**
> + * blk_end_request - Helper function for drivers to complete the request.
> + * @rq:       the request being processed
> + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> + * @nr_bytes: number of bytes to complete
> + *
> + * Description:
> + *     Ends I/O on a number of bytes attached to @rq.
> + *     If @rq has leftover, sets it up for the next range of segments.
> + *
> + * Return:
> + *     0 - we are done with this request
> + *     1 - still buffers pending for this request
> + **/
> +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> +{
> +	return blk_end_io(rq, uptodate, nr_bytes, 0, NULL);
> +}
>  EXPORT_SYMBOL_GPL(blk_end_request);
>  

All above looks fine, thanks.

>  /**
> @@ -3878,25 +3906,7 @@ EXPORT_SYMBOL_GPL(__blk_end_request);
>  int blk_end_request_callback(struct request *rq, int uptodate, int nr_bytes,
>  			     int (drv_callback)(struct request *))
>  {
> -	struct request_queue *q = rq->q;
> -	unsigned long flags = 0UL;
> -
> -	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> -		if (__end_that_request_first(rq, uptodate, nr_bytes))
> -			return 1;
> -	}
> -
> -	/* Special feature for tricky drivers */
> -	if (drv_callback && drv_callback(rq))
> -		return 1;
> -
> -	add_disk_randomness(rq->rq_disk);
> -
> -	spin_lock_irqsave(q->queue_lock, flags);
> -	complete_request(rq, uptodate);
> -	spin_unlock_irqrestore(q->queue_lock, flags);
> -
> -	return 0;
> +	return blk_end_io(rq, uptodate, nr_bytes, 0, drv_callback);
>  }
>  EXPORT_SYMBOL_GPL(blk_end_request_callback);
>  

Boaz

Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)

[Kiyoshi Ueda]

Hi Boaz,

On Sun, 09 Dec 2007 11:43:31 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> >>> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> <snip>
> >> No I don't like it. The only client left for blk_end_request_callback()
> >> is bidi,
> > 
> > ide-cd (cdrom_newpc_intr) is another client.
> > So I can't drop blk_end_request_callback() even if bidi doesn't use it.
>
> It looks like all is needed for the ide-cd case is a flag that says
> "don't complete the request". And the call-back is not actually used.
> (Inspecting the last: [PATCH 26/28] blk_end_request: changing ide-cd (take 3))
> The same exact flag could also help the bidi case. Perhaps have an API
> for that?

Thank you for looking at the ide-cd part, too.

But, no, I don't want to add the "don't complete" flag to the API.
It could be an alternative, but having such a flag explicitly may blur
the purpose of the blk_end_reuqest interfaces, which drivers give over
the ownership of the request to the block-layer when calling
blk_end_request interfaces.
(With such a flag, the API looks like explicitly allowing drivers
 to have the ownership of the request even after the API is called,
 and it is like end_that_request_chunk().)
And the API also looks easy to use for drivers and it might help
to make other tricky drivers in the future.

I would like to go with the callback API, since the usege in ide-cd
shows that the driver is very tricky and the API can be used for
other requirements like "a driver wants to do something after data
completion and before request completion."


> > Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> > ===================================================================
> > --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> > +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> > @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
> >  		scsi_run_queue(sdev->request_queue);
> >  }
> >  
snip
> > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> > +{
> > +	return blk_end_io(rq, uptodate, nr_bytes, 0, NULL);
> > +}
> >  EXPORT_SYMBOL_GPL(blk_end_request);
> >  
> 
> All above looks fine, thanks.

OK, I'll update the patch-set using the bidi API and blk_end_io().
I'm currently updating the patch-set based on 2.6.24-rc4, not -mm.
So the new patch-set will include the blk_end_bidi_request() API
but not the scsi bidi changes.

Thanks,
Kiyoshi Ueda