From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753879AbXLDOQl (ORCPT ); Tue, 4 Dec 2007 09:16:41 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752049AbXLDOQb (ORCPT ); Tue, 4 Dec 2007 09:16:31 -0500 Received: from bzq-219-195-70.pop.bezeqint.net ([62.219.195.70]:47503 "EHLO bh-buildlin2.bhalevy.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753262AbXLDOQa (ORCPT ); Tue, 4 Dec 2007 09:16:30 -0500 Message-ID: <47555880.9070902@panasas.com> Date: Tue, 04 Dec 2007 15:39:12 +0200 From: Boaz Harrosh User-Agent: Thunderbird 2.0.0.6 (X11/20070728) MIME-Version: 1.0 To: Kiyoshi Ueda CC: jens.axboe@oracle.com, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org, dm-devel@redhat.com, j-nomura@ce.jp.nec.com Subject: Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3) References: <20071130.183512.120442861.k-ueda@ct.jp.nec.com> In-Reply-To: <20071130.183512.120442861.k-ueda@ct.jp.nec.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda 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 > Signed-off-by: Jun'ichi Nomura > --- > 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