From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boaz Harrosh Subject: Re: [PATCH 2/3] scsi_data_buffer Date: Thu, 08 Nov 2007 16:17:35 +0200 Message-ID: <47331A7F.4010301@panasas.com> References: <4730ACAA.2060007@panasas.com> <4730B034.6050309@panasas.com> <473312AE.3000605@panasas.com> <20071108135437.GP5011@kernel.dk> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from sa10.bezeqint.net ([192.115.104.24]:32976 "EHLO sa10.bezeqint.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754633AbXKHORn (ORCPT ); Thu, 8 Nov 2007 09:17:43 -0500 In-Reply-To: <20071108135437.GP5011@kernel.dk> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Jens Axboe Cc: James Bottomley , linux-scsi , FUJITA Tomonori , Mike Christie , Pete Wyckoff , Benny Halevy On Thu, Nov 08 2007 at 15:54 +0200, Jens Axboe wrote: > On Thu, Nov 08 2007, Boaz Harrosh wrote: >> James, Jens please note the question below >> It is something that bothers me about sr.c >> >> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh wrote: >>> In preparation for bidi we abstract all IO members of scsi_cmnd, >>> that will need to duplicate, into a substructure. >>> >> >>> - sd.c and sr.c >>> * sd and sr would adjust IO size to align on device's block >>> size so code needs to change once we move to scsi_data_buff >>> implementation. >>> * Convert code to use scsi_for_each_sg >>> * Use data accessors where appropriate. >>> * Remove dead code (req_data_dir() != READ && != WRITE) >>> >> >>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c >>> index 7702681..6d3bf41 100644 >>> --- a/drivers/scsi/sr.c >>> +++ b/drivers/scsi/sr.c >>> @@ -226,7 +226,7 @@ out: >>> static int sr_done(struct scsi_cmnd *SCpnt) >>> { >>> int result = SCpnt->result; >>> - int this_count = SCpnt->request_bufflen; >>> + int this_count = scsi_bufflen(SCpnt); >>> int good_bytes = (result == 0 ? this_count : 0); >>> int block_sectors = 0; >>> long error_sector; >>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) >>> } else if (rq_data_dir(rq) == READ) { >>> SCpnt->cmnd[0] = READ_10; >>> SCpnt->sc_data_direction = DMA_FROM_DEVICE; >>> - } else { >>> - blk_dump_rq_flags(rq, "Unknown sr command"); >>> - goto out; >>> } >>> >>> { >>> - struct scatterlist *sg = SCpnt->request_buffer; >>> - int i, size = 0; >>> - for (i = 0; i < SCpnt->use_sg; i++) >>> - size += sg[i].length; >>> + struct scatterlist *sg; >>> + int i, size = 0, sg_count = scsi_sg_count(SCpnt); >>> + >>> + scsi_for_each_sg (SCpnt, sg, sg_count, i) >>> + size += sg->length; >>> >>> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) { >>> + if (size != scsi_bufflen(SCpnt)) { >>> scmd_printk(KERN_ERR, SCpnt, >>> "mismatch count %d, bytes %d\n", >>> - size, SCpnt->request_bufflen); >>> - if (SCpnt->request_bufflen > size) >>> - SCpnt->request_bufflen = size; >>> + size, scsi_bufflen(SCpnt)); >>> + if (scsi_bufflen(SCpnt) > size) >>> + SCpnt->sdb.length = size; >>> } >>> } >>> >>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) >>> * request doesn't start on hw block boundary, add scatter pads >>> */ >>> if (((unsigned int)rq->sector % (s_size >> 9)) || >>> - (SCpnt->request_bufflen % s_size)) { >>> + (scsi_bufflen(SCpnt) % s_size)) { >>> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n"); >>> goto out; >>> } >> Here we check I/O is "large-block" aligned. Both start and size >> >>> >>> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9); >>> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9); >>> >> Number of "large-blocks" >> >>> >>> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n", >>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) >>> >>> if (this_count > 0xffff) { >>> this_count = 0xffff; >>> - SCpnt->request_bufflen = this_count * s_size; >>> + SCpnt->sdb.length = this_count * s_size; >>> } >>> >> Here is my problem: >> In the case that the transfer is bigger than 0xffff * s_size >> (512/1024/2048) we modify ->request_bufflen. Now this has two bad >> effects, the way I understand it, please fix me in my >> misunderstanding. >> >> 1. Later in sr_done doing return good_bytes=cmd->request_bufflen will >> only complete the cut-out bytes. Meaning possible BIO leak, since the >> original request_bufflen was lost. (not all bytes are completed) >> >> >> 2. What mechanics will re-send, or even knows, that not the complete >> request was transfered? The way I understand it, a cmnd->resid must be >> set, in this case. Now the normal cmnd->resid is not enough because >> it will be written by drivers, sr needs to stash a resid somewhere and >> add it to cmnd->resid in sr_done(). But ... >> > > It's not lost, the request will be requeued in scsi_end_request(). The > original state is in the request structure, and end_that_request_chunk() > will return not-done when you complete this first part. > OK, I see it now, thanks. Should we adjust the q->max_hw_sectors anyway so elevator can divide the work load more evenly? The way it is now, it's possible that we always do small leftovers at the end. Boaz