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 ...
I have a better solution for this. At attachment time. sr will modify the
request_queue's max_hw_sectors to not max over 0xffff * s_size, this way
the block layer will split it's I/O, and no extra resid handling is needed.
If the later is accepted than where this blk_set_max_hw_sectors() be?
on the first request, like block-size. Or at sr_open()/sr_block_open()
Please comment and I'll scribble a patch.
> SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 178e8c2..2d9a32b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
<snip>
Thanks
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
