On Thu, Nov 08 2007 at 15:54 +0200, Jens Axboe <jens.axboe@xxxxxxxxxx> 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 <bharrosh@xxxxxxxxxxx> wrote:
>>> In preparation for bidi we abstract all IO members of scsi_cmnd,
>>> that will need to duplicate, into a substructure.
>>>
>> <snip>
>>> - 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)
>>>
>> <snip>
>>> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
