Re: [PATCH 2/3] scsi_data_buffer

[Jens Axboe <jens.axboe@oracle.com>]

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@panasas.com> 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.

-- 
Jens Axboe