Re: regression: CD burning (k3b) went broke

Re: regression: CD burning (k3b) went broke

[Andrew Morton]

On Tue, 26 Feb 2008 14:36:43 +0100 Mike Galbraith <efault@gmx.de> wrote:

> 
> On Tue, 2008-02-26 at 10:48 +0100, Mike Galbraith wrote:
> > Greetings,
> > 
> > I straced both a good and a bad kernel (good being .git with attached
> > revert patch applied) and filtered/diffed/merged the output.  Scroll
> > down to "HERE" to see the problem (resid).
> > 
> > I'm poking around, but not having much luck.

cc's added.

I'm told this is part of "Tejun's DMA drain handling".

> Seems the problem is data_len changes, but raw_data_len doesn't.  I've
> not the foggiest IO-land clue, but k3b works again, so the below may
> have some diagnostic value.

So this change fixes a bug?  Can we have a recap of how it does this?

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ba21d97..7a6f784 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -871,7 +871,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			scsi_end_bidi_request(cmd);
>  			return;
>  		}
> -		req->data_len = scsi_get_resid(cmd);
> +		req->data_len = req->raw_data_len = scsi_get_resid(cmd);
>  	}
>  
>  	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
> 

Thanks.

Re: regression: CD burning (k3b) went broke

[Jeff Garzik]

Andrew Morton wrote:
> On Tue, 26 Feb 2008 14:36:43 +0100 Mike Galbraith <efault@gmx.de> wrote:
> 
>> On Tue, 2008-02-26 at 10:48 +0100, Mike Galbraith wrote:
>>> Greetings,
>>>
>>> I straced both a good and a bad kernel (good being .git with attached
>>> revert patch applied) and filtered/diffed/merged the output.  Scroll
>>> down to "HERE" to see the problem (resid).
>>>
>>> I'm poking around, but not having much luck.
> 
> cc's added.
> 
> I'm told this is part of "Tejun's DMA drain handling".

Correct.


>> Seems the problem is data_len changes, but raw_data_len doesn't.  I've
>> not the foggiest IO-land clue, but k3b works again, so the below may
>> have some diagnostic value.
> 
> So this change fixes a bug?  Can we have a recap of how it does this?
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index ba21d97..7a6f784 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -871,7 +871,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>  			scsi_end_bidi_request(cmd);
>>  			return;
>>  		}
>> -		req->data_len = scsi_get_resid(cmd);
>> +		req->data_len = req->raw_data_len = scsi_get_resid(cmd);
>>  	}

I would love to get an answer as to what data_len (and of course 
raw_data_len) should be set to AFTER the command completes, which is 
what is going on here.

I can see the above being correct -- scsi_get_resid() returns the length 
of the left-over data after the command is processed -- but I am mainly 
curious why setting [raw_]data_len matters after I/O completion.

	Jeff

Re: regression: CD burning (k3b) went broke

[Mike Galbraith]

On Tue, 2008-02-26 at 19:46 -0500, Jeff Garzik wrote:

> I would love to get an answer as to what data_len (and of course 
> raw_data_len) should be set to AFTER the command completes, which is 
> what is going on here.

Yeah, blk_complete_sghdr_rq() used to do hdr->resid = irq->data_len,
which is modified down lower.  How/where that hdr->resid percolates back
up, and turns into a retry/nogo, I don't know.

	-Mike

Re: regression: CD burning (k3b) went broke

[Mike Galbraith]

On Tue, 2008-02-26 at 15:08 -0800, Andrew Morton wrote:
> On Tue, 26 Feb 2008 14:36:43 +0100 Mike Galbraith <efault@gmx.de> wrote:

> > Seems the problem is data_len changes, but raw_data_len doesn't.  I've
> > not the foggiest IO-land clue, but k3b works again, so the below may
> > have some diagnostic value.
> 
> So this change fixes a bug?  Can we have a recap of how it does this?

Yeah, it fixes the problem.  (wrt recap, if I could write it, it would
be a changelog;)

	-Mike

Re: regression: CD burning (k3b) went broke

[Mike Galbraith]

On Wed, 2008-02-27 at 03:24 +0100, Mike Galbraith wrote:
> On Tue, 2008-02-26 at 15:08 -0800, Andrew Morton wrote:
 
> > So this change fixes a bug?  Can we have a recap of how it does this?
> 
> Yeah, it fixes the problem.  (wrt recap, if I could write it, it would
> be a changelog;)

Hm.  After rummaging around some more in both kernel and userland, I
think this patchlet is not only functional, but  (random accident)
technically correct.  What the heck, let's see if it flies...

snippet from userland:
/*
 * Return the residual DMA count for last command.
 * If this count is < 0, then a DMA overrun occured.
 */
EXPORT int
scg_getresid(scgp)
	SCSI	*scgp;
{
	return (scgp->scmd->resid);
}

This function is used all over the place in cdrecord to determine
transfer size.

(patchlet takes wing, and... goes splat?)
 
Fix CD burning regression introduced by
6b00769fe1502b4ad97bb327ef7ac971b208bfb5.  raw_data_len must be updated
to reflect residual data upon IO completion because it is used by
blk_complete_sghdr_rq() to set hdr->resid which eventually becomes
visible to userland.

Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba21d97..7a6f784 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -871,7 +871,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
unsigned int good_bytes)
 			scsi_end_bidi_request(cmd);
 			return;
 		}
-		req->data_len = scsi_get_resid(cmd);
+		req->data_len = req->raw_data_len = scsi_get_resid(cmd);
 	}
 
 	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet
*/


	-Mike

Re: regression: CD burning (k3b) went broke

[Mike Galbraith]

On Wed, 2008-02-27 at 07:00 +0100, Mike Galbraith wrote:

> (patchlet takes wing, and... goes splat?)

Bugger, went splat... forgot preformat for patchlet insert.  <quiltuple
checks>
 
Fix CD burning regression introduced by
6b00769fe1502b4ad97bb327ef7ac971b208bfb5.  raw_data_len must be updated
to reflect residual data upon IO completion because it is used by
blk_complete_sghdr_rq() to set hdr->resid which eventually becomes
visible to userland.

Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba21d97..7a6f784 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -871,7 +871,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			scsi_end_bidi_request(cmd);
 			return;
 		}
-		req->data_len = scsi_get_resid(cmd);
+		req->data_len = req->raw_data_len = scsi_get_resid(cmd);
 	}
 
 	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */

Re: regression: CD burning (k3b) went broke

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

Hello, all.

Sorry about the delay.  Was buried under other stuff.  Mike, thanks a
lot for reporting and analyzing the problem; however, the patch is
slightly incorrect.  rq->data_len is rq->data_len + extra stuff for
alignment and padding, so the correct thing to do is...

  req->raw_data_len -= req->data_len - scsi_get_resid(cmd);
  req->data_len = scsi_get_resid(cmd);

which is ugly and error-prone.  In addition, this isn't the only place
where resid is set.  Other block drivers do this too.  This definitely
should be done in block layer.

With rq->data_len and rq->raw_data_len, it's impossible to translate
resid of rq->data_len to resid of rq->raw_data_len as block layer
doesn't know how much was extra data after rq->data_len is modified.
The attached patch substitutes rq->raw_data_len w/ rq->extra_len and
adds blk_rq_raw_data_len().  Things look cleaner this way and the resid
problem should be solved with this.

Can you please verify the attached patch fixes the problem?

Thanks.

-- 
tejun

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4608 bytes --]

diff --git a/block/blk-core.c b/block/blk-core.c
index 775c851..929ab61 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -127,7 +127,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
+	rq->extra_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 09f7fd0..c67a75f 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -156,6 +155,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
 		bio->bi_size += pad_len;
 		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7506c4f..efb5b4d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -232,6 +232,7 @@ new_segment:
 			    (PAGE_SIZE - 1));
 		nsegs++;
 		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 7f3c095..81b2133 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
+		hdr->din_resid = blk_rq_raw_data_len(rq->next_rq);
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = blk_rq_raw_data_len(rq);
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
 
 	/*
 	 * If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e993cac..32424b3 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = blk_rq_raw_data_len(rq);
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0562b0a..5cab84c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2539,7 +2539,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(blk_rq_raw_data_len(scmd->request),
+		     (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6fe67d1..57e2a9e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;
 	unsigned int sense_len;
 	void *data;
 	void *sense;
@@ -477,6 +477,11 @@ enum {
 
 #define rq_data_dir(rq)		((rq)->cmd_flags & 1)
 
+static inline unsigned int blk_rq_raw_data_len(struct request *rq)
+{
+	return rq->data_len - min(rq->extra_len, rq->data_len);
+}
+
 /*
  * We regard a request as sync, if it's a READ or a SYNC write.
  */

Re: regression: CD burning (k3b) went broke

[Mike Galbraith]

On Thu, 2008-02-28 at 16:43 +0900, Tejun Heo wrote:
> Hello, all.
> 
> Sorry about the delay.  Was buried under other stuff.  Mike, thanks a
> lot for reporting and analyzing the problem; however, the patch is
> slightly incorrect.  rq->data_len is rq->data_len + extra stuff for
> alignment and padding, so the correct thing to do is...
> 
>   req->raw_data_len -= req->data_len - scsi_get_resid(cmd);
>   req->data_len = scsi_get_resid(cmd);

Ah, close but no banana.  (feeds poor wingless patchlet to bit-wolf)

> which is ugly and error-prone.  In addition, this isn't the only place
> where resid is set.  Other block drivers do this too.  This definitely
> should be done in block layer.
> 
> With rq->data_len and rq->raw_data_len, it's impossible to translate
> resid of rq->data_len to resid of rq->raw_data_len as block layer
> doesn't know how much was extra data after rq->data_len is modified.
> The attached patch substitutes rq->raw_data_len w/ rq->extra_len and
> adds blk_rq_raw_data_len().  Things look cleaner this way and the resid
> problem should be solved with this.
> 
> Can you please verify the attached patch fixes the problem?
> 
> Thanks.

Thank you, works fine.

> plain text document attachment (patch)
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 775c851..929ab61 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -127,7 +127,7 @@ void rq_init(struct request_queue *q, struct request *rq)
>  	rq->nr_hw_segments = 0;
>  	rq->ioprio = 0;
>  	rq->special = NULL;
> -	rq->raw_data_len = 0;
> +	rq->extra_len = 0;
>  	rq->buffer = NULL;
>  	rq->tag = -1;
>  	rq->errors = 0;
> @@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
>  	rq->hard_cur_sectors = rq->current_nr_sectors;
>  	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
>  	rq->buffer = bio_data(bio);
> -	rq->raw_data_len = bio->bi_size;
>  	rq->data_len = bio->bi_size;
>  
>  	rq->bio = rq->biotail = bio;
> diff --git a/block/blk-map.c b/block/blk-map.c
> index 09f7fd0..c67a75f 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
>  		rq->biotail->bi_next = bio;
>  		rq->biotail = bio;
>  
> -		rq->raw_data_len += bio->bi_size;
>  		rq->data_len += bio->bi_size;
>  	}
>  	return 0;
> @@ -156,6 +155,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
>  		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
>  		bio->bi_size += pad_len;
>  		rq->data_len += pad_len;
> +		rq->extra_len += pad_len;
>  	}
>  
>  	rq->buffer = rq->data = NULL;
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 7506c4f..efb5b4d 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -232,6 +232,7 @@ new_segment:
>  			    (PAGE_SIZE - 1));
>  		nsegs++;
>  		rq->data_len += q->dma_drain_size;
> +		rq->extra_len += q->dma_drain_size;
>  	}
>  
>  	if (sg)
> diff --git a/block/bsg.c b/block/bsg.c
> index 7f3c095..81b2133 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
>  	}
>  
>  	if (rq->next_rq) {
> -		hdr->dout_resid = rq->raw_data_len;
> -		hdr->din_resid = rq->next_rq->raw_data_len;
> +		hdr->dout_resid = blk_rq_raw_data_len(rq);
> +		hdr->din_resid = blk_rq_raw_data_len(rq->next_rq);
>  		blk_rq_unmap_user(bidi_bio);
>  		blk_put_request(rq->next_rq);
>  	} else if (rq_data_dir(rq) == READ)
> -		hdr->din_resid = rq->raw_data_len;
> +		hdr->din_resid = blk_rq_raw_data_len(rq);
>  	else
> -		hdr->dout_resid = rq->raw_data_len;
> +		hdr->dout_resid = blk_rq_raw_data_len(rq);
>  
>  	/*
>  	 * If the request generated a negative error number, return it
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index e993cac..32424b3 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
>  	hdr->info = 0;
>  	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
>  		hdr->info |= SG_INFO_CHECK;
> -	hdr->resid = rq->raw_data_len;
> +	hdr->resid = blk_rq_raw_data_len(rq);
>  	hdr->sb_len_wr = 0;
>  
>  	if (rq->sense_len && hdr->sbp) {
> @@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
>  	rq = blk_get_request(q, WRITE, __GFP_WAIT);
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->data = NULL;
> -	rq->raw_data_len = 0;
>  	rq->data_len = 0;
> +	rq->extra_len = 0;
>  	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
>  	memset(rq->cmd, 0, sizeof(rq->cmd));
>  	rq->cmd[0] = cmd;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 0562b0a..5cab84c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2539,7 +2539,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>  	 * want to set it properly, and for DMA where it is
>  	 * effectively meaningless.
>  	 */
> -	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
> +	nbytes = min(blk_rq_raw_data_len(scmd->request),
> +		     (unsigned int)63 * 1024);
>  
>  	/* Most ATAPI devices which honor transfer chunk size don't
>  	 * behave according to the spec when odd chunk size which
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 6fe67d1..57e2a9e 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -216,8 +216,8 @@ struct request {
>  	unsigned int cmd_len;
>  	unsigned char cmd[BLK_MAX_CDB];
>  
> -	unsigned int raw_data_len;
>  	unsigned int data_len;
> +	unsigned int extra_len;
>  	unsigned int sense_len;
>  	void *data;
>  	void *sense;
> @@ -477,6 +477,11 @@ enum {
>  
>  #define rq_data_dir(rq)		((rq)->cmd_flags & 1)
>  
> +static inline unsigned int blk_rq_raw_data_len(struct request *rq)
> +{
> +	return rq->data_len - min(rq->extra_len, rq->data_len);
> +}
> +
>  /*
>   * We regard a request as sync, if it's a READ or a SYNC write.
>   */

[PATCH] block: fix residual byte count handling

[Tejun Heo]

rq->raw_data_len introduced for block layer padding and draining
(commit 6b00769fe1502b4ad97bb327ef7ac971b208bfb5) broke residual byte
count handling.  Block drivers modify rq->data_len to notify residual
byte count to the block layer which blindly reported unmodified
rq->raw_data_len to userland.

To keep block drivers dealing only with rq->data_len, this should be
handled inside block layer.  However, how much extra buffer was
appened is lost after rq->data_len is modified.

This patch replaces rq->raw_data_len with rq->extra_len and add
blk_rq_raw_data_len() helper to calculate raw data size from
rq->data_len and rq->extra_len.  The helper returns correct raw
residual byte count when called on a rq whose data_len is modified to
carry residual byte count.

This problem was reported and diagnosed by Mike Galbraith.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
---
 block/blk-core.c          |    3 +--
 block/blk-map.c           |    2 +-
 block/blk-merge.c         |    1 +
 block/bsg.c               |    8 ++++----
 block/scsi_ioctl.c        |    4 ++--
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/blkdev.h    |    8 +++++++-
 7 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 775c851..929ab61 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -127,7 +127,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
+	rq->extra_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 09f7fd0..c67a75f 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -156,6 +155,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
 		bio->bi_size += pad_len;
 		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7506c4f..efb5b4d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -232,6 +232,7 @@ new_segment:
 			    (PAGE_SIZE - 1));
 		nsegs++;
 		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 7f3c095..81b2133 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
+		hdr->din_resid = blk_rq_raw_data_len(rq->next_rq);
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = blk_rq_raw_data_len(rq);
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
 
 	/*
 	 * If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e993cac..32424b3 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = blk_rq_raw_data_len(rq);
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0562b0a..5cab84c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2539,7 +2539,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(blk_rq_raw_data_len(scmd->request),
+		     (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6fe67d1..917b97f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;	/* length of alignment and padding */
 	unsigned int sense_len;
 	void *data;
 	void *sense;
@@ -477,6 +477,12 @@ enum {
 
 #define rq_data_dir(rq)		((rq)->cmd_flags & 1)
 
+/* data_len of the request sans extra stuff for alignment and padding */
+static inline unsigned int blk_rq_raw_data_len(struct request *rq)
+{
+	return rq->data_len - min(rq->extra_len, rq->data_len);
+}
+
 /*
  * We regard a request as sync, if it's a READ or a SYNC write.
  */

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Thu, Feb 28 2008, Tejun Heo wrote:
> rq->raw_data_len introduced for block layer padding and draining
> (commit 6b00769fe1502b4ad97bb327ef7ac971b208bfb5) broke residual byte
> count handling.  Block drivers modify rq->data_len to notify residual
> byte count to the block layer which blindly reported unmodified
> rq->raw_data_len to userland.
> 
> To keep block drivers dealing only with rq->data_len, this should be
> handled inside block layer.  However, how much extra buffer was
> appened is lost after rq->data_len is modified.
> 
> This patch replaces rq->raw_data_len with rq->extra_len and add
> blk_rq_raw_data_len() helper to calculate raw data size from
> rq->data_len and rq->extra_len.  The helper returns correct raw
> residual byte count when called on a rq whose data_len is modified to
> carry residual byte count.
> 
> This problem was reported and diagnosed by Mike Galbraith.

Tejun, this patch isn't much cleaner at all. It really shows the pain of
these two seperate, yet related, variables.

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Jens Axboe wrote:
>> This problem was reported and diagnosed by Mike Galbraith.
> 
> Tejun, this patch isn't much cleaner at all. It really shows the pain of
> these two seperate, yet related, variables.

Not much cleaner compared to what?  I think padding stuff is bound to be
somewhat complex.  It's a nasty thing in nature.  I think ->extra_len is
better than ->raw_data_len because ->extra_len only needs to be updated
where the dirty jobs are done and extra buffer areas are added.  Any
better suggestions?

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[James Bottomley]

On Fri, 2008-02-29 at 00:46 +0900, Tejun Heo wrote:
> Jens Axboe wrote:
> >> This problem was reported and diagnosed by Mike Galbraith.
> > 
> > Tejun, this patch isn't much cleaner at all. It really shows the pain of
> > these two seperate, yet related, variables.
> 
> Not much cleaner compared to what?  I think padding stuff is bound to be
> somewhat complex.  It's a nasty thing in nature.  I think ->extra_len is
> better than ->raw_data_len because ->extra_len only needs to be updated
> where the dirty jobs are done and extra buffer areas are added.  Any
> better suggestions?

Well, I just investigated a bug report in the SCSI transport class.  Our
SMP handler is broken in exactly the same way.  We rely on the incoming
reported request lengths to size our request data, and they've blown up
from the true length to 512 bytes (the size of our alignment).

With the original patch, I have to run through the whole of libsas and
scsi_transport_sas doing

s/data_len/raw_data_len/

With your update it looks like I have to run through them all doing

s/data_len/data_len - extra_len/

which is even worse.  Can't we put things back to a point where data_len
means exactly that and extra_len means how much we have spare on the
end, so you know you can DMA up to data_len + extra_len if need be?

That way we don't have to sweep through every block driver altering the
way it uses data_len.

James

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Fri, Feb 29 2008, James Bottomley wrote:
> 
> On Fri, 2008-02-29 at 00:46 +0900, Tejun Heo wrote:
> > Jens Axboe wrote:
> > >> This problem was reported and diagnosed by Mike Galbraith.
> > > 
> > > Tejun, this patch isn't much cleaner at all. It really shows the pain of
> > > these two seperate, yet related, variables.
> > 
> > Not much cleaner compared to what?  I think padding stuff is bound to be
> > somewhat complex.  It's a nasty thing in nature.  I think ->extra_len is
> > better than ->raw_data_len because ->extra_len only needs to be updated
> > where the dirty jobs are done and extra buffer areas are added.  Any
> > better suggestions?
> 
> Well, I just investigated a bug report in the SCSI transport class.  Our
> SMP handler is broken in exactly the same way.  We rely on the incoming
> reported request lengths to size our request data, and they've blown up
> from the true length to 512 bytes (the size of our alignment).
> 
> With the original patch, I have to run through the whole of libsas and
> scsi_transport_sas doing
> 
> s/data_len/raw_data_len/
> 
> With your update it looks like I have to run through them all doing
> 
> s/data_len/data_len - extra_len/
> 
> which is even worse.  Can't we put things back to a point where data_len
> means exactly that and extra_len means how much we have spare on the
> end, so you know you can DMA up to data_len + extra_len if need be?
> 
> That way we don't have to sweep through every block driver altering the
> way it uses data_len.

Fully agree. The reason why I think it's so ugly is that you have to
keep these two seperate variables in sync. The burning was just one bug,
there will be others...

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Hello, Jens, James.

Jens Axboe wrote:
>> With the original patch, I have to run through the whole of libsas and
>> scsi_transport_sas doing
>>
>> s/data_len/raw_data_len/
>>
>> With your update it looks like I have to run through them all doing
>>
>> s/data_len/data_len - extra_len/

blk_rq_raw_data_len() should do.

>> which is even worse.  Can't we put things back to a point where data_len
>> means exactly that and extra_len means how much we have spare on the
>> end, so you know you can DMA up to data_len + extra_len if need be?
>>
>> That way we don't have to sweep through every block driver altering the
>> way it uses data_len.

If SMP is broken because it needs start address alignment but not
padding to align the size, what should be done is to make that exact
requirement visible to the block layer.  Say,
blk_queue_dma_start_alignment() or maybe change
blk_queue_dma_alignment() such that it only indicates start address
alignment and add blk_queue_dma_size_alignment() for drivers which
require size to be aligned too.  I think those are few.

I think the decision which value rq->data_len represents comes down to
which size is used more in low level drivers because no matter which way
we choose we'll have to update some of the drivers which expects the
other thing from rq->data_len.

blk_rq_raw_data_len() is needed iff a driver needs dummy buffers
attached at the end and still needs to know the original request size
which isn't the common case.

> Fully agree. The reason why I think it's so ugly is that you have to
> keep these two seperate variables in sync. The burning was just one bug,
> there will be others...

The posted modification isn't too bad as the maintenance of the two
variables is at places where the nasty things happen.  I think what
rq->data_len should represent when seen from LLDs is more important and
please note that if SMP is broken because it simply doesn't require
512byte size alignment, it's a different issue.

As long as both raw_data_len and data_len are accessible, I'm okay
either way.  My biggest reluctance is against breaking sum(sg) ==
rq->data_len.  I think this can lead to much more subtle problems such
as programming the controller w/ wrong bytes count and wrapped-around
resid calculation.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[James Bottomley]

On Sat, 2008-03-01 at 15:17 +0900, Tejun Heo wrote:
> Hello, Jens, James.
> 
> Jens Axboe wrote:
> >> With the original patch, I have to run through the whole of libsas and
> >> scsi_transport_sas doing
> >>
> >> s/data_len/raw_data_len/
> >>
> >> With your update it looks like I have to run through them all doing
> >>
> >> s/data_len/data_len - extra_len/
> 
> blk_rq_raw_data_len() should do.

I know we *could* sweep through all the block drivers altering them; my
point is that I don't think we *should*.  Fundamentally, every driver
that cares is assuming req->data_len is the length of the request that
came down.  The fact that it got padded is irrelevant (and actually
detrimental) to most of them as the SMP driver illustrates.

We use a high dma_alignment not because we care about padding, but
because we want to avoid scatter gather.  So we care about alignment of
the start of the buffer (to avoid sg), but fundamentally, we need to
know what its true length (not its padded length) is.  The true length
feeds into the smp frame size and is checked by the interfaces, which is
why the changes caused an SMP failure.

Just for the principle of least surprise, can we not keep req->data_len
what it has always been, namely the true data length of the request and
express the fact that we've padded it by req->extra_len or something, so
we don't have to do all of these driver changes.

> >> which is even worse.  Can't we put things back to a point where data_len
> >> means exactly that and extra_len means how much we have spare on the
> >> end, so you know you can DMA up to data_len + extra_len if need be?
> >>
> >> That way we don't have to sweep through every block driver altering the
> >> way it uses data_len.
> 
> If SMP is broken because it needs start address alignment but not
> padding to align the size, what should be done is to make that exact
> requirement visible to the block layer.  Say,
> blk_queue_dma_start_alignment() or maybe change
> blk_queue_dma_alignment() such that it only indicates start address
> alignment and add blk_queue_dma_size_alignment() for drivers which
> require size to be aligned too.  I think those are few.

But this is true of *every* current user of the block layer apart from
IDE ... we all care about alignment not padding.  Any current user that
actually cares about padding will be doing their own adjustments, so
they need changing anyway.

We can frame that with a different API, but blk_queue_dma_alignment()
better be the common case (start but not pad alignment).

> I think the decision which value rq->data_len represents comes down to
> which size is used more in low level drivers because no matter which way
> we choose we'll have to update some of the drivers which expects the
> other thing from rq->data_len.

Right, and currently, apart from IDE, they all want it to mean the true
data length.

> blk_rq_raw_data_len() is needed iff a driver needs dummy buffers
> attached at the end and still needs to know the original request size
> which isn't the common case.

I think it *is* the common case.

> > Fully agree. The reason why I think it's so ugly is that you have to
> > keep these two seperate variables in sync. The burning was just one bug,
> > there will be others...
> 
> The posted modification isn't too bad as the maintenance of the two
> variables is at places where the nasty things happen.  I think what
> rq->data_len should represent when seen from LLDs is more important and
> please note that if SMP is broken because it simply doesn't require
> 512byte size alignment, it's a different issue.

But we still have to find all the bugs this causes in all the block
drivers ... that's my biggest concern right now.

> As long as both raw_data_len and data_len are accessible, I'm okay
> either way.  My biggest reluctance is against breaking sum(sg) ==
> rq->data_len.  I think this can lead to much more subtle problems such
> as programming the controller w/ wrong bytes count and wrapped-around
> resid calculation.

OK, so can we go back to data_len being the true value and add an
extra_len for drivers who want to know where the padding lies?

James

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Sat, 01 Mar 2008 15:17:32 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Hello, Jens, James.
> 
> Jens Axboe wrote:
> >> With the original patch, I have to run through the whole of libsas and
> >> scsi_transport_sas doing
> >>
> >> s/data_len/raw_data_len/
> >>
> >> With your update it looks like I have to run through them all doing
> >>
> >> s/data_len/data_len - extra_len/
> 
> blk_rq_raw_data_len() should do.
> 
> >> which is even worse.  Can't we put things back to a point where data_len
> >> means exactly that and extra_len means how much we have spare on the
> >> end, so you know you can DMA up to data_len + extra_len if need be?
> >>
> >> That way we don't have to sweep through every block driver altering the
> >> way it uses data_len.
> 
> If SMP is broken because it needs start address alignment but not
> padding to align the size, what should be done is to make that exact
> requirement visible to the block layer.  Say,
> blk_queue_dma_start_alignment() or maybe change
> blk_queue_dma_alignment() such that it only indicates start address
> alignment and add blk_queue_dma_size_alignment() for drivers which
> require size to be aligned too.  I think those are few.
> 
> I think the decision which value rq->data_len represents comes down to
> which size is used more in low level drivers because no matter which way
> we choose we'll have to update some of the drivers which expects the
> other thing from rq->data_len.
> 
> blk_rq_raw_data_len() is needed iff a driver needs dummy buffers
> attached at the end and still needs to know the original request size
> which isn't the common case.
> 
> > Fully agree. The reason why I think it's so ugly is that you have to
> > keep these two seperate variables in sync. The burning was just one bug,
> > there will be others...
> 
> The posted modification isn't too bad as the maintenance of the two
> variables is at places where the nasty things happen.  I think what
> rq->data_len should represent when seen from LLDs is more important and
> please note that if SMP is broken because it simply doesn't require
> 512byte size alignment, it's a different issue.
> 
> As long as both raw_data_len and data_len are accessible, I'm okay
> either way.  My biggest reluctance is against breaking sum(sg) ==
> rq->data_len.  I think this can lead to much more subtle problems such
> as programming the controller w/ wrong bytes count and wrapped-around
> resid calculation.

sum(sg) == rq->data_len is already broken; sg sends such requests
(though it would be nice if it doesn't).

I've not followed the earlier discussion (because I thought the drain
buffer stuff affected only libata but seems it doesn't ...). Why did
we need to change the meaning of rq->data_len?

rq->data_len meant the true data length and the patch to change it
doesn't look to make anything simple. Can we revert the meaning of
rq->data_len? I'm not sure that we need to add rq->extra_len but it's
fine as long as it's only for drivers that want to use it.

This is only compile tested.

=
diff --git a/block/blk-core.c b/block/blk-core.c
index 775c851..bfec406 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -127,7 +127,6 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -135,6 +134,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->cmd_len = 0;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->sense_len = 0;
 	rq->data = NULL;
 	rq->sense = NULL;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 09f7fd0..3287637 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -155,7 +154,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 
 		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
 		bio->bi_size += pad_len;
-		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7506c4f..0f58616 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -231,7 +231,7 @@ new_segment:
 			    ((unsigned long)q->dma_drain_buffer) &
 			    (PAGE_SIZE - 1));
 		nsegs++;
-		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 7f3c095..8917c51 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
+		hdr->din_resid = rq->next_rq->data_len;
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = rq->data_len;
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
 
 	/*
 	 * If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e993cac..a2c3a93 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = rq->data_len;
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7b1f1ee..fe47922 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2538,7 +2538,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	}
 
 	qc->tf.command = ATA_CMD_PACKET;
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* check whether ATAPI DMA is safe */
 	if (!using_pio && ata_check_atapi_dma(qc))
@@ -2549,7 +2549,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(scmd->request->data_len, (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
@@ -2875,7 +2875,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* request result TF and be quiet about device error */
 	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_QUIET;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6fe67d1..b72526c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;	/* length of alignment and padding */
 	unsigned int sense_len;
 	void *data;
 	void *sense;

Re: [PATCH] block: fix residual byte count handling

[Mike Christie]

FUJITA Tomonori wrote:
> sum(sg) == rq->data_len is already broken; sg sends such requests
> (though it would be nice if it doesn't).
> 

Actually, I think I was half wrong on that when you asked about 
scsi_debug. The scatterlist that sg.c uses is never seen by the block 
layer or scsi layer. It is just used as a container to hold segments. 
sg.c and st.c use their scatterlist to manage their preallocated 
pages/segments. When they pass it to scsi_execute_async, that function 
will create a request struct and add bios for the pages.

In 2.6.24 and below, sg.c will send a scatterlist length that does not 
match the IO length, and scsi_execute_async will goof up and send a 
rq->data_len that does not match the sum of the bios. That is what I was 
trying to fix in 2.6.24, but the patch got messed up. In 2.6.25-rc2 and 
above that is fixed and scsi_execute_async will catch sg.c doing this 
and set rq->data_len and the bio lengths correctly.

So hopefully that helps any fixes you might have planned.

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

Is it possible to teach Thunderturd to NOT munge the cc line?  It
stripped names from cc addresses, and here when that happens the message
lands (intentionally) in my spam grinder.  I just happened to see this
one before flushing, but now, thanks to Thunderturd, every follow-up
will also land there.  (well, not any more since I restored it)

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
> sum(sg) == rq->data_len is already broken; sg sends such requests
> (though it would be nice if it doesn't).
> 
> I've not followed the earlier discussion (because I thought the drain
> buffer stuff affected only libata but seems it doesn't ...). Why did
> we need to change the meaning of rq->data_len?

At this point, it's not clear what the original meaning of rq->data_len
is because before moving alignment and padding to block layer,
rq->data_len equaled both the requested data length and the length of sg
list.  AFAIK, it's SCSI midlayer which makes sg list and data length
mismatch not block layer.

>From the POV of the block layer, as now it might extend the sg list, it
has to decide what rq->data_len means in this case - the requested
transfer length from userland or the length of mapped sg list.

I think that currently the biggest problem is that drivers which don't
require request size adjustment are getting it because alignment setting
doesn't distinguish between start address alignment and size alignment.
For drivers which don't require data size adjustment from block layer,
data_len or raw_data_len doesn't matter.  They're equal anyway.  I'm
prepping a patch for this now.

For drivers which do require request size adjustments, I think it's
better to keep rq->data_len in line with the size of mapped sg list.
The rationales are...

- Those are dumb controllers which want to see requests which meet
certain size requirements and they're likely to care more about actual
data buffer size than user requested buffer size.  IOW, they wanna see
sizes which meet certain requirements, so give them those values.

- I think bugs caused by using raw_data_len instead of data_len are more
subtle than the other way around.  Using data_len instead of
raw_data_len usually affects the application layer while using
raw_data_len instead of data_len affects the DMA engine and transport layer.

> rq->data_len meant the true data length and the patch to change it
> doesn't look to make anything simple. Can we revert the meaning of
> rq->data_len? I'm not sure that we need to add rq->extra_len but it's
> fine as long as it's only for drivers that want to use it.
> 
> This is only compile tested.

If we're gonna go this way, we'll need blk_rq_total_data_len() and use
it for drivers which requires request size adjustments.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 11:40:08 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> > sum(sg) == rq->data_len is already broken; sg sends such requests
> > (though it would be nice if it doesn't).
> > 
> > I've not followed the earlier discussion (because I thought the drain
> > buffer stuff affected only libata but seems it doesn't ...). Why did
> > we need to change the meaning of rq->data_len?
> 
> At this point, it's not clear what the original meaning of rq->data_len
> is because before moving alignment and padding to block layer,
> rq->data_len equaled both the requested data length and the length of sg
> list.  AFAIK, it's SCSI midlayer which makes sg list and data length
> mismatch not block layer.
> 
> From the POV of the block layer, as now it might extend the sg list, it
> has to decide what rq->data_len means in this case - the requested
> transfer length from userland or the length of mapped sg list.

Yeah. It meant the requested transfer length from userland in the past
and I think that chaning to the length of mapped sg list doesn't make
anything simpler.


> I think that currently the biggest problem is that drivers which don't
> require request size adjustment are getting it because alignment setting
> doesn't distinguish between start address alignment and size alignment.
> For drivers which don't require data size adjustment from block layer,
> data_len or raw_data_len doesn't matter.  They're equal anyway.  I'm
> prepping a patch for this now.
> 
> For drivers which do require request size adjustments, I think it's
> better to keep rq->data_len in line with the size of mapped sg list.
> The rationales are...
> 
> - Those are dumb controllers which want to see requests which meet
> certain size requirements and they're likely to care more about actual
> data buffer size than user requested buffer size.  IOW, they wanna see
> sizes which meet certain requirements, so give them those values.

The drivers care about the actual data buffer size.

The dumb controllers drivers can get what they want to use
rq->extra_len or walk through the sg list.


> - I think bugs caused by using raw_data_len instead of data_len are more
> subtle than the other way around.  Using data_len instead of
> raw_data_len usually affects the application layer while using
> raw_data_len instead of data_len affects the DMA engine and transport layer.

If we add extra_len, we can get what raw_data_len and data_len
provide.


I can't see what changing the meaning of rq->data_len (and
investigating all the block drivers) gives us.


> > rq->data_len meant the true data length and the patch to change it
> > doesn't look to make anything simple. Can we revert the meaning of
> > rq->data_len? I'm not sure that we need to add rq->extra_len but it's
> > fine as long as it's only for drivers that want to use it.
> > 
> > This is only compile tested.
> 
> If we're gonna go this way, we'll need blk_rq_total_data_len() and use
> it for drivers which requires request size adjustments.

No problem. It would be much better to add blk_rq_total_data_len
rather than chainging the meaning of rq->data_len and all the block
drivers.

Here's an updated patch (I forgot to remove the bi_size adjustment in
blk_rq_map_user in the previous patch). Can we agree on it if we add
blk_rq_total_data_len()?


diff --git a/block/blk-core.c b/block/blk-core.c
index 775c851..bfec406 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -127,7 +127,6 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -135,6 +134,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->cmd_len = 0;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->sense_len = 0;
 	rq->data = NULL;
 	rq->sense = NULL;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 09f7fd0..f559832 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -151,11 +150,8 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 	 */
 	if (len & queue_dma_alignment(q)) {
 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
-		struct bio *bio = rq->biotail;
 
-		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
-		bio->bi_size += pad_len;
-		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7506c4f..0f58616 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -231,7 +231,7 @@ new_segment:
 			    ((unsigned long)q->dma_drain_buffer) &
 			    (PAGE_SIZE - 1));
 		nsegs++;
-		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 7f3c095..8917c51 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
+		hdr->din_resid = rq->next_rq->data_len;
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = rq->data_len;
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
 
 	/*
 	 * If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e993cac..a2c3a93 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = rq->data_len;
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7b1f1ee..fe47922 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2538,7 +2538,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	}
 
 	qc->tf.command = ATA_CMD_PACKET;
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* check whether ATAPI DMA is safe */
 	if (!using_pio && ata_check_atapi_dma(qc))
@@ -2549,7 +2549,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(scmd->request->data_len, (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
@@ -2875,7 +2875,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* request result TF and be quiet about device error */
 	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_QUIET;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6fe67d1..b72526c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;	/* length of alignment and padding */
 	unsigned int sense_len;
 	void *data;
 	void *sense;

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>> - I think bugs caused by using raw_data_len instead of data_len are more
>> subtle than the other way around.  Using data_len instead of
>> raw_data_len usually affects the application layer while using
>> raw_data_len instead of data_len affects the DMA engine and transport layer.
> 
> If we add extra_len, we can get what raw_data_len and data_len
> provide.
> 
> I can't see what changing the meaning of rq->data_len (and
> investigating all the block drivers) gives us.

No matter which way you go, you change the meaning of rq->data_len and
you MUST inspect rq->data_len usage whichever way you go.  Apply your
patch and try to do sg IO on IDE cdrom w/ various transfer lengths.

-- 
tejun

[PATCH 1/2] block: fix residual byte count handling

[Tejun Heo]

rq->raw_data_len introduced for block layer padding and draining
(commit 6b00769fe1502b4ad97bb327ef7ac971b208bfb5) broke residual byte
count handling.  Block drivers modify rq->data_len to notify residual
byte count to the block layer which blindly reported unmodified
rq->raw_data_len to userland.

To keep block drivers dealing only with rq->data_len, this should be
handled inside block layer.  However, how much extra buffer was
appened is lost after rq->data_len is modified.

This patch replaces rq->raw_data_len with rq->extra_len and add
blk_rq_raw_data_len() helper to calculate raw data size from
rq->data_len and rq->extra_len.  The helper returns correct raw
residual byte count when called on a rq whose data_len is modified to
carry residual byte count.

This problem was reported and diagnosed by Mike Galbraith.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
---
Comments updated compared to the previous version.

 block/blk-core.c          |    3 +--
 block/blk-map.c           |    2 +-
 block/blk-merge.c         |    1 +
 block/blk-settings.c      |    4 ++++
 block/bsg.c               |    8 ++++----
 block/scsi_ioctl.c        |    4 ++--
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/blkdev.h    |    8 +++++++-
 8 files changed, 22 insertions(+), 11 deletions(-)

Index: work/block/blk-core.c
===================================================================
--- work.orig/block/blk-core.c
+++ work/block/blk-core.c
@@ -127,7 +127,7 @@ void rq_init(struct request_queue *q, st
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
+	rq->extra_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queu
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
Index: work/block/blk-map.c
===================================================================
--- work.orig/block/blk-map.c
+++ work/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_que
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -156,6 +155,7 @@ int blk_rq_map_user(struct request_queue
 		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
 		bio->bi_size += pad_len;
 		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
Index: work/block/blk-merge.c
===================================================================
--- work.orig/block/blk-merge.c
+++ work/block/blk-merge.c
@@ -232,6 +232,7 @@ new_segment:
 			    (PAGE_SIZE - 1));
 		nsegs++;
 		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
Index: work/block/bsg.c
===================================================================
--- work.orig/block/bsg.c
+++ work/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(stru
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
+		hdr->din_resid = blk_rq_raw_data_len(rq->next_rq);
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = blk_rq_raw_data_len(rq);
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = blk_rq_raw_data_len(rq);
 
 	/*
 	 * If the request generated a negative error number, return it
Index: work/block/scsi_ioctl.c
===================================================================
--- work.orig/block/scsi_ioctl.c
+++ work/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct 
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = blk_rq_raw_data_len(rq);
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct req
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -2549,7 +2549,8 @@ static unsigned int atapi_xlat(struct at
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(blk_rq_raw_data_len(scmd->request),
+		     (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
Index: work/include/linux/blkdev.h
===================================================================
--- work.orig/include/linux/blkdev.h
+++ work/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;	/* length of padding and draining buffers */
 	unsigned int sense_len;
 	void *data;
 	void *sense;
@@ -477,6 +477,12 @@ enum {
 
 #define rq_data_dir(rq)		((rq)->cmd_flags & 1)
 
+/* data_len of the request sans extra stuff for padding and draining */
+static inline unsigned int blk_rq_raw_data_len(struct request *rq)
+{
+	return rq->data_len - min(rq->extra_len, rq->data_len);
+}
+
 /*
  * We regard a request as sync, if it's a READ or a SYNC write.
  */
Index: work/block/blk-settings.c
===================================================================
--- work.orig/block/blk-settings.c
+++ work/block/blk-settings.c
@@ -309,6 +309,10 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
  * does is adjust the queue so that the buf is always appended
  * silently to the scatterlist.
  *
+ * Appending draining buffer to a request modifies ->data_len such
+ * that it includes the drain buffer.  The original requested data
+ * length can be obtained using blk_rq_raw_data_len().
+ *
  * Note: This routine adjusts max_hw_segments to make room for
  * appending the drain buffer.  If you call
  * blk_queue_max_hw_segments() or blk_queue_max_phys_segments() after

[PATCH] block: separate out padding from alignment

[Tejun Heo]

Block layer alignment was used for two different purposes - memory
alignment and padding.  This causes problems in lower layers because
drivers which only require memory alignment ends up with adjusted
rq->data_len.  Separate out padding such that padding occurs iff
driver explicitly requests it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
As wrote before, the major problem was that drivers which don't want
size adjustment got it acciedentally by mixing up aligning and padding
which are two conceptually separate things.  Let padding occur iff the
driver explicitly requested it.  This makes both parties happy.

 block/blk-map.c           |   16 +++++++++-------
 block/blk-settings.c      |   17 +++++++++++++++++
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/blkdev.h    |    2 ++
 4 files changed, 30 insertions(+), 8 deletions(-)

Index: work/block/blk-settings.c
===================================================================
--- work.orig/block/blk-settings.c
+++ work/block/blk-settings.c
@@ -293,6 +293,23 @@ void blk_queue_stack_limits(struct reque
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
 /**
+ * blk_queue_dma_pad - set pad mask
+ * @q:     the request queue for the device
+ * @mask:  pad mask
+ *
+ * Set pad mask.  Direct IO requests are padded to the mask specified.
+ *
+ * Appending pad buffer to a request modifies ->data_len such that it
+ * includes the pad buffer.  The original requested data length can be
+ * obtained using blk_rq_raw_data_len().
+ **/
+void blk_queue_dma_pad(struct request_queue *q, unsigned int mask)
+{
+	q->dma_pad_mask = mask;
+}
+EXPORT_SYMBOL(blk_queue_dma_pad);
+
+/**
  * blk_queue_dma_drain - Set up a drain buffer for excess dma.
  *
  * @q:  the request queue for the device
Index: work/block/blk-map.c
===================================================================
--- work.orig/block/blk-map.c
+++ work/block/blk-map.c
@@ -43,6 +43,7 @@ static int __blk_rq_map_user(struct requ
 			     void __user *ubuf, unsigned int len)
 {
 	unsigned long uaddr;
+	unsigned int alignment;
 	struct bio *bio, *orig_bio;
 	int reading, ret;
 
@@ -53,8 +54,8 @@ static int __blk_rq_map_user(struct requ
 	 * direct dma. else, set up kernel bounce buffers
 	 */
 	uaddr = (unsigned long) ubuf;
-	if (!(uaddr & queue_dma_alignment(q)) &&
-	    !(len & queue_dma_alignment(q)))
+	alignment = queue_dma_alignment(q) | q->dma_pad_mask;
+	if (!(uaddr & alignment) && !(len & alignment))
 		bio = bio_map_user(q, NULL, uaddr, len, reading);
 	else
 		bio = bio_copy_user(q, uaddr, len, reading);
@@ -141,15 +142,16 @@ int blk_rq_map_user(struct request_queue
 
 	/*
 	 * __blk_rq_map_user() copies the buffers if starting address
-	 * or length isn't aligned.  As the copied buffer is always
-	 * page aligned, we know that there's enough room for padding.
-	 * Extend the last bio and update rq->data_len accordingly.
+	 * or length isn't aligned to dma_pad_mask.  As the copied
+	 * buffer is always page aligned, we know that there's enough
+	 * room for padding.  Extend the last bio and update
+	 * rq->data_len accordingly.
 	 *
 	 * On unmap, bio_uncopy_user() will use unmodified
 	 * bio_map_data pointed to by bio->bi_private.
 	 */
-	if (len & queue_dma_alignment(q)) {
-		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
+	if (len & q->dma_pad_mask) {
+		unsigned int pad_len = (q->dma_pad_mask & ~len) + 1;
 		struct bio *bio = rq->biotail;
 
 		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
Index: work/include/linux/blkdev.h
===================================================================
--- work.orig/include/linux/blkdev.h
+++ work/include/linux/blkdev.h
@@ -362,6 +362,7 @@ struct request_queue
 	unsigned long		seg_boundary_mask;
 	void			*dma_drain_buffer;
 	unsigned int		dma_drain_size;
+	unsigned int		dma_pad_mask;
 	unsigned int		dma_alignment;
 
 	struct blk_queue_tag	*queue_tags;
@@ -707,6 +708,7 @@ extern void blk_queue_max_hw_segments(st
 extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
 extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
+extern void blk_queue_dma_pad(struct request_queue *, unsigned int);
 extern int blk_queue_dma_drain(struct request_queue *q,
 			       dma_drain_needed_fn *dma_drain_needed,
 			       void *buf, unsigned int size);
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -862,9 +862,10 @@ static int ata_scsi_dev_config(struct sc
 		struct request_queue *q = sdev->request_queue;
 		void *buf;
 
-		/* set the min alignment */
+		/* set the min alignment and padding */
 		blk_queue_update_dma_alignment(sdev->request_queue,
 					       ATA_DMA_PAD_SZ - 1);
+		blk_queue_dma_pad(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
 
 		/* configure draining */
 		buf = kmalloc(ATAPI_MAX_DRAIN, q->bounce_gfp | GFP_KERNEL);

Re: [PATCH] block: separate out padding from alignment

[James Bottomley]

On Mon, 2008-03-03 at 15:10 +0900, Tejun Heo wrote:
> Block layer alignment was used for two different purposes - memory
> alignment and padding.  This causes problems in lower layers because
> drivers which only require memory alignment ends up with adjusted
> rq->data_len.  Separate out padding such that padding occurs iff
> driver explicitly requests it.

This puts the libsas SMP handler back into a working state again.

Thanks,

James

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 13:09:13 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> >> - I think bugs caused by using raw_data_len instead of data_len are more
> >> subtle than the other way around.  Using data_len instead of
> >> raw_data_len usually affects the application layer while using
> >> raw_data_len instead of data_len affects the DMA engine and transport layer.
> > 
> > If we add extra_len, we can get what raw_data_len and data_len
> > provide.
> > 
> > I can't see what changing the meaning of rq->data_len (and
> > investigating all the block drivers) gives us.
> 
> No matter which way you go, you change the meaning of rq->data_len and
> you MUST inspect rq->data_len usage whichever way you go.

The patch doens't change that rq->data_len means the true data
length. But yeah, it breaks rq->data_len == sum(sg). So it might break
some drivers.


> Apply your patch and try to do sg IO on IDE cdrom w/ various
> transfer lengths.

I've just tried the patch with both ata and libata and it seems to
work.

For anyone hitting this problem, please try the following patch:

http://lkml.org/lkml/2008/3/2/218


Thanks,

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>>> I can't see what changing the meaning of rq->data_len (and
>>> investigating all the block drivers) gives us.
>> No matter which way you go, you change the meaning of rq->data_len and
>> you MUST inspect rq->data_len usage whichever way you go.
> 
> The patch doens't change that rq->data_len means the true data
> length. But yeah, it breaks rq->data_len == sum(sg). So it might break
> some drivers.

Yeah, that's what I was saying.  You end up breaking one of the two
assumptions.  As sglist is getting modified for any driver if it has DMA
alignment set, whether rq->data_len is adjusted together or not, sglist
and data_len usages have to be audited.

>> Apply your patch and try to do sg IO on IDE cdrom w/ various
>> transfer lengths.
> 
> I've just tried the patch with both ata and libata and it seems to
> work.

Right, I missed you added extra_len in libata and IDE isn't using block
layer stuff yet.

> For anyone hitting this problem, please try the following patch:
> 
> http://lkml.org/lkml/2008/3/2/218

Whether rq->data_len stays with requested data buffer size or sum(sg), I
think we need to separate out padding from address alignment; otherwise,
we'll have to audit every block driver to make sure they can deal with
extended sglist no matter which value rq->data_len ends up indicating.

If padding is applied iff explicitly requested, rq->data_len indicates
matters only to the drivers which want to see the data length adjusted,
so most of the problems go away.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 18:21:13 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> >>> I can't see what changing the meaning of rq->data_len (and
> >>> investigating all the block drivers) gives us.
> >> No matter which way you go, you change the meaning of rq->data_len and
> >> you MUST inspect rq->data_len usage whichever way you go.
> > 
> > The patch doens't change that rq->data_len means the true data
> > length. But yeah, it breaks rq->data_len == sum(sg). So it might break
> > some drivers.
> 
> Yeah, that's what I was saying.  You end up breaking one of the two
> assumptions.  As sglist is getting modified for any driver if it has DMA
> alignment set, whether rq->data_len is adjusted together or not, sglist
> and data_len usages have to be audited.

My patch (well, James' original approach) doesn't affect drivers that
don't use drain buffer. rq->data_len still means the true data length
and rq->data_len is equal to sum(sg) for them. So right now we need to
audit only libata.

But your patch changes the meaning of rq->data_len. It affects all the
drivers. So it breaks non libata stuff, like the SMP handler. We need
to audit all the drivers.

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
> On Mon, 03 Mar 2008 18:21:13 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>>>> I can't see what changing the meaning of rq->data_len (and
>>>>> investigating all the block drivers) gives us.
>>>> No matter which way you go, you change the meaning of rq->data_len and
>>>> you MUST inspect rq->data_len usage whichever way you go.
>>> The patch doens't change that rq->data_len means the true data
>>> length. But yeah, it breaks rq->data_len == sum(sg). So it might break
>>> some drivers.
>> Yeah, that's what I was saying.  You end up breaking one of the two
>> assumptions.  As sglist is getting modified for any driver if it has DMA
>> alignment set, whether rq->data_len is adjusted together or not, sglist
>> and data_len usages have to be audited.
> 
> My patch (well, James' original approach) doesn't affect drivers that
> don't use drain buffer. rq->data_len still means the true data length
> and rq->data_len is equal to sum(sg) for them. So right now we need to
> audit only libata.

Your patch does change sglist for any driver which sets DMA alignment.
You'll definitely need to audit more than libata.

> But your patch changes the meaning of rq->data_len. It affects all the
> drivers. So it breaks non libata stuff, like the SMP handler. We need
> to audit all the drivers.

With both patches applied, sglist and data_len are adjusted only for
libata, so only drivers which explicitly requested buffer size
manipulation (currently only libata) need to be audited / updated.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 22:38:55 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> > On Mon, 03 Mar 2008 18:21:13 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>>>> I can't see what changing the meaning of rq->data_len (and
> >>>>> investigating all the block drivers) gives us.
> >>>> No matter which way you go, you change the meaning of rq->data_len and
> >>>> you MUST inspect rq->data_len usage whichever way you go.
> >>> The patch doens't change that rq->data_len means the true data
> >>> length. But yeah, it breaks rq->data_len == sum(sg). So it might break
> >>> some drivers.
> >> Yeah, that's what I was saying.  You end up breaking one of the two
> >> assumptions.  As sglist is getting modified for any driver if it has DMA
> >> alignment set, whether rq->data_len is adjusted together or not, sglist
> >> and data_len usages have to be audited.
> > 
> > My patch (well, James' original approach) doesn't affect drivers that
> > don't use drain buffer. rq->data_len still means the true data length
> > and rq->data_len is equal to sum(sg) for them. So right now we need to
> > audit only libata.
> 
> Your patch does change sglist for any driver which sets DMA alignment.

I overlook it. Where does it changes sglist?

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>>>> FUJITA Tomonori wrote:
>>>>>>> I can't see what changing the meaning of rq->data_len (and
>>>>>>> investigating all the block drivers) gives us.
>>>>>> No matter which way you go, you change the meaning of rq->data_len and
>>>>>> you MUST inspect rq->data_len usage whichever way you go.
>>>>> The patch doens't change that rq->data_len means the true data
>>>>> length. But yeah, it breaks rq->data_len == sum(sg). So it might break
>>>>> some drivers.
>>>> Yeah, that's what I was saying.  You end up breaking one of the two
>>>> assumptions.  As sglist is getting modified for any driver if it has DMA
>>>> alignment set, whether rq->data_len is adjusted together or not, sglist
>>>> and data_len usages have to be audited.
>>> My patch (well, James' original approach) doesn't affect drivers that
>>> don't use drain buffer. rq->data_len still means the true data length
>>> and rq->data_len is equal to sum(sg) for them. So right now we need to
>>> audit only libata.
>> Your patch does change sglist for any driver which sets DMA alignment.
> 
> I overlook it. Where does it changes sglist?

At the end of blk_rq_map_user() together with data_len / extra_len
mangling or were you talking about James' original patch?

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 22:55:56 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> >>>> FUJITA Tomonori wrote:
> >>>>>>> I can't see what changing the meaning of rq->data_len (and
> >>>>>>> investigating all the block drivers) gives us.
> >>>>>> No matter which way you go, you change the meaning of rq->data_len and
> >>>>>> you MUST inspect rq->data_len usage whichever way you go.
> >>>>> The patch doens't change that rq->data_len means the true data
> >>>>> length. But yeah, it breaks rq->data_len == sum(sg). So it might break
> >>>>> some drivers.
> >>>> Yeah, that's what I was saying.  You end up breaking one of the two
> >>>> assumptions.  As sglist is getting modified for any driver if it has DMA
> >>>> alignment set, whether rq->data_len is adjusted together or not, sglist
> >>>> and data_len usages have to be audited.
> >>> My patch (well, James' original approach) doesn't affect drivers that
> >>> don't use drain buffer. rq->data_len still means the true data length
> >>> and rq->data_len is equal to sum(sg) for them. So right now we need to
> >>> audit only libata.
> >> Your patch does change sglist for any driver which sets DMA alignment.
> > 
> > I overlook it. Where does it changes sglist?
> 
> At the end of blk_rq_map_user() together with data_len / extra_len
> mangling or were you talking about James' original patch?

With my patch, at the end of blk_rq_map_user, we have:

	if (len & queue_dma_alignment(q)) {
		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;

		rq->extra_len += pad_len;
	}


So no change as compared with 2.6.24?

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>> At the end of blk_rq_map_user() together with data_len / extra_len
>> mangling or were you talking about James' original patch?
> 
> With my patch, at the end of blk_rq_map_user, we have:
> 
> 	if (len & queue_dma_alignment(q)) {
> 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
> 
> 		rq->extra_len += pad_len;
> 	}
> 
> 
> So no change as compared with 2.6.24?

Oh.. you killed sg list manipulation.  Many controllers do allow odd
bytes as the last sg entry but not all.  Also, if you append drain
buffer after it, it ends up with unaligned sg entry in the middle and
rq->data_len + rq->extra_len will overrun the sg entry after the drain
page which is really dangerous.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Mon, 03 Mar 2008 23:22:46 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> >> At the end of blk_rq_map_user() together with data_len / extra_len
> >> mangling or were you talking about James' original patch?
> > 
> > With my patch, at the end of blk_rq_map_user, we have:
> > 
> > 	if (len & queue_dma_alignment(q)) {
> > 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
> > 
> > 		rq->extra_len += pad_len;
> > 	}
> > 
> > 
> > So no change as compared with 2.6.24?
> 
> Oh.. you killed sg list manipulation.  Many controllers do allow odd
> bytes as the last sg entry but not all.  Also, if you append drain

Until 2.6.24, these drivers have taken care about the issue by
themselves. There is no change as compared with 2.6.24.


> buffer after it, it ends up with unaligned sg entry in the middle and
> rq->data_len + rq->extra_len will overrun the sg entry after the drain
> page which is really dangerous.

The drivers know that they use drain buffer. They can take care about
themselves on this too. If we want to do explicitly, we could have
rq->pad_len and rq->drain_len instead of rq->extra_len, though I think
that we are fine without these values because these drivers already
tell the block layer what they want and know that the block layer
gives it.


Jens, want's your verdict on this?

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
> On Mon, 03 Mar 2008 23:22:46 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>>> At the end of blk_rq_map_user() together with data_len / extra_len
>>>> mangling or were you talking about James' original patch?
>>> With my patch, at the end of blk_rq_map_user, we have:
>>>
>>> 	if (len & queue_dma_alignment(q)) {
>>> 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
>>>
>>> 		rq->extra_len += pad_len;
>>> 	}
>>>
>>>
>>> So no change as compared with 2.6.24?
>> Oh.. you killed sg list manipulation.  Many controllers do allow odd
>> bytes as the last sg entry but not all.  Also, if you append drain
> 
> Until 2.6.24, these drivers have taken care about the issue by
> themselves. There is no change as compared with 2.6.24.

Yeah, libata did its own padding and needed to add draining.  Private
implementation was complex as hell and James suggested moving them to
block layer.  Are you suggesting moving them back to drivers?

>> buffer after it, it ends up with unaligned sg entry in the middle and
>> rq->data_len + rq->extra_len will overrun the sg entry after the drain
>> page which is really dangerous.
> 
> The drivers know that they use drain buffer. They can take care about
> themselves on this too. If we want to do explicitly, we could have
> rq->pad_len and rq->drain_len instead of rq->extra_len, though I think
> that we are fine without these values because these drivers already
> tell the block layer what they want and know that the block layer
> gives it.

So, if a driver has requested aligning and draining, the driver should
extend the sg entry before the last one by the alignment if draining was
used for the request and extent the last sg if the draining wasn't used.
 I'd rather just implement them in the drivers.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Tue, 04 Mar 2008 07:44:13 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> > On Mon, 03 Mar 2008 23:22:46 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>>> At the end of blk_rq_map_user() together with data_len / extra_len
> >>>> mangling or were you talking about James' original patch?
> >>> With my patch, at the end of blk_rq_map_user, we have:
> >>>
> >>> 	if (len & queue_dma_alignment(q)) {
> >>> 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
> >>>
> >>> 		rq->extra_len += pad_len;
> >>> 	}
> >>>
> >>>
> >>> So no change as compared with 2.6.24?
> >> Oh.. you killed sg list manipulation.  Many controllers do allow odd
> >> bytes as the last sg entry but not all.  Also, if you append drain
> > 
> > Until 2.6.24, these drivers have taken care about the issue by
> > themselves. There is no change as compared with 2.6.24.
> 
> Yeah, libata did its own padding and needed to add draining.  Private
> implementation was complex as hell and James suggested moving them to
> block layer.  Are you suggesting moving them back to drivers?

No, I'm not. I've been working on the IOMMUs to remove such
workarounds in LLDs.

What drivers need to do on this is just adding a padding length, that
is, drivers don't need to change the structure of the sg list (like
splitting a sg entry), right? And it doesn't break the SAS drivers
that support SATAPI, does it?

But I agree that drivers want to get a complete sglist so I'm fine
with adjusting sglist entries in the block layer with your secode
patch (separate out padding from alignment). As we discussed, I'm fine
with breaking sum(sg) == rq->data_len as long as rq->data_len means
the true data length.


> >> buffer after it, it ends up with unaligned sg entry in the middle and
> >> rq->data_len + rq->extra_len will overrun the sg entry after the drain
> >> page which is really dangerous.
> > 
> > The drivers know that they use drain buffer. They can take care about
> > themselves on this too. If we want to do explicitly, we could have
> > rq->pad_len and rq->drain_len instead of rq->extra_len, though I think
> > that we are fine without these values because these drivers already
> > tell the block layer what they want and know that the block layer
> > gives it.
> 
> So, if a driver has requested aligning and draining, the driver should
> extend the sg entry before the last one by the alignment if draining was
> used for the request and extent the last sg if the draining wasn't used.
>  I'd rather just implement them in the drivers.

The block layer extends the sg entry? The drivers just adjust
sg->length?

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>> Yeah, libata did its own padding and needed to add draining.  Private
>> implementation was complex as hell and James suggested moving them to
>> block layer.  Are you suggesting moving them back to drivers?
> 
> No, I'm not. I've been working on the IOMMUs to remove such
> workarounds in LLDs.
> 
> What drivers need to do on this is just adding a padding length, that
> is, drivers don't need to change the structure of the sg list (like
> splitting a sg entry), right? And it doesn't break the SAS drivers
> that support SATAPI, does it?
> 
> But I agree that drivers want to get a complete sglist so I'm fine
> with adjusting sglist entries in the block layer with your secode
> patch (separate out padding from alignment). As we discussed, I'm fine
> with breaking sum(sg) == rq->data_len as long as rq->data_len means
> the true data length.

As long as the second patch is in, what value rq->data_len indicates
doesn't matter to drivers which don't use explicit padding or draining,
so the situation is much more controlled.  I don't care which value
rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
is what IDE and libata which will be the major users of padding and/or
draining expect in rq->data_len but fixing up that shouldn't be too
difficult.  I guess this can be determined by Jens.  If Jens likes
rq->data_len to contain requested transfer size, I'll post updated patches.

>>>> buffer after it, it ends up with unaligned sg entry in the middle and
>>>> rq->data_len + rq->extra_len will overrun the sg entry after the drain
>>>> page which is really dangerous.
>>> The drivers know that they use drain buffer. They can take care about
>>> themselves on this too. If we want to do explicitly, we could have
>>> rq->pad_len and rq->drain_len instead of rq->extra_len, though I think
>>> that we are fine without these values because these drivers already
>>> tell the block layer what they want and know that the block layer
>>> gives it.
>> So, if a driver has requested aligning and draining, the driver should
>> extend the sg entry before the last one by the alignment if draining was
>> used for the request and extent the last sg if the draining wasn't used.
>>  I'd rather just implement them in the drivers.
> 
> The block layer extends the sg entry? The drivers just adjust
> sg->length?

Still, do you really wanna force such things into low level drivers?
That will be one extremely fragile API and will be really difficult to
tell when things go wrong.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Tue, 04 Mar 2008 11:32:56 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> >> Yeah, libata did its own padding and needed to add draining.  Private
> >> implementation was complex as hell and James suggested moving them to
> >> block layer.  Are you suggesting moving them back to drivers?
> > 
> > No, I'm not. I've been working on the IOMMUs to remove such
> > workarounds in LLDs.
> > 
> > What drivers need to do on this is just adding a padding length, that
> > is, drivers don't need to change the structure of the sg list (like
> > splitting a sg entry), right? And it doesn't break the SAS drivers
> > that support SATAPI, does it?
> > 
> > But I agree that drivers want to get a complete sglist so I'm fine
> > with adjusting sglist entries in the block layer with your secode
> > patch (separate out padding from alignment). As we discussed, I'm fine
> > with breaking sum(sg) == rq->data_len as long as rq->data_len means
> > the true data length.
> 
> As long as the second patch is in, what value rq->data_len indicates
> doesn't matter to drivers which don't use explicit padding or draining,
> so the situation is much more controlled.  I don't care which value
> rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
> is what IDE and libata which will be the major users of padding and/or
> draining expect in rq->data_len but fixing up that shouldn't be too
> difficult.  I guess this can be determined by Jens.  If Jens likes
> rq->data_len to contain requested transfer size, I'll post updated patches.

OK, I prefer rq->data_len means the true data length though you prefer
rq->data_len means the allocated buffer length (the true data length
plus padding and drain). We agree on other things. We can live with
either way.

Jens, what's your preference?


> >>>> buffer after it, it ends up with unaligned sg entry in the middle and
> >>>> rq->data_len + rq->extra_len will overrun the sg entry after the drain
> >>>> page which is really dangerous.
> >>> The drivers know that they use drain buffer. They can take care about
> >>> themselves on this too. If we want to do explicitly, we could have
> >>> rq->pad_len and rq->drain_len instead of rq->extra_len, though I think
> >>> that we are fine without these values because these drivers already
> >>> tell the block layer what they want and know that the block layer
> >>> gives it.
> >> So, if a driver has requested aligning and draining, the driver should
> >> extend the sg entry before the last one by the alignment if draining was
> >> used for the request and extent the last sg if the draining wasn't used.
> >>  I'd rather just implement them in the drivers.
> > 
> > The block layer extends the sg entry? The drivers just adjust
> > sg->length?
> 
> Still, do you really wanna force such things into low level drivers?
> That will be one extremely fragile API and will be really difficult to
> tell when things go wrong.

No, I don't, as I explained above. As long as rq->data_len means the
true data length, I'm fine. I knew that James' drain buffer patch
breaks rq->data_len == sum(sg). I don't care about it. I can
understand that drivers wants to a perfect sglist.

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, FUJITA Tomonori wrote:
> On Tue, 04 Mar 2008 11:32:56 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
> > FUJITA Tomonori wrote:
> > >> Yeah, libata did its own padding and needed to add draining.  Private
> > >> implementation was complex as hell and James suggested moving them to
> > >> block layer.  Are you suggesting moving them back to drivers?
> > > 
> > > No, I'm not. I've been working on the IOMMUs to remove such
> > > workarounds in LLDs.
> > > 
> > > What drivers need to do on this is just adding a padding length, that
> > > is, drivers don't need to change the structure of the sg list (like
> > > splitting a sg entry), right? And it doesn't break the SAS drivers
> > > that support SATAPI, does it?
> > > 
> > > But I agree that drivers want to get a complete sglist so I'm fine
> > > with adjusting sglist entries in the block layer with your secode
> > > patch (separate out padding from alignment). As we discussed, I'm fine
> > > with breaking sum(sg) == rq->data_len as long as rq->data_len means
> > > the true data length.
> > 
> > As long as the second patch is in, what value rq->data_len indicates
> > doesn't matter to drivers which don't use explicit padding or draining,
> > so the situation is much more controlled.  I don't care which value
> > rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
> > is what IDE and libata which will be the major users of padding and/or
> > draining expect in rq->data_len but fixing up that shouldn't be too
> > difficult.  I guess this can be determined by Jens.  If Jens likes
> > rq->data_len to contain requested transfer size, I'll post updated patches.
> 
> OK, I prefer rq->data_len means the true data length though you prefer
> rq->data_len means the allocated buffer length (the true data length
> plus padding and drain). We agree on other things. We can live with
> either way.
> 
> Jens, what's your preference?

I completely agree with you, ->data_len meaning true data length is way
cleaner imho. Only the driver should care for the padded length, all
other parts of the kernel only need to know what they actually got.

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Tue, 4 Mar 2008 09:59:46 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Tue, Mar 04 2008, FUJITA Tomonori wrote:
> > On Tue, 04 Mar 2008 11:32:56 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> > > FUJITA Tomonori wrote:
> > > >> Yeah, libata did its own padding and needed to add draining.  Private
> > > >> implementation was complex as hell and James suggested moving them to
> > > >> block layer.  Are you suggesting moving them back to drivers?
> > > > 
> > > > No, I'm not. I've been working on the IOMMUs to remove such
> > > > workarounds in LLDs.
> > > > 
> > > > What drivers need to do on this is just adding a padding length, that
> > > > is, drivers don't need to change the structure of the sg list (like
> > > > splitting a sg entry), right? And it doesn't break the SAS drivers
> > > > that support SATAPI, does it?
> > > > 
> > > > But I agree that drivers want to get a complete sglist so I'm fine
> > > > with adjusting sglist entries in the block layer with your secode
> > > > patch (separate out padding from alignment). As we discussed, I'm fine
> > > > with breaking sum(sg) == rq->data_len as long as rq->data_len means
> > > > the true data length.
> > > 
> > > As long as the second patch is in, what value rq->data_len indicates
> > > doesn't matter to drivers which don't use explicit padding or draining,
> > > so the situation is much more controlled.  I don't care which value
> > > rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
> > > is what IDE and libata which will be the major users of padding and/or
> > > draining expect in rq->data_len but fixing up that shouldn't be too
> > > difficult.  I guess this can be determined by Jens.  If Jens likes
> > > rq->data_len to contain requested transfer size, I'll post updated patches.
> > 
> > OK, I prefer rq->data_len means the true data length though you prefer
> > rq->data_len means the allocated buffer length (the true data length
> > plus padding and drain). We agree on other things. We can live with
> > either way.
> > 
> > Jens, what's your preference?
> 
> I completely agree with you, ->data_len meaning true data length is way
> cleaner imho. Only the driver should care for the padded length, all
> other parts of the kernel only need to know what they actually got.

OK, now we can fix the whole SG_IO (and bsg handler) mess.

Here's my patch with a proper description. which several people have
already tested (thanks!). Then we need an updated version of Tejun's
separate out padding from alignment patch.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] block: restore the meaning of rq->data_len to the true data length

The meaning of rq->data_len was changed to the length of an allocated
buffer from the true data length. It breaks SG_IO friends and
bsg. This patch restores the meaning of rq->data_len to the true data
length and adds rq->extra_len to store an extended length (due to
drain buffer and padding).

This patch also removes the code to update bio in blk_rq_map_user
introduced by the commit 40b01b9bbdf51ae543a04744283bf2d56c4a6afa.
The commit adjusts bio according to memory alignment
(queue_dma_alignment). However, memory alignment is NOT padding
alignment. This adjustment also breaks SG_IO friends and bsg. Padding
alignment needs to be fixed in a proper way (by a separate patch).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 block/blk-core.c          |    3 +--
 block/blk-map.c           |    6 +-----
 block/blk-merge.c         |    2 +-
 block/bsg.c               |    8 ++++----
 block/scsi_ioctl.c        |    4 ++--
 drivers/ata/libata-scsi.c |    6 +++---
 include/linux/blkdev.h    |    2 +-
 7 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 775c851..bfec406 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -127,7 +127,6 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->nr_hw_segments = 0;
 	rq->ioprio = 0;
 	rq->special = NULL;
-	rq->raw_data_len = 0;
 	rq->buffer = NULL;
 	rq->tag = -1;
 	rq->errors = 0;
@@ -135,6 +134,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->cmd_len = 0;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->sense_len = 0;
 	rq->data = NULL;
 	rq->sense = NULL;
@@ -2016,7 +2016,6 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 	rq->hard_cur_sectors = rq->current_nr_sectors;
 	rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
 	rq->buffer = bio_data(bio);
-	rq->raw_data_len = bio->bi_size;
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 09f7fd0..f559832 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,7 +19,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
 
-		rq->raw_data_len += bio->bi_size;
 		rq->data_len += bio->bi_size;
 	}
 	return 0;
@@ -151,11 +150,8 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 	 */
 	if (len & queue_dma_alignment(q)) {
 		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
-		struct bio *bio = rq->biotail;
 
-		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
-		bio->bi_size += pad_len;
-		rq->data_len += pad_len;
+		rq->extra_len += pad_len;
 	}
 
 	rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7506c4f..0f58616 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -231,7 +231,7 @@ new_segment:
 			    ((unsigned long)q->dma_drain_buffer) &
 			    (PAGE_SIZE - 1));
 		nsegs++;
-		rq->data_len += q->dma_drain_size;
+		rq->extra_len += q->dma_drain_size;
 	}
 
 	if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 7f3c095..8917c51 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	}
 
 	if (rq->next_rq) {
-		hdr->dout_resid = rq->raw_data_len;
-		hdr->din_resid = rq->next_rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
+		hdr->din_resid = rq->next_rq->data_len;
 		blk_rq_unmap_user(bidi_bio);
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
-		hdr->din_resid = rq->raw_data_len;
+		hdr->din_resid = rq->data_len;
 	else
-		hdr->dout_resid = rq->raw_data_len;
+		hdr->dout_resid = rq->data_len;
 
 	/*
 	 * If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e993cac..a2c3a93 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	hdr->info = 0;
 	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
 		hdr->info |= SG_INFO_CHECK;
-	hdr->resid = rq->raw_data_len;
+	hdr->resid = rq->data_len;
 	hdr->sb_len_wr = 0;
 
 	if (rq->sense_len && hdr->sbp) {
@@ -528,8 +528,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->data = NULL;
-	rq->raw_data_len = 0;
 	rq->data_len = 0;
+	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7b1f1ee..fe47922 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2538,7 +2538,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	}
 
 	qc->tf.command = ATA_CMD_PACKET;
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* check whether ATAPI DMA is safe */
 	if (!using_pio && ata_check_atapi_dma(qc))
@@ -2549,7 +2549,7 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->raw_data_len, (unsigned int)63 * 1024);
+	nbytes = min(scmd->request->data_len, (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
@@ -2875,7 +2875,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nbytes = scsi_bufflen(scmd);
+	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
 
 	/* request result TF and be quiet about device error */
 	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_QUIET;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6fe67d1..b72526c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -216,8 +216,8 @@ struct request {
 	unsigned int cmd_len;
 	unsigned char cmd[BLK_MAX_CDB];
 
-	unsigned int raw_data_len;
 	unsigned int data_len;
+	unsigned int extra_len;	/* length of alignment and padding */
 	unsigned int sense_len;
 	void *data;
 	void *sense;
-- 
1.5.3.6

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Tue, 04 Mar 2008 18:06:48 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 4 Mar 2008 09:59:46 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Tue, Mar 04 2008, FUJITA Tomonori wrote:
> > > On Tue, 04 Mar 2008 11:32:56 +0900
> > > Tejun Heo <htejun@gmail.com> wrote:
> > > 
> > > > FUJITA Tomonori wrote:
> > > > >> Yeah, libata did its own padding and needed to add draining.  Private
> > > > >> implementation was complex as hell and James suggested moving them to
> > > > >> block layer.  Are you suggesting moving them back to drivers?
> > > > > 
> > > > > No, I'm not. I've been working on the IOMMUs to remove such
> > > > > workarounds in LLDs.
> > > > > 
> > > > > What drivers need to do on this is just adding a padding length, that
> > > > > is, drivers don't need to change the structure of the sg list (like
> > > > > splitting a sg entry), right? And it doesn't break the SAS drivers
> > > > > that support SATAPI, does it?
> > > > > 
> > > > > But I agree that drivers want to get a complete sglist so I'm fine
> > > > > with adjusting sglist entries in the block layer with your secode
> > > > > patch (separate out padding from alignment). As we discussed, I'm fine
> > > > > with breaking sum(sg) == rq->data_len as long as rq->data_len means
> > > > > the true data length.
> > > > 
> > > > As long as the second patch is in, what value rq->data_len indicates
> > > > doesn't matter to drivers which don't use explicit padding or draining,
> > > > so the situation is much more controlled.  I don't care which value
> > > > rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
> > > > is what IDE and libata which will be the major users of padding and/or
> > > > draining expect in rq->data_len but fixing up that shouldn't be too
> > > > difficult.  I guess this can be determined by Jens.  If Jens likes
> > > > rq->data_len to contain requested transfer size, I'll post updated patches.
> > > 
> > > OK, I prefer rq->data_len means the true data length though you prefer
> > > rq->data_len means the allocated buffer length (the true data length
> > > plus padding and drain). We agree on other things. We can live with
> > > either way.
> > > 
> > > Jens, what's your preference?
> > 
> > I completely agree with you, ->data_len meaning true data length is way
> > cleaner imho. Only the driver should care for the padded length, all
> > other parts of the kernel only need to know what they actually got.
> 
> OK, now we can fix the whole SG_IO (and bsg handler) mess.
> 
> Here's my patch with a proper description. which several people have
> already tested (thanks!). Then we need an updated version of Tejun's
> separate out padding from alignment patch.

OK, I've updated his patch. Tejun, can you audit this?

Thanks,

=
From: Tejun Heo <htejun@gmail.com>
Subject: [PATCH] block: separate out padding from alignment

Block layer alignment was used for two different purposes - memory
alignment and padding.  This causes problems in lower layers because
drivers which only require memory alignment ends up with adjusted
rq->data_len.  Separate out padding such that padding occurs iff
driver explicitly requests it.

Tomo: restorethe code to update bio in blk_rq_map_user
      introduced by the commit 40b01b9bbdf51ae543a04744283bf2d56c4a6afa
      according to padding alignment.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 block/blk-map.c           |   20 +++++++++++++-------
 block/blk-settings.c      |   17 +++++++++++++++++
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/blkdev.h    |    2 ++
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index f559832..4e17dfd 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -43,6 +43,7 @@ static int __blk_rq_map_user(struct request_queue *q, struct request *rq,
 			     void __user *ubuf, unsigned int len)
 {
 	unsigned long uaddr;
+	unsigned int alignment;
 	struct bio *bio, *orig_bio;
 	int reading, ret;
 
@@ -53,8 +54,8 @@ static int __blk_rq_map_user(struct request_queue *q, struct request *rq,
 	 * direct dma. else, set up kernel bounce buffers
 	 */
 	uaddr = (unsigned long) ubuf;
-	if (!(uaddr & queue_dma_alignment(q)) &&
-	    !(len & queue_dma_alignment(q)))
+	alignment = queue_dma_alignment(q) | q->dma_pad_mask;
+	if (!(uaddr & alignment) && !(len & alignment))
 		bio = bio_map_user(q, NULL, uaddr, len, reading);
 	else
 		bio = bio_copy_user(q, uaddr, len, reading);
@@ -141,15 +142,20 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 
 	/*
 	 * __blk_rq_map_user() copies the buffers if starting address
-	 * or length isn't aligned.  As the copied buffer is always
-	 * page aligned, we know that there's enough room for padding.
-	 * Extend the last bio and update rq->data_len accordingly.
+	 * or length isn't aligned to dma_pad_mask.  As the copied
+	 * buffer is always page aligned, we know that there's enough
+	 * room for padding.  Extend the last bio and update
+	 * rq->data_len accordingly.
 	 *
 	 * On unmap, bio_uncopy_user() will use unmodified
 	 * bio_map_data pointed to by bio->bi_private.
 	 */
-	if (len & queue_dma_alignment(q)) {
-		unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
+	if (len & q->dma_pad_mask) {
+		unsigned int pad_len = (q->dma_pad_mask & ~len) + 1;
+		struct bio *bio = rq->biotail;
+
+		bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
+		bio->bi_size += pad_len;
 
 		rq->extra_len += pad_len;
 	}
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 9a8ffdd..5fcb625 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -293,6 +293,23 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
 /**
+ * blk_queue_dma_pad - set pad mask
+ * @q:     the request queue for the device
+ * @mask:  pad mask
+ *
+ * Set pad mask.  Direct IO requests are padded to the mask specified.
+ *
+ * Appending pad buffer to a request modifies ->data_len such that it
+ * includes the pad buffer.  The original requested data length can be
+ * obtained using blk_rq_raw_data_len().
+ **/
+void blk_queue_dma_pad(struct request_queue *q, unsigned int mask)
+{
+	q->dma_pad_mask = mask;
+}
+EXPORT_SYMBOL(blk_queue_dma_pad);
+
+/**
  * blk_queue_dma_drain - Set up a drain buffer for excess dma.
  *
  * @q:  the request queue for the device
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index fe47922..8f0e8f2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -862,9 +862,10 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		struct request_queue *q = sdev->request_queue;
 		void *buf;
 
-		/* set the min alignment */
+		/* set the min alignment and padding */
 		blk_queue_update_dma_alignment(sdev->request_queue,
 					       ATA_DMA_PAD_SZ - 1);
+		blk_queue_dma_pad(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
 
 		/* configure draining */
 		buf = kmalloc(ATAPI_MAX_DRAIN, q->bounce_gfp | GFP_KERNEL);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b72526c..6f79d40 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -362,6 +362,7 @@ struct request_queue
 	unsigned long		seg_boundary_mask;
 	void			*dma_drain_buffer;
 	unsigned int		dma_drain_size;
+	unsigned int		dma_pad_mask;
 	unsigned int		dma_alignment;
 
 	struct blk_queue_tag	*queue_tags;
@@ -701,6 +702,7 @@ extern void blk_queue_max_hw_segments(struct request_queue *, unsigned short);
 extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
 extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
+extern void blk_queue_dma_pad(struct request_queue *, unsigned int);
 extern int blk_queue_dma_drain(struct request_queue *q,
 			       dma_drain_needed_fn *dma_drain_needed,
 			       void *buf, unsigned int size);
-- 
1.5.3.6

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
> OK, I've updated his patch. Tejun, can you audit this?

Looks good to me.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, FUJITA Tomonori wrote:
> On Tue, 04 Mar 2008 18:06:48 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > On Tue, 4 Mar 2008 09:59:46 +0100
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > On Tue, Mar 04 2008, FUJITA Tomonori wrote:
> > > > On Tue, 04 Mar 2008 11:32:56 +0900
> > > > Tejun Heo <htejun@gmail.com> wrote:
> > > > 
> > > > > FUJITA Tomonori wrote:
> > > > > >> Yeah, libata did its own padding and needed to add draining.  Private
> > > > > >> implementation was complex as hell and James suggested moving them to
> > > > > >> block layer.  Are you suggesting moving them back to drivers?
> > > > > > 
> > > > > > No, I'm not. I've been working on the IOMMUs to remove such
> > > > > > workarounds in LLDs.
> > > > > > 
> > > > > > What drivers need to do on this is just adding a padding length, that
> > > > > > is, drivers don't need to change the structure of the sg list (like
> > > > > > splitting a sg entry), right? And it doesn't break the SAS drivers
> > > > > > that support SATAPI, does it?
> > > > > > 
> > > > > > But I agree that drivers want to get a complete sglist so I'm fine
> > > > > > with adjusting sglist entries in the block layer with your secode
> > > > > > patch (separate out padding from alignment). As we discussed, I'm fine
> > > > > > with breaking sum(sg) == rq->data_len as long as rq->data_len means
> > > > > > the true data length.
> > > > > 
> > > > > As long as the second patch is in, what value rq->data_len indicates
> > > > > doesn't matter to drivers which don't use explicit padding or draining,
> > > > > so the situation is much more controlled.  I don't care which value
> > > > > rq->data_len would indicate.  I'd prefer it equal sum(sg) as that value
> > > > > is what IDE and libata which will be the major users of padding and/or
> > > > > draining expect in rq->data_len but fixing up that shouldn't be too
> > > > > difficult.  I guess this can be determined by Jens.  If Jens likes
> > > > > rq->data_len to contain requested transfer size, I'll post updated patches.
> > > > 
> > > > OK, I prefer rq->data_len means the true data length though you prefer
> > > > rq->data_len means the allocated buffer length (the true data length
> > > > plus padding and drain). We agree on other things. We can live with
> > > > either way.
> > > > 
> > > > Jens, what's your preference?
> > > 
> > > I completely agree with you, ->data_len meaning true data length is way
> > > cleaner imho. Only the driver should care for the padded length, all
> > > other parts of the kernel only need to know what they actually got.
> > 
> > OK, now we can fix the whole SG_IO (and bsg handler) mess.
> > 
> > Here's my patch with a proper description. which several people have
> > already tested (thanks!). Then we need an updated version of Tejun's
> > separate out padding from alignment patch.
> 
> OK, I've updated his patch. Tejun, can you audit this?

Looks excellent to me, has a variant of this been tested as OK by the
users reporting the regression?

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Jens Axboe wrote:
> Looks excellent to me, has a variant of this been tested as OK by the
> users reporting the regression?

Yeah, the other version which added extra_len to data_len has been
verified to work.  The only difference is now libata is adding
extra_len, so this one should be safe.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Tejun Heo wrote:
> Jens Axboe wrote:
> > Looks excellent to me, has a variant of this been tested as OK by the
> > users reporting the regression?
> 
> Yeah, the other version which added extra_len to data_len has been
> verified to work.  The only difference is now libata is adding
> extra_len, so this one should be safe.

Great, since we all agree, I'll merge it up and pass it on.

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:

> Looks excellent to me, has a variant of this been tested as OK by the
> users reporting the regression?

K3b burning seems to be a nogo here.  This is git pulled this morning
though, so it's a somewhat different tree than previously tested fwtw.

[  136.440021] ata1.01: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
[  136.440043] ata1.01: cmd a0/00:00:00:00:00/00:00:00:00:00/b0 tag 0
[  136.440045]          cdb 51 00 00 00 00 00 00 00  02 00 00 00 00 00 00 00
[  136.440047]          res 58/00:02:00:02:00/00:00:00:00:00/b0 Emask 0x2 (HSM violation)
[  136.440053] ata1.01: status: { DRDY DRQ }
[  136.440086] ata1: soft resetting link
[  165.327627] ata1.01: qc timeout (cmd 0xa1)
[  165.327627] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
[  165.327627] ata1.01: revalidation failed (errno=-5)
[  165.327627] ata1: failed to recover some devices, retrying in 5 secs
[  177.272373] ata1: port is slow to respond, please be patient (Status 0x80)
[  180.388879] ata1: device not ready (errno=-16), forcing hardreset
[  180.388879] ata1: soft resetting link
[  210.832471] ata1.01: qc timeout (cmd 0xa1)
[  210.832471] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
[  210.832471] ata1.01: revalidation failed (errno=-5)
[  210.832471] ata1: failed to recover some devices, retrying in 5 secs
[  223.392899] ata1: port is slow to respond, please be patient (Status 0x80)
[  225.920376] ata1: device not ready (errno=-16), forcing hardreset
[  225.920376] ata1: soft resetting link
[  256.542565] ata1.01: qc timeout (cmd 0xa1)
[  256.542565] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
[  256.542565] ata1.01: revalidation failed (errno=-5)
[  256.542565] ata1.01: disabled
[  259.995199] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x40)
[  259.995214] ata1.00: revalidation failed (errno=-5)
[  259.995219] ata1: failed to recover some devices, retrying in 5 secs
[  265.047502] ata1: soft resetting link
[  262.397570] ata1.00: limited to UDMA/33 due to 40-wire cable
[  262.420039] ata1.00: configured for UDMA/33
[  262.420039] sr 0:0:1:0: [sr0] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE,SUGGEST_OK
[  262.420039] sr 0:0:1:0: [sr0] Sense Key : Aborted Command [current] [descriptor]
[  262.420039] Descriptor sense data with sense descriptors (in hex):
[  262.420039]         72 0b 47 00 00 00 00 0e 09 0c 00 00 00 02 00 00 
[  262.420039]         00 02 00 00 b0 58 
[  262.420039] sr 0:0:1:0: [sr0] Add. Sense: Scsi parity error
[  262.420039] ata1: EH complete
[  262.420257] sd 0:0:0:0: [sda] 312581808 512-byte hardware sectors (160042 MB)
[  262.420320] sd 0:0:0:0: [sda] Write Protect is off
[  262.420326] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
[  262.420390] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> 
> > Looks excellent to me, has a variant of this been tested as OK by the
> > users reporting the regression?
> 
> K3b burning seems to be a nogo here.  This is git pulled this morning
> though, so it's a somewhat different tree than previously tested fwtw.

can you please try git as of this morning without any patches applied,
and then pull

git://git.kernel.dk/linux-2.6-block.git for-linus

into that and see if that works?

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 13:39 +0100, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> > 
> > > Looks excellent to me, has a variant of this been tested as OK by the
> > > users reporting the regression?
> > 
> > K3b burning seems to be a nogo here.  This is git pulled this morning
> > though, so it's a somewhat different tree than previously tested fwtw.
> 
> can you please try git as of this morning without any patches applied,
> and then pull
> 
> git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> into that and see if that works?

I'll give it a shot in a bit.

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 13:43 +0100, Mike Galbraith wrote:

> > can you please try git as of this morning without any patches applied,
> > and then pull
> > 
> > git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > into that and see if that works?
> 
> I'll give it a shot in a bit.

Aw poo, so many choices.
I did:
	git add remote block-for-linus git://git.kernel.dk/linux-2.6-block.git
	git remote update
Now, which one do I check out?  block-for-linus/master maybe, or
block-for-linus/for-linus?

homer:..git/linux-2.6 # git checkout block-for-linus
error: pathspec 'block-for-linus' did not match any file(s) known to git.
Did you forget to 'git add'?
homer:..git/linux-2.6 # git branch -a
* master
  x86/master
  x86/mm
  block-for-linus/blktrace
  block-for-linus/cmdfilter
  block-for-linus/dynpipe
  block-for-linus/fcache
  block-for-linus/for-akpm
  block-for-linus/for-linus
  block-for-linus/io-cpu-affinity
  block-for-linus/io-cpu-affinity-kthread
  block-for-linus/loop-extent_map
  block-for-linus/loop-fastfs
  block-for-linus/master
  block-for-linus/plug
  block-for-linus/splice
  block-for-linus/syslet
  block-for-linus/syslet-share
  block-for-linus/timeout
  linux-next/master
  linux-next/stable
  origin/HEAD
  origin/master
  x86/base
  x86/for-akpm
  x86/for-linus
  x86/latest
  x86/master
  x86/mm
  x86/origin
  x86/testing

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 13:43 +0100, Mike Galbraith wrote:
> 
> > > can you please try git as of this morning without any patches applied,
> > > and then pull
> > > 
> > > git://git.kernel.dk/linux-2.6-block.git for-linus
> > > 
> > > into that and see if that works?
> > 
> > I'll give it a shot in a bit.
> 
> Aw poo, so many choices.
> I did:
> 	git add remote block-for-linus git://git.kernel.dk/linux-2.6-block.git
> 	git remote update
> Now, which one do I check out?  block-for-linus/master maybe, or
> block-for-linus/for-linus?

Re-read my original mail! It states that you should just pull:

git://git.kernel.dk/linux-2.6-block.git for-linus

into your linus branch, or just create a test branch off linus' master
and pull into that. IOW, it's the for-linus branch that you should pull,
nothing else.

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 14:03 +0100, Jens Axboe wrote:

> Re-read my original mail! It states that you should just pull:
> 
> git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> into your linus branch, or just create a test branch off linus' master
> and pull into that. IOW, it's the for-linus branch that you should pull,
> nothing else.

Well, I had a good reason.  You know how to un-pull, I know how to
un-remote to get back to pristine after I'm done testing... guaranteed
without whimpering pathetically on the git list ;-)

Anyway, I checked out the one with the big-fat-hint in it's name
(block-for-linus/for-linus).
Same error.  Git this morning with patches...
	restore_meaning_of_data_len.diff
	seperate_out_padding_from_alignment.diff
...reverted restored me to the originally reported k3b error, nothing
new noted.

If I tested the wrong branch, whack me upside the head, and I'll follow
your pull destructions, and figure out how to un-pull later.

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 14:03 +0100, Jens Axboe wrote:
> 
> > Re-read my original mail! It states that you should just pull:
> > 
> > git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > into your linus branch, or just create a test branch off linus' master
> > and pull into that. IOW, it's the for-linus branch that you should pull,
> > nothing else.
> 
> Well, I had a good reason.  You know how to un-pull, I know how to
> un-remote to get back to pristine after I'm done testing... guaranteed
> without whimpering pathetically on the git list ;-)

OK, if you're on master, it's pretty easy:

$ git branch test-branch
$ git checkout test-branch
$ git pull git://git.kernel.dk/linux-2.6-block.git for-linus

[build, boot, test]
$ git checkout master
$ git branch -D test-branch

> Anyway, I checked out the one with the big-fat-hint in it's name
> (block-for-linus/for-linus).
> Same error.  Git this morning with patches...
> 	restore_meaning_of_data_len.diff
> 	seperate_out_padding_from_alignment.diff
> ...reverted restored me to the originally reported k3b error, nothing
> new noted.
> 
> If I tested the wrong branch, whack me upside the head, and I'll follow
> your pull destructions, and figure out how to un-pull later.

for-linus is the right branch, but I'm just a little worried that you
didn't test what you think you tested. What does cat .git/HEAD say? If
that is a ref to a file (eg refs/heads/master), what does that file
contain?

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Tue, 2008-03-04 at 14:03 +0100, Jens Axboe wrote:
> > 
> > > Re-read my original mail! It states that you should just pull:
> > > 
> > > git://git.kernel.dk/linux-2.6-block.git for-linus
> > > 
> > > into your linus branch, or just create a test branch off linus' master
> > > and pull into that. IOW, it's the for-linus branch that you should pull,
> > > nothing else.
> > 
> > Well, I had a good reason.  You know how to un-pull, I know how to
> > un-remote to get back to pristine after I'm done testing... guaranteed
> > without whimpering pathetically on the git list ;-)
> 
> OK, if you're on master, it's pretty easy:
> 
> $ git branch test-branch
> $ git checkout test-branch
> $ git pull git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> [build, boot, test]
> $ git checkout master
> $ git branch -D test-branch
> 
> > Anyway, I checked out the one with the big-fat-hint in it's name
> > (block-for-linus/for-linus).
> > Same error.  Git this morning with patches...
> > 	restore_meaning_of_data_len.diff
> > 	seperate_out_padding_from_alignment.diff
> > ...reverted restored me to the originally reported k3b error, nothing
> > new noted.
> > 
> > If I tested the wrong branch, whack me upside the head, and I'll follow
> > your pull destructions, and figure out how to un-pull later.
> 
> for-linus is the right branch, but I'm just a little worried that you
> didn't test what you think you tested. What does cat .git/HEAD say? If
> that is a ref to a file (eg refs/heads/master), what does that file
> contain?

Or just re-pull Linus' tree, the stuff is in now.

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 19:17 +0100, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Tue, 2008-03-04 at 14:03 +0100, Jens Axboe wrote:
> > 
> > > Re-read my original mail! It states that you should just pull:
> > > 
> > > git://git.kernel.dk/linux-2.6-block.git for-linus
> > > 
> > > into your linus branch, or just create a test branch off linus' master
> > > and pull into that. IOW, it's the for-linus branch that you should pull,
> > > nothing else.
> > 
> > Well, I had a good reason.  You know how to un-pull, I know how to
> > un-remote to get back to pristine after I'm done testing... guaranteed
> > without whimpering pathetically on the git list ;-)
> 
> OK, if you're on master, it's pretty easy:
> 
> $ git branch test-branch
> $ git checkout test-branch
> $ git pull git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> [build, boot, test]
> $ git checkout master
> $ git branch -D test-branch

Hm, that's simple enough.  I'll do this for the edification.  Thanks.
Maybe some day, I'll cease to be so paranoid that my test setup may
become compromised.  (at which time...)

> > Anyway, I checked out the one with the big-fat-hint in it's name
> > (block-for-linus/for-linus).
> > Same error.  Git this morning with patches...
> > 	restore_meaning_of_data_len.diff
> > 	seperate_out_padding_from_alignment.diff
> > ...reverted restored me to the originally reported k3b error, nothing
> > new noted.
> > 
> > If I tested the wrong branch, whack me upside the head, and I'll follow
> > your pull destructions, and figure out how to un-pull later.
> 
> for-linus is the right branch, but I'm just a little worried that you
> didn't test what you think you tested. What does cat .git/HEAD say? If
> that is a ref to a file (eg refs/heads/master), what does that file
> contain?

That wouldn't surprise me one bit.  (ergo...)

It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 19:17 +0100, Jens Axboe wrote:
> > On Tue, Mar 04 2008, Mike Galbraith wrote:
> > > 
> > > On Tue, 2008-03-04 at 14:03 +0100, Jens Axboe wrote:
> > > 
> > > > Re-read my original mail! It states that you should just pull:
> > > > 
> > > > git://git.kernel.dk/linux-2.6-block.git for-linus
> > > > 
> > > > into your linus branch, or just create a test branch off linus' master
> > > > and pull into that. IOW, it's the for-linus branch that you should pull,
> > > > nothing else.
> > > 
> > > Well, I had a good reason.  You know how to un-pull, I know how to
> > > un-remote to get back to pristine after I'm done testing... guaranteed
> > > without whimpering pathetically on the git list ;-)
> > 
> > OK, if you're on master, it's pretty easy:
> > 
> > $ git branch test-branch
> > $ git checkout test-branch
> > $ git pull git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > [build, boot, test]
> > $ git checkout master
> > $ git branch -D test-branch
> 
> Hm, that's simple enough.  I'll do this for the edification.  Thanks.
> Maybe some day, I'll cease to be so paranoid that my test setup may
> become compromised.  (at which time...)
> 
> > > Anyway, I checked out the one with the big-fat-hint in it's name
> > > (block-for-linus/for-linus).
> > > Same error.  Git this morning with patches...
> > > 	restore_meaning_of_data_len.diff
> > > 	seperate_out_padding_from_alignment.diff
> > > ...reverted restored me to the originally reported k3b error, nothing
> > > new noted.
> > > 
> > > If I tested the wrong branch, whack me upside the head, and I'll follow
> > > your pull destructions, and figure out how to un-pull later.
> > 
> > for-linus is the right branch, but I'm just a little worried that you
> > didn't test what you think you tested. What does cat .git/HEAD say? If
> > that is a ref to a file (eg refs/heads/master), what does that file
> > contain?
> 
> That wouldn't surprise me one bit.  (ergo...)
> 
> It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?

That looks right, then perhaps there's still an issue there :/
Logs?

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 19:45 +0100, Jens Axboe wrote:

> > It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?
> 
> That looks right, then perhaps there's still an issue there :/
> Logs?

Tejuns patchlet (below) fixed it here.

Date:	Wed, 05 Mar 2008 01:42:45 +0900
From:	Tejun Heo <htejun@gmail.com>
To:	FUJITA Tomonori <tomof@acm.org>
CC:	efault@gmx.de, jens.axboe@oracle.com, fujita.tomonori@lab.ntt.co.jp, James.Bottomley@HansenPartnership.com, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org, linux-scsi@vger.kernel.org, jgarzik@pobox.com, bzolnier@gmail.com
Subject: Re: [PATCH] block: fix residual byte count handling

Tejun Heo wrote:
> Tejun Heo wrote:
>> FUJITA Tomonori wrote:
>>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>>> gruesome.  Can you please try the other patches?
>>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
>> The extra_len you added to qc->nbytes should be it.  The only other
>> place to pay attention is the ATAPI transfer chunk size and your patch
>> seems to get it right.
>>
>>> Now Jens' git tree should work with all the non libata stuff, ide,
>>> firewire, bsg, etc. But I'm not sure about libata.
>> With the second patch, all others should be fine no matter what.  I'll
>> go check libata part again.
> 
> I can reproduce the problem here and it's very weird.  I'll report back
> when I know more.

Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
ready for rq->data_len != sum(sg).  When adjusted command completes,
SCSI midlayer completes the command with rq->data_len for PC commands
which eventually ends up in __end_that_request_first().  As there are
extra sg area left after completing rq->data_len, blk layer says so to
SCSI layer and SCSI layer retries the command only with the appended
area.

The following patch gets the writing going.  I really think it's a
serious mistake to break rq->data_len == sum(sg).  If we break
rq->data_len == requested size, the worst bugs are giving wrong size
when issuing commands to application layer of devices which is
relatively easy to spot and not all that command anyway.  Breaking
rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
engine programming and transport layer.  Oh well...

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fecba05..32439ac 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd);
+	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 19:45 +0100, Jens Axboe wrote:
> 
> > > It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?
> > 
> > That looks right, then perhaps there's still an issue there :/
> > Logs?
> 
> Tejuns patchlet (below) fixed it here.

OK, can you try changing that to

        good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;

and retest?

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 19:54 +0100, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Tue, 2008-03-04 at 19:45 +0100, Jens Axboe wrote:
> > 
> > > > It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?
> > > 
> > > That looks right, then perhaps there's still an issue there :/
> > > Logs?
> > 
> > Tejuns patchlet (below) fixed it here.
> 
> OK, can you try changing that to
> 
>         good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> 
> and retest?

Yup, disk #42 is happily burning away.

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 19:54 +0100, Jens Axboe wrote:
> > On Tue, Mar 04 2008, Mike Galbraith wrote:
> > > 
> > > On Tue, 2008-03-04 at 19:45 +0100, Jens Axboe wrote:
> > > 
> > > > > It says cc66b4512cae8df4ed1635483210aabf7690ec27... kewpie doll?
> > > > 
> > > > That looks right, then perhaps there's still an issue there :/
> > > > Logs?
> > > 
> > > Tejuns patchlet (below) fixed it here.
> > 
> > OK, can you try changing that to
> > 
> >         good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> > 
> > and retest?
> 
> Yup, disk #42 is happily burning away.

Super, patch heading to Linus now. Thanks for all your testing, Mike!

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[James Bottomley]

On Tue, 2008-03-04 at 13:39 +0100, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> > 
> > > Looks excellent to me, has a variant of this been tested as OK by the
> > > users reporting the regression?
> > 
> > K3b burning seems to be a nogo here.  This is git pulled this morning
> > though, so it's a somewhat different tree than previously tested fwtw.
> 
> can you please try git as of this morning without any patches applied,
> and then pull
> 
> git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> into that and see if that works?

Works for me with the SAS SMP handler.  Both input request and output
response frame sizes are picked up and returned with the correct
residues.

James

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, James Bottomley wrote:
> On Tue, 2008-03-04 at 13:39 +0100, Jens Axboe wrote:
> > On Tue, Mar 04 2008, Mike Galbraith wrote:
> > > 
> > > On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> > > 
> > > > Looks excellent to me, has a variant of this been tested as OK by the
> > > > users reporting the regression?
> > > 
> > > K3b burning seems to be a nogo here.  This is git pulled this morning
> > > though, so it's a somewhat different tree than previously tested fwtw.
> > 
> > can you please try git as of this morning without any patches applied,
> > and then pull
> > 
> > git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > into that and see if that works?
> 
> Works for me with the SAS SMP handler.  Both input request and output
> response frame sizes are picked up and returned with the correct
> residues.

Goodie, now we just need to figure out why it doesn't work for Mike
yet...

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[walt]

Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
>> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
>>
>>> Looks excellent to me, has a variant of this been tested as OK by the
>>> users reporting the regression?
>> K3b burning seems to be a nogo here.  This is git pulled this morning
>> though, so it's a somewhat different tree than previously tested fwtw.
>
> can you please try git as of this morning without any patches applied,
> and then pull
>
> git://git.kernel.dk/linux-2.6-block.git for-linus
>
> into that and see if that works?

Unfortunately this doesn't fix a problem I've discussed off-list with
Kiyoshi Ueda, who suggested that I should follow this thread and try
any patches posted here.

Here is what happens when I try to mount a CD (before and after I
pull 'for-linus'):

hdc: ide_cd_check_ireason: wrong transfer direction!
cdrom: failed setting lba address space
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown
hdc: drive not ready for command
hdc: status timeout: status=0xd0 { Busy }
ide: failed opcode was: unknown
hdc: DMA disabled
hdc: drive not ready for command
hdc: ATAPI reset complete
ISO 9660 Extensions: Microsoft Joliet Level 3
ISOFS: changing to secondary root
VFS: busy inodes on changed media.

The mount can take from 5 seconds on up to a minute or so before the
CD can be accessed.

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

walt wrote:
> Jens Axboe wrote:
>> On Tue, Mar 04 2008, Mike Galbraith wrote:
>>> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
>>>
>>>> Looks excellent to me, has a variant of this been tested as OK by the
>>>> users reporting the regression?
>>> K3b burning seems to be a nogo here.  This is git pulled this morning
>>> though, so it's a somewhat different tree than previously tested fwtw.
>>
>> can you please try git as of this morning without any patches applied,
>> and then pull
>>
>> git://git.kernel.dk/linux-2.6-block.git for-linus
>>
>> into that and see if that works?
> 
> Unfortunately this doesn't fix a problem I've discussed off-list with
> Kiyoshi Ueda, who suggested that I should follow this thread and try
> any patches posted here.
> 
> Here is what happens when I try to mount a CD (before and after I
> pull 'for-linus'):
> 
> hdc: ide_cd_check_ireason: wrong transfer direction!
> cdrom: failed setting lba address space
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
> ide: failed opcode was: unknown
> hdc: drive not ready for command
> hdc: status timeout: status=0xd0 { Busy }
> ide: failed opcode was: unknown
> hdc: DMA disabled
> hdc: drive not ready for command
> hdc: ATAPI reset complete
> ISO 9660 Extensions: Microsoft Joliet Level 3
> ISOFS: changing to secondary root
> VFS: busy inodes on changed media.
> 
> The mount can take from 5 seconds on up to a minute or so before the
> CD can be accessed.

Which version did you try?  There was a recent IDE bug fix which
affected CD recording.  Commit bcd88ac3b2ff2eae3d0fa57a6b02d4fce5392f32
which is included in 2.6.25-rc3.  Also, did 2.6.24 work?

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[walt]

Tejun Heo wrote:
> walt wrote:
>> Jens Axboe wrote:
>>> On Tue, Mar 04 2008, Mike Galbraith wrote:
>>>> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
>>>>
>>>>> Looks excellent to me, has a variant of this been tested as OK by the
>>>>> users reporting the regression?
>>>> K3b burning seems to be a nogo here.  This is git pulled this morning
>>>> though, so it's a somewhat different tree than previously tested fwtw.
>>> can you please try git as of this morning without any patches applied,
>>> and then pull
>>>
>>> git://git.kernel.dk/linux-2.6-block.git for-linus
>>>
>>> into that and see if that works?
>> Unfortunately this doesn't fix a problem I've discussed off-list with
>> Kiyoshi Ueda, who suggested that I should follow this thread and try
>> any patches posted here.
>>
>> Here is what happens when I try to mount a CD (before and after I
>> pull 'for-linus'):
>>
>> hdc: ide_cd_check_ireason: wrong transfer direction!
>> cdrom: failed setting lba address space
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status error: status=0x58 { DriveReady SeekComplete DataRequest }
>> ide: failed opcode was: unknown
>> hdc: drive not ready for command
>> hdc: status timeout: status=0xd0 { Busy }
>> ide: failed opcode was: unknown
>> hdc: DMA disabled
>> hdc: drive not ready for command
>> hdc: ATAPI reset complete
>> ISO 9660 Extensions: Microsoft Joliet Level 3
>> ISOFS: changing to secondary root
>> VFS: busy inodes on changed media.
>>
>> The mount can take from 5 seconds on up to a minute or so before the
>> CD can be accessed.
>
> Which version did you try?  There was a recent IDE bug fix which
> affected CD recording.  Commit bcd88ac3b2ff2eae3d0fa57a6b02d4fce5392f32
> which is included in 2.6.25-rc3.

I pulled from Jens to Linus' latest git, as Jens instructed.  I noticed
no difference at all.

> Also, did 2.6.24 work?

Yes, vanilla 2.6.24 works fine.  Your patchlet of earlier today also
didn't fix it, so I'm beginning to think that maybe I'm in the wrong
thread :o)

Re: [PATCH] block: fix residual byte count handling

[Kiyoshi Ueda]

Hi,

On Tue, 04 Mar 2008 09:34:56 -0800, walt wrote:
> Jens Axboe wrote:
> > On Tue, Mar 04 2008, Mike Galbraith wrote:
> >> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> >>
> >>> Looks excellent to me, has a variant of this been tested as OK by the
> >>> users reporting the regression?
> >> K3b burning seems to be a nogo here.  This is git pulled this morning
> >> though, so it's a somewhat different tree than previously tested fwtw.
> >
> > can you please try git as of this morning without any patches applied,
> > and then pull
> >
> > git://git.kernel.dk/linux-2.6-block.git for-linus
> >
> > into that and see if that works?
> 
> Unfortunately this doesn't fix a problem I've discussed off-list with
> Kiyoshi Ueda, who suggested that I should follow this thread and try
> any patches posted here.
 
I think there was misunderstanding between us.
On off-list, I meant:
  o Your original problem was CD burning, and it looked same problem
    being discussed on this thread, according to this message:

> cdrecord: Warning: controller returns zero sized CD capabilities page.
> cdrecord: Warning: controller returns wrong page 0 for CD capabilities page (2A). 

    So I suggested you to watch this thread and try patches of this
    thread for CD burning problem.

  o The problem of ide_cd_check_ireason looked different from
    CD burning one.
    So I suggested you to report it as a different problem.

Thanks,
Kiyoshi Ueda

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Mike Galbraith wrote:
> On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> 
>> Looks excellent to me, has a variant of this been tested as OK by the
>> users reporting the regression?
> 
> K3b burning seems to be a nogo here.  This is git pulled this morning
> though, so it's a somewhat different tree than previously tested fwtw.
> 
> [  136.440021] ata1.01: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
> [  136.440043] ata1.01: cmd a0/00:00:00:00:00/00:00:00:00:00/b0 tag 0
> [  136.440045]          cdb 51 00 00 00 00 00 00 00  02 00 00 00 00 00 00 00
> [  136.440047]          res 58/00:02:00:02:00/00:00:00:00:00/b0 Emask 0x2 (HSM violation)
> [  136.440053] ata1.01: status: { DRDY DRQ }
> [  136.440086] ata1: soft resetting link
> [  165.327627] ata1.01: qc timeout (cmd 0xa1)
> [  165.327627] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> [  165.327627] ata1.01: revalidation failed (errno=-5)
> [  165.327627] ata1: failed to recover some devices, retrying in 5 secs
> [  177.272373] ata1: port is slow to respond, please be patient (Status 0x80)
> [  180.388879] ata1: device not ready (errno=-16), forcing hardreset
> [  180.388879] ata1: soft resetting link
> [  210.832471] ata1.01: qc timeout (cmd 0xa1)
> [  210.832471] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> [  210.832471] ata1.01: revalidation failed (errno=-5)
> [  210.832471] ata1: failed to recover some devices, retrying in 5 secs
> [  223.392899] ata1: port is slow to respond, please be patient (Status 0x80)
> [  225.920376] ata1: device not ready (errno=-16), forcing hardreset
> [  225.920376] ata1: soft resetting link
> [  256.542565] ata1.01: qc timeout (cmd 0xa1)
> [  256.542565] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> [  256.542565] ata1.01: revalidation failed (errno=-5)
> [  256.542565] ata1.01: disabled

Aiee... device going down after timing out on READ_DISC_INFO.  That's
gruesome.  Can you please try the other patches?

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 21:40 +0900, Tejun Heo wrote:

> Aiee... device going down after timing out on READ_DISC_INFO.  That's
> gruesome.  Can you please try the other patches?

I tried your last yesterday, and k3b worked fine.

	-Mike

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Tue, 04 Mar 2008 21:40:53 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Mike Galbraith wrote:
> > On Tue, 2008-03-04 at 10:35 +0100, Jens Axboe wrote:
> > 
> >> Looks excellent to me, has a variant of this been tested as OK by the
> >> users reporting the regression?
> > 
> > K3b burning seems to be a nogo here.  This is git pulled this morning
> > though, so it's a somewhat different tree than previously tested fwtw.
> > 
> > [  136.440021] ata1.01: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
> > [  136.440043] ata1.01: cmd a0/00:00:00:00:00/00:00:00:00:00/b0 tag 0
> > [  136.440045]          cdb 51 00 00 00 00 00 00 00  02 00 00 00 00 00 00 00
> > [  136.440047]          res 58/00:02:00:02:00/00:00:00:00:00/b0 Emask 0x2 (HSM violation)
> > [  136.440053] ata1.01: status: { DRDY DRQ }
> > [  136.440086] ata1: soft resetting link
> > [  165.327627] ata1.01: qc timeout (cmd 0xa1)
> > [  165.327627] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> > [  165.327627] ata1.01: revalidation failed (errno=-5)
> > [  165.327627] ata1: failed to recover some devices, retrying in 5 secs
> > [  177.272373] ata1: port is slow to respond, please be patient (Status 0x80)
> > [  180.388879] ata1: device not ready (errno=-16), forcing hardreset
> > [  180.388879] ata1: soft resetting link
> > [  210.832471] ata1.01: qc timeout (cmd 0xa1)
> > [  210.832471] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> > [  210.832471] ata1.01: revalidation failed (errno=-5)
> > [  210.832471] ata1: failed to recover some devices, retrying in 5 secs
> > [  223.392899] ata1: port is slow to respond, please be patient (Status 0x80)
> > [  225.920376] ata1: device not ready (errno=-16), forcing hardreset
> > [  225.920376] ata1: soft resetting link
> > [  256.542565] ata1.01: qc timeout (cmd 0xa1)
> > [  256.542565] ata1.01: failed to IDENTIFY (I/O error, err_mask=0x4)
> > [  256.542565] ata1.01: revalidation failed (errno=-5)
> > [  256.542565] ata1.01: disabled
> 
> Aiee... device going down after timing out on READ_DISC_INFO.  That's
> gruesome.  Can you please try the other patches?

Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?

Now Jens' git tree should work with all the non libata stuff, ide,
firewire, bsg, etc. But I'm not sure about libata.

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>> gruesome.  Can you please try the other patches?
> 
> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?

The extra_len you added to qc->nbytes should be it.  The only other
place to pay attention is the ATAPI transfer chunk size and your patch
seems to get it right.

> Now Jens' git tree should work with all the non libata stuff, ide,
> firewire, bsg, etc. But I'm not sure about libata.

With the second patch, all others should be fine no matter what.  I'll
go check libata part again.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Tejun Heo wrote:
> FUJITA Tomonori wrote:
>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>> gruesome.  Can you please try the other patches?
>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
> 
> The extra_len you added to qc->nbytes should be it.  The only other
> place to pay attention is the ATAPI transfer chunk size and your patch
> seems to get it right.
> 
>> Now Jens' git tree should work with all the non libata stuff, ide,
>> firewire, bsg, etc. But I'm not sure about libata.
> 
> With the second patch, all others should be fine no matter what.  I'll
> go check libata part again.

I can reproduce the problem here and it's very weird.  I'll report back
when I know more.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Tejun Heo wrote:
> Tejun Heo wrote:
>> FUJITA Tomonori wrote:
>>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>>> gruesome.  Can you please try the other patches?
>>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
>> The extra_len you added to qc->nbytes should be it.  The only other
>> place to pay attention is the ATAPI transfer chunk size and your patch
>> seems to get it right.
>>
>>> Now Jens' git tree should work with all the non libata stuff, ide,
>>> firewire, bsg, etc. But I'm not sure about libata.
>> With the second patch, all others should be fine no matter what.  I'll
>> go check libata part again.
> 
> I can reproduce the problem here and it's very weird.  I'll report back
> when I know more.

Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
ready for rq->data_len != sum(sg).  When adjusted command completes,
SCSI midlayer completes the command with rq->data_len for PC commands
which eventually ends up in __end_that_request_first().  As there are
extra sg area left after completing rq->data_len, blk layer says so to
SCSI layer and SCSI layer retries the command only with the appended
area.

The following patch gets the writing going.  I really think it's a
serious mistake to break rq->data_len == sum(sg).  If we break
rq->data_len == requested size, the worst bugs are giving wrong size
when issuing commands to application layer of devices which is
relatively easy to spot and not all that command anyway.  Breaking
rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
engine programming and transport layer.  Oh well...

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fecba05..32439ac 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd);
+	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)


-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Boaz Harrosh]

On Tue, Mar 04 2008 at 18:42 +0200, Tejun Heo <htejun@gmail.com> wrote:
> Tejun Heo wrote:
>> Tejun Heo wrote:
>>> FUJITA Tomonori wrote:
>>>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>>>> gruesome.  Can you please try the other patches?
>>>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
>>> The extra_len you added to qc->nbytes should be it.  The only other
>>> place to pay attention is the ATAPI transfer chunk size and your patch
>>> seems to get it right.
>>>
>>>> Now Jens' git tree should work with all the non libata stuff, ide,
>>>> firewire, bsg, etc. But I'm not sure about libata.
>>> With the second patch, all others should be fine no matter what.  I'll
>>> go check libata part again.
>> I can reproduce the problem here and it's very weird.  I'll report back
>> when I know more.
> 
> Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
> ready for rq->data_len != sum(sg).  When adjusted command completes,
> SCSI midlayer completes the command with rq->data_len for PC commands
> which eventually ends up in __end_that_request_first().  As there are
> extra sg area left after completing rq->data_len, blk layer says so to
> SCSI layer and SCSI layer retries the command only with the appended
> area.
> 
> The following patch gets the writing going.  I really think it's a
> serious mistake to break rq->data_len == sum(sg).  If we break
> rq->data_len == requested size, the worst bugs are giving wrong size
> when issuing commands to application layer of devices which is
> relatively easy to spot and not all that command anyway.  Breaking
> rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
> engine programming and transport layer.  Oh well...
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index fecba05..32439ac 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = scsi_bufflen(cmd);
> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;

Are you sure? is it not:
+	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len

>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
> 
> 

I hate this patch. I wish you could maybe take the extra_len into 
account inside blk_end_request. The padding should be transparent
to all concerned but the requesting LLD and the internals of the
block layer. If block layer added padding it should take that into
account on completion. My $0.2.

Boaz
 

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Boaz Harrosh wrote:
>> -	good_bytes = scsi_bufflen(cmd);
>> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;
> 
> Are you sure? is it not:
> +	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len

You're right.  Sorry about the confusion.

>>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>>  		drv = scsi_cmd_to_driver(cmd);
>>  		if (drv->done)
>>
>>
> 
> I hate this patch. I wish you could maybe take the extra_len into 
> account inside blk_end_request. The padding should be transparent
> to all concerned but the requesting LLD and the internals of the
> block layer. If block layer added padding it should take that into
> account on completion. My $0.2.

Yeah, I hate it too.  As I've been saying all along, I think it just
should be rq->data_len.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[James Bottomley]

On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:
> Tejun Heo wrote:
> > Tejun Heo wrote:
> >> FUJITA Tomonori wrote:
> >>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
> >>>> gruesome.  Can you please try the other patches?
> >>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
> >> The extra_len you added to qc->nbytes should be it.  The only other
> >> place to pay attention is the ATAPI transfer chunk size and your patch
> >> seems to get it right.
> >>
> >>> Now Jens' git tree should work with all the non libata stuff, ide,
> >>> firewire, bsg, etc. But I'm not sure about libata.
> >> With the second patch, all others should be fine no matter what.  I'll
> >> go check libata part again.
> > 
> > I can reproduce the problem here and it's very weird.  I'll report back
> > when I know more.
> 
> Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
> ready for rq->data_len != sum(sg).  When adjusted command completes,
> SCSI midlayer completes the command with rq->data_len for PC commands
> which eventually ends up in __end_that_request_first().  As there are
> extra sg area left after completing rq->data_len, blk layer says so to
> SCSI layer and SCSI layer retries the command only with the appended
> area.
> 
> The following patch gets the writing going.  I really think it's a
> serious mistake to break rq->data_len == sum(sg).  If we break
> rq->data_len == requested size, the worst bugs are giving wrong size
> when issuing commands to application layer of devices which is
> relatively easy to spot and not all that command anyway.  Breaking
> rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
> engine programming and transport layer.  Oh well...
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index fecba05..32439ac 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = scsi_bufflen(cmd);
> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;

This doesn't look right.  scsi_bufflen(cmd) is req->data_len for PC
commands ... did you mean to add extra_len here?

James

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

James Bottomley wrote:
> On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:
>> Tejun Heo wrote:
>>> Tejun Heo wrote:
>>>> FUJITA Tomonori wrote:
>>>>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>>>>> gruesome.  Can you please try the other patches?
>>>>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
>>>> The extra_len you added to qc->nbytes should be it.  The only other
>>>> place to pay attention is the ATAPI transfer chunk size and your patch
>>>> seems to get it right.
>>>>
>>>>> Now Jens' git tree should work with all the non libata stuff, ide,
>>>>> firewire, bsg, etc. But I'm not sure about libata.
>>>> With the second patch, all others should be fine no matter what.  I'll
>>>> go check libata part again.
>>> I can reproduce the problem here and it's very weird.  I'll report back
>>> when I know more.
>> Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
>> ready for rq->data_len != sum(sg).  When adjusted command completes,
>> SCSI midlayer completes the command with rq->data_len for PC commands
>> which eventually ends up in __end_that_request_first().  As there are
>> extra sg area left after completing rq->data_len, blk layer says so to
>> SCSI layer and SCSI layer retries the command only with the appended
>> area.
>>
>> The following patch gets the writing going.  I really think it's a
>> serious mistake to break rq->data_len == sum(sg).  If we break
>> rq->data_len == requested size, the worst bugs are giving wrong size
>> when issuing commands to application layer of devices which is
>> relatively easy to spot and not all that command anyway.  Breaking
>> rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
>> engine programming and transport layer.  Oh well...
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index fecba05..32439ac 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>>  				"Notifying upper driver of completion "
>>  				"(result %x)\n", cmd->result));
>>  
>> -	good_bytes = scsi_bufflen(cmd);
>> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;
> 
> This doesn't look right.  scsi_bufflen(cmd) is req->data_len for PC
> commands ... did you mean to add extra_len here?

Yeap, sorry about the confusion.  Adding two times data_len accidentally
worked tho.  :-)

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:

> The following patch gets the writing going.

Bingo.

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:
> 
> > The following patch gets the writing going.
> 
> Bingo.

Pretty please test this on top of current -git?

I'll merge this up, it should do the trick. Would just be nice if you
could verify! :-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fecba05..e5c6f6a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd);
+	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[Mike Galbraith]

On Tue, 2008-03-04 at 20:25 +0100, Jens Axboe wrote:
> On Tue, Mar 04 2008, Mike Galbraith wrote:
> > 
> > On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:
> > 
> > > The following patch gets the writing going.
> > 
> > Bingo.
> 
> Pretty please test this on top of current -git?

?  That's the patch I just tested, and the tree.  Oh.. 976dde0..87baa2b
just a sec....

	-Mike

Re: [PATCH] block: fix residual byte count handling

[Jens Axboe]

On Tue, Mar 04 2008, Mike Galbraith wrote:
> 
> On Tue, 2008-03-04 at 20:25 +0100, Jens Axboe wrote:
> > On Tue, Mar 04 2008, Mike Galbraith wrote:
> > > 
> > > On Wed, 2008-03-05 at 01:42 +0900, Tejun Heo wrote:
> > > 
> > > > The following patch gets the writing going.
> > > 
> > > Bingo.
> > 
> > Pretty please test this on top of current -git?
> 
> ?  That's the patch I just tested, and the tree.  Oh.. 976dde0..87baa2b
> just a sec....

Yeah it is, mid-air collision of emails. So just disregard this one!

-- 
Jens Axboe

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Wed, 05 Mar 2008 01:42:45 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Tejun Heo wrote:
> > Tejun Heo wrote:
> >> FUJITA Tomonori wrote:
> >>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
> >>>> gruesome.  Can you please try the other patches?
> >>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
> >> The extra_len you added to qc->nbytes should be it.  The only other
> >> place to pay attention is the ATAPI transfer chunk size and your patch
> >> seems to get it right.
> >>
> >>> Now Jens' git tree should work with all the non libata stuff, ide,
> >>> firewire, bsg, etc. But I'm not sure about libata.
> >> With the second patch, all others should be fine no matter what.  I'll
> >> go check libata part again.
> > 
> > I can reproduce the problem here and it's very weird.  I'll report back
> > when I know more.
> 
> Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
> ready for rq->data_len != sum(sg).  When adjusted command completes,
> SCSI midlayer completes the command with rq->data_len for PC commands
> which eventually ends up in __end_that_request_first().  As there are
> extra sg area left after completing rq->data_len, blk layer says so to
> SCSI layer and SCSI layer retries the command only with the appended
> area.

Ah, thanks!


> The following patch gets the writing going.  I really think it's a
> serious mistake to break rq->data_len == sum(sg).  If we break
> rq->data_len == requested size, the worst bugs are giving wrong size
> when issuing commands to application layer of devices which is
> relatively easy to spot and not all that command anyway.  Breaking
> rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
> engine programming and transport layer.  Oh well...
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index fecba05..32439ac 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = scsi_bufflen(cmd);
> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;
>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
> 
> 

Hmm, does SCSI mid-layer need to care about how many bytes the block
layer allocates? I don't think that extra_len is NOT good_bytes.

I think that the block layer had better take care about it (fix
__end_that_request_first?).

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

FUJITA Tomonori wrote:
> Hmm, does SCSI mid-layer need to care about how many bytes the block
> layer allocates? I don't think that extra_len is NOT good_bytes.
> 
> I think that the block layer had better take care about it (fix
> __end_that_request_first?).

Yeah, probably calling completion functions w/o bytes count is the right
thing to do but what I was talking about was what could break when the
semantics of rq->data_len changed.  If we keep rq->data_len() ==
sum(sg), we keep it business as usual for all the rest except for the
device application layer if we don't we do the reverse and SCSI midlayer
completion was a good example, I think.

Things going the other way is fine with me but I at least want to hear a
valid rationale.  Till now all I got is "because that's the true size"
which doesn't really make much sense to me.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Tejun Heo wrote:
> FUJITA Tomonori wrote:
>> Hmm, does SCSI mid-layer need to care about how many bytes the block
>> layer allocates? I don't think that extra_len is NOT good_bytes.
>>
>> I think that the block layer had better take care about it (fix
>> __end_that_request_first?).
> 
> Yeah, probably calling completion functions w/o bytes count is the right
> thing to do but what I was talking about was what could break when the
> semantics of rq->data_len changed.  If we keep rq->data_len() ==
> sum(sg), we keep it business as usual for all the rest except for the
> device application layer if we don't we do the reverse and SCSI midlayer
> completion was a good example, I think.
> 
> Things going the other way is fine with me but I at least want to hear a
> valid rationale.  Till now all I got is "because that's the true size"
> which doesn't really make much sense to me.

I'm giving it another shot.  When the padding / draining thing was in
libata (or IDE) in that matter.  The whole thing looked like this.

  user - blk - SCSI - libata - LLD - controller - device
  <---------------------><----------------------><----->
            a                       b               c

a: Uses the 'true' request size and matching sg
b: Requires adjusted request size and matching sg
c: Don't really care about sg, but sometimes needs the true size.  For
   anything which gets attached behind ATA and which may require
   padding, transfer size is also sent in the CDB as well, which not
   all devices honor and that's one of the reasons why size adjustment
   is necessary.

If we move the adjustment to block layer and keep data_len == sum(sg),
it looks like.

  user - blk - SCSI - libata - LLD - controller - device
  <------><-------------------------------------><----->
      a                      b                      c

And a, b and c stay the same.  If we keep the requested size in
data_len.  Whole b gets inconsistent values in the middle while c gets
the value it wants in data_len, so we're risking much more to keep the
true size in rq->data_len when we could simply make it mean sum(sg).

Before the only thing which need updating was to correctly determine
the transfer size to feed to device.  Now we need to audit whole b.
In addition, such adjustments are made only when the driver explicitly
requested it, so for all others it doesn't really matter.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Wed, 05 Mar 2008 08:33:05 +0900
Tejun Heo <htejun@gmail.com> wrote:

> FUJITA Tomonori wrote:
> > Hmm, does SCSI mid-layer need to care about how many bytes the block
> > layer allocates? I don't think that extra_len is NOT good_bytes.
> > 
> > I think that the block layer had better take care about it (fix
> > __end_that_request_first?).
> 
> Yeah, probably calling completion functions w/o bytes count is the right
> thing to do but what I was talking about was what could break when the
> semantics of rq->data_len changed.  If we keep rq->data_len() ==
> sum(sg), we keep it business as usual for all the rest except for the
> device application layer if we don't we do the reverse and SCSI midlayer
> completion was a good example, I think.

sglist is a low-level I/O representation for device drivers. SCSI
midlayer should not care about sglist. We should not fix SCSI midlayer
for rq->data_len != sum(sg) change (so I can't agree with your
diagrams in another mail).

When if we change a rule, we need to fix something.

If we keep rq->data_len == sum(sg), we need to fix the device
application layer. If we keep rq->data_len == the true data length, we
need to fix the low-level drivers.

Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
since we are in -rc stages. But I plan to send a patch to revert it
and fix this issue in the block layer. I'd like to test it in -mm for
a while.

Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
know, we really want to remove it.


> Things going the other way is fine with me but I at least want to hear a
> valid rationale.  Till now all I got is "because that's the true size"
> which doesn't really make much sense to me.

Most of users of request structure care about only the real data
length, don't care about padding and drain length. Why do they bother
to use a helper function to get the real data length?

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Hello,

FUJITA Tomonori wrote:
> sglist is a low-level I/O representation for device drivers. SCSI
> midlayer should not care about sglist. We should not fix SCSI midlayer
> for rq->data_len != sum(sg) change (so I can't agree with your
> diagrams in another mail).

But that's not the way things currently are.

> When if we change a rule, we need to fix something.
> 
> If we keep rq->data_len == sum(sg), we need to fix the device
> application layer. If we keep rq->data_len == the true data length, we
> need to fix the low-level drivers.

Basically everything under block layer.

> Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
> since we are in -rc stages. But I plan to send a patch to revert it
> and fix this issue in the block layer. I'd like to test it in -mm for
> a while.
> 
> Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
> know, we really want to remove it.

If the way forward is to make anything but the low level drivers not
care about sglist, in the long term, the current scheme is fine but I
still don't think this way of doing things is safe one.  We're affecting
large portion of code based on what things should be in future not what
they currently are.

>> Things going the other way is fine with me but I at least want to hear a
>> valid rationale.  Till now all I got is "because that's the true size"
>> which doesn't really make much sense to me.
> 
> Most of users of request structure care about only the real data
> length, don't care about padding and drain length. Why do they bother
> to use a helper function to get the real data length?

I think this is where the difference comes from.  To me it seems
internal usage seems more wide-spread and more delicate and not too many
care about the true size and when they do only in well defined places.
Maybe it comes from the difference between your most and my most.

Thanks.

-- 
tejun

Re: [PATCH] block: fix residual byte count handling

[FUJITA Tomonori]

On Wed, 05 Mar 2008 09:44:01 +0900
Tejun Heo <htejun@gmail.com> wrote:

> >> Things going the other way is fine with me but I at least want to hear a
> >> valid rationale.  Till now all I got is "because that's the true size"
> >> which doesn't really make much sense to me.
> > 
> > Most of users of request structure care about only the real data
> > length, don't care about padding and drain length. Why do they bother
> > to use a helper function to get the real data length?
> 
> I think this is where the difference comes from.  To me it seems
> internal usage seems more wide-spread and more delicate and not too many
> care about the true size and when they do only in well defined places.
> Maybe it comes from the difference between your most and my most.

I don't think that they only in well defined places.

If you see scsi mid-layer (and LLDs), you can find several places that
use rq->data_len as the true data length.

Breaking rq->data_len == the true data length theoretically
wrong. Even if it affects only libata now, it will hurt us, I think.

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Hello, FUJITA.

FUJITA Tomonori wrote:
>>>> Things going the other way is fine with me but I at least want to hear a
>>>> valid rationale.  Till now all I got is "because that's the true size"
>>>> which doesn't really make much sense to me.
>>> Most of users of request structure care about only the real data
>>> length, don't care about padding and drain length. Why do they bother
>>> to use a helper function to get the real data length?
>> I think this is where the difference comes from.  To me it seems
>> internal usage seems more wide-spread and more delicate and not too many
>> care about the true size and when they do only in well defined places.
>> Maybe it comes from the difference between your most and my most.
> 
> I don't think that they only in well defined places.
> 
> If you see scsi mid-layer (and LLDs), you can find several places that
> use rq->data_len as the true data length.
> 
> Breaking rq->data_len == the true data length theoretically
> wrong. Even if it affects only libata now, it will hurt us, I think.

Yeap, I fully agree it's much better not to break any of the two
assumptions except when it's actually needed.  Both padding and draining
are requirements from low level driver which usually stems from hardware
kinkiness, so adjusting sg and length there and let the rest of system
not care about it sounds like a good idea to me.  Maybe something good
can come out of this long thread.  :-)

Thanks.

-- 
tejun

[PATCH] blk: missing add of padded bytes to io completion byte count

[Boaz Harrosh]

On Wed, Mar 05 2008 at 2:26 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Wed, 05 Mar 2008 08:33:05 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>> Hmm, does SCSI mid-layer need to care about how many bytes the block
>>> layer allocates? I don't think that extra_len is NOT good_bytes.
>>>
>>> I think that the block layer had better take care about it (fix
>>> __end_that_request_first?).
>> Yeah, probably calling completion functions w/o bytes count is the right
>> thing to do but what I was talking about was what could break when the
>> semantics of rq->data_len changed.  If we keep rq->data_len() ==
>> sum(sg), we keep it business as usual for all the rest except for the
>> device application layer if we don't we do the reverse and SCSI midlayer
>> completion was a good example, I think.
> 
> sglist is a low-level I/O representation for device drivers. SCSI
> midlayer should not care about sglist. We should not fix SCSI midlayer
> for rq->data_len != sum(sg) change (so I can't agree with your
> diagrams in another mail).
> 
> When if we change a rule, we need to fix something.
> 
> If we keep rq->data_len == sum(sg), we need to fix the device
> application layer. If we keep rq->data_len == the true data length, we
> need to fix the low-level drivers.
> 
> Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
> since we are in -rc stages. But I plan to send a patch to revert it
> and fix this issue in the block layer. I'd like to test it in -mm for
> a while.

No this commit is a serious bug, and the only fix is like you suggested
in __end_that_request_first. This is because it breaks that scsi-ml loop
where scsi_bufflen() can be less then blk_rq_bytes(). In that case this 
commit is a data corruption.

> Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
> know, we really want to remove it.
> 
> 
>> Things going the other way is fine with me but I at least want to hear a
>> valid rationale.  Till now all I got is "because that's the true size"
>> which doesn't really make much sense to me.
> 
> Most of users of request structure care about only the real data
> length, don't care about padding and drain length. Why do they bother
> to use a helper function to get the real data length?
> --

Submitted is the right fix to this problem, as pointed out by TOMO.
Please test it solves the CD burning problem.
(The patch includes the revert of commit e97a294e)
---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Wed, 5 Mar 2008 12:07:12 +0200
Subject: [PATCH] blk: missing add of padded bytes to io completion byte count

the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
because scsi-ml supports the ability to split a request into smaller chunks,
in which case scsi_bufflen() is smaller then request length. Then at completion
time the remainder can be issued as a new scsi command. In that case the above
commit is a data corruption.

Also in this fix all users of block layer are taken care of, and not only
scsi devices.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 block/blk-core.c    |    4 ++++
 drivers/scsi/scsi.c |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..37fcccc 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
 			     nr_bytes >> 9, req->sector);
 	}
 
+	if (nr_bytes >= blk_rq_bytes(req))
+		nr_bytes += req->extra_len;
+
 	total_bytes = bio_nbytes = 0;
 	while ((bio = req->bio) != NULL) {
 		int nbytes;
@@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
 	if (!req->bio)
 		return 0;
 
+	BUG_ON(total_bytes >= blk_rq_bytes(req));
 	/*
 	 * if the request wasn't completed, update state
 	 */
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..fecba05 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
-- 
1.5.3.3

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Mike Galbraith]

On Wed, 2008-03-05 at 12:16 +0200, Boaz Harrosh wrote:

> Please test it solves the CD burning problem.

Works for me.

	-Mike

> (The patch includes the revert of commit e97a294e)
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Wed, 5 Mar 2008 12:07:12 +0200
> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
> 
> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
> because scsi-ml supports the ability to split a request into smaller chunks,
> in which case scsi_bufflen() is smaller then request length. Then at completion
> time the remainder can be issued as a new scsi command. In that case the above
> commit is a data corruption.
> 
> Also in this fix all users of block layer are taken care of, and not only
> scsi devices.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  block/blk-core.c    |    4 ++++
>  drivers/scsi/scsi.c |    2 +-
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2a438a9..37fcccc 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>  			     nr_bytes >> 9, req->sector);
>  	}
>  
> +	if (nr_bytes >= blk_rq_bytes(req))
> +		nr_bytes += req->extra_len;
> +
>  	total_bytes = bio_nbytes = 0;
>  	while ((bio = req->bio) != NULL) {
>  		int nbytes;
> @@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
>  	if (!req->bio)
>  		return 0;
>  
> +	BUG_ON(total_bytes >= blk_rq_bytes(req));
>  	/*
>  	 * if the request wasn't completed, update state
>  	 */
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index e5c6f6a..fecba05 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> +	good_bytes = scsi_bufflen(cmd);
>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Jens Axboe]

On Wed, Mar 05 2008, Boaz Harrosh wrote:
> On Wed, Mar 05 2008 at 2:26 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Wed, 05 Mar 2008 08:33:05 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>> Hmm, does SCSI mid-layer need to care about how many bytes the block
> >>> layer allocates? I don't think that extra_len is NOT good_bytes.
> >>>
> >>> I think that the block layer had better take care about it (fix
> >>> __end_that_request_first?).
> >> Yeah, probably calling completion functions w/o bytes count is the right
> >> thing to do but what I was talking about was what could break when the
> >> semantics of rq->data_len changed.  If we keep rq->data_len() ==
> >> sum(sg), we keep it business as usual for all the rest except for the
> >> device application layer if we don't we do the reverse and SCSI midlayer
> >> completion was a good example, I think.
> > 
> > sglist is a low-level I/O representation for device drivers. SCSI
> > midlayer should not care about sglist. We should not fix SCSI midlayer
> > for rq->data_len != sum(sg) change (so I can't agree with your
> > diagrams in another mail).
> > 
> > When if we change a rule, we need to fix something.
> > 
> > If we keep rq->data_len == sum(sg), we need to fix the device
> > application layer. If we keep rq->data_len == the true data length, we
> > need to fix the low-level drivers.
> > 
> > Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
> > since we are in -rc stages. But I plan to send a patch to revert it
> > and fix this issue in the block layer. I'd like to test it in -mm for
> > a while.
> 
> No this commit is a serious bug, and the only fix is like you suggested
> in __end_that_request_first. This is because it breaks that scsi-ml loop
> where scsi_bufflen() can be less then blk_rq_bytes(). In that case this 
> commit is a data corruption.
> 
> > Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
> > know, we really want to remove it.
> > 
> > 
> >> Things going the other way is fine with me but I at least want to hear a
> >> valid rationale.  Till now all I got is "because that's the true size"
> >> which doesn't really make much sense to me.
> > 
> > Most of users of request structure care about only the real data
> > length, don't care about padding and drain length. Why do they bother
> > to use a helper function to get the real data length?
> > --
> 
> Submitted is the right fix to this problem, as pointed out by TOMO.
> Please test it solves the CD burning problem.
> (The patch includes the revert of commit e97a294e)
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Wed, 5 Mar 2008 12:07:12 +0200
> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
> 
> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
> because scsi-ml supports the ability to split a request into smaller chunks,
> in which case scsi_bufflen() is smaller then request length. Then at completion
> time the remainder can be issued as a new scsi command. In that case the above
> commit is a data corruption.

We needed something for -rc4, so it had to be rushed a bit...

> Also in this fix all users of block layer are taken care of, and not only
> scsi devices.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  block/blk-core.c    |    4 ++++
>  drivers/scsi/scsi.c |    2 +-
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2a438a9..37fcccc 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>  			     nr_bytes >> 9, req->sector);
>  	}
>  
> +	if (nr_bytes >= blk_rq_bytes(req))
> +		nr_bytes += req->extra_len;
> +
>  	total_bytes = bio_nbytes = 0;
>  	while ((bio = req->bio) != NULL) {
>  		int nbytes;
> @@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
>  	if (!req->bio)
>  		return 0;
>  
> +	BUG_ON(total_bytes >= blk_rq_bytes(req));

Make that a WARN_ON() first please. It's indeed a bug, but it wont be
critical and it's not fair killing everything since this padding stuff
is so fresh and may still need a tweak or two.

I'd be fine with making it a BUG_ON() post 2.6.25.

-- 
Jens Axboe

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Boaz Harrosh]

On Wed, Mar 05 2008 at 14:33 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Mar 05 2008, Boaz Harrosh wrote:
>> On Wed, Mar 05 2008 at 2:26 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>> On Wed, 05 Mar 2008 08:33:05 +0900
>>> Tejun Heo <htejun@gmail.com> wrote:
>>>
>>>> FUJITA Tomonori wrote:
>>>>> Hmm, does SCSI mid-layer need to care about how many bytes the block
>>>>> layer allocates? I don't think that extra_len is NOT good_bytes.
>>>>>
>>>>> I think that the block layer had better take care about it (fix
>>>>> __end_that_request_first?).
>>>> Yeah, probably calling completion functions w/o bytes count is the right
>>>> thing to do but what I was talking about was what could break when the
>>>> semantics of rq->data_len changed.  If we keep rq->data_len() ==
>>>> sum(sg), we keep it business as usual for all the rest except for the
>>>> device application layer if we don't we do the reverse and SCSI midlayer
>>>> completion was a good example, I think.
>>> sglist is a low-level I/O representation for device drivers. SCSI
>>> midlayer should not care about sglist. We should not fix SCSI midlayer
>>> for rq->data_len != sum(sg) change (so I can't agree with your
>>> diagrams in another mail).
>>>
>>> When if we change a rule, we need to fix something.
>>>
>>> If we keep rq->data_len == sum(sg), we need to fix the device
>>> application layer. If we keep rq->data_len == the true data length, we
>>> need to fix the low-level drivers.
>>>
>>> Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
>>> since we are in -rc stages. But I plan to send a patch to revert it
>>> and fix this issue in the block layer. I'd like to test it in -mm for
>>> a while.
>> No this commit is a serious bug, and the only fix is like you suggested
>> in __end_that_request_first. This is because it breaks that scsi-ml loop
>> where scsi_bufflen() can be less then blk_rq_bytes(). In that case this 
>> commit is a data corruption.
>>
>>> Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
>>> know, we really want to remove it.
>>>
>>>
>>>> Things going the other way is fine with me but I at least want to hear a
>>>> valid rationale.  Till now all I got is "because that's the true size"
>>>> which doesn't really make much sense to me.
>>> Most of users of request structure care about only the real data
>>> length, don't care about padding and drain length. Why do they bother
>>> to use a helper function to get the real data length?
>>> --
>> Submitted is the right fix to this problem, as pointed out by TOMO.
>> Please test it solves the CD burning problem.
>> (The patch includes the revert of commit e97a294e)
>> ---
>> From: Boaz Harrosh <bharrosh@panasas.com>
>> Date: Wed, 5 Mar 2008 12:07:12 +0200
>> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
>>
>> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
>> because scsi-ml supports the ability to split a request into smaller chunks,
>> in which case scsi_bufflen() is smaller then request length. Then at completion
>> time the remainder can be issued as a new scsi command. In that case the above
>> commit is a data corruption.
> 
> We needed something for -rc4, so it had to be rushed a bit...
> 
>> Also in this fix all users of block layer are taken care of, and not only
>> scsi devices.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  block/blk-core.c    |    4 ++++
>>  drivers/scsi/scsi.c |    2 +-
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 2a438a9..37fcccc 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>>  			     nr_bytes >> 9, req->sector);
>>  	}
>>  
>> +	if (nr_bytes >= blk_rq_bytes(req))
>> +		nr_bytes += req->extra_len;
>> +
>>  	total_bytes = bio_nbytes = 0;
>>  	while ((bio = req->bio) != NULL) {
>>  		int nbytes;
>> @@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
>>  	if (!req->bio)
>>  		return 0;
>>  
>> +	BUG_ON(total_bytes >= blk_rq_bytes(req));
> 
> Make that a WARN_ON() first please. It's indeed a bug, but it wont be
> critical and it's not fair killing everything since this padding stuff
> is so fresh and may still need a tweak or two.
> 
> I'd be fine with making it a BUG_ON() post 2.6.25.
> 
Updated, you are absolutely right, thanks.

Will you commit below patch for 2.6.25? I know that, at the time, I have 
seen this scsi-ml-loop in action on a sata drive here in the lab, on an 
x86_64 machine. The current solution will silently corrupt data, which 
is very hard to find.

Boaz
---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Wed, 5 Mar 2008 12:07:12 +0200
Subject: [PATCH] blk: missing add of padded bytes to io completion byte count

the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
because scsi-ml supports the ability to split a request into smaller chunks,
in which case scsi_bufflen() is smaller then request length. Then at completion
time the remainder can be issued as a new scsi command. In that case the above
commit is a data corruption.

Also in this fix all users of block layer are taken care of, and not only
scsi devices.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 block/blk-core.c    |    4 ++++
 drivers/scsi/scsi.c |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..c82e68a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
 			     nr_bytes >> 9, req->sector);
 	}
 
+	if (nr_bytes >= blk_rq_bytes(req))
+		nr_bytes += req->extra_len;
+
 	total_bytes = bio_nbytes = 0;
 	while ((bio = req->bio) != NULL) {
 		int nbytes;
@@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
 	if (!req->bio)
 		return 0;
 
+	WARN_ON(total_bytes >= blk_rq_bytes(req));
 	/*
 	 * if the request wasn't completed, update state
 	 */
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..fecba05 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
-- 
1.5.3.3

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Jens Axboe]

On Wed, Mar 05 2008, Boaz Harrosh wrote:
> On Wed, Mar 05 2008 at 14:33 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Wed, Mar 05 2008, Boaz Harrosh wrote:
> >> On Wed, Mar 05 2008 at 2:26 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >>> On Wed, 05 Mar 2008 08:33:05 +0900
> >>> Tejun Heo <htejun@gmail.com> wrote:
> >>>
> >>>> FUJITA Tomonori wrote:
> >>>>> Hmm, does SCSI mid-layer need to care about how many bytes the block
> >>>>> layer allocates? I don't think that extra_len is NOT good_bytes.
> >>>>>
> >>>>> I think that the block layer had better take care about it (fix
> >>>>> __end_that_request_first?).
> >>>> Yeah, probably calling completion functions w/o bytes count is the right
> >>>> thing to do but what I was talking about was what could break when the
> >>>> semantics of rq->data_len changed.  If we keep rq->data_len() ==
> >>>> sum(sg), we keep it business as usual for all the rest except for the
> >>>> device application layer if we don't we do the reverse and SCSI midlayer
> >>>> completion was a good example, I think.
> >>> sglist is a low-level I/O representation for device drivers. SCSI
> >>> midlayer should not care about sglist. We should not fix SCSI midlayer
> >>> for rq->data_len != sum(sg) change (so I can't agree with your
> >>> diagrams in another mail).
> >>>
> >>> When if we change a rule, we need to fix something.
> >>>
> >>> If we keep rq->data_len == sum(sg), we need to fix the device
> >>> application layer. If we keep rq->data_len == the true data length, we
> >>> need to fix the low-level drivers.
> >>>
> >>> Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
> >>> since we are in -rc stages. But I plan to send a patch to revert it
> >>> and fix this issue in the block layer. I'd like to test it in -mm for
> >>> a while.
> >> No this commit is a serious bug, and the only fix is like you suggested
> >> in __end_that_request_first. This is because it breaks that scsi-ml loop
> >> where scsi_bufflen() can be less then blk_rq_bytes(). In that case this 
> >> commit is a data corruption.
> >>
> >>> Only sglist stuff in SCSI midlayer is scsi_req_map_sg now. As you
> >>> know, we really want to remove it.
> >>>
> >>>
> >>>> Things going the other way is fine with me but I at least want to hear a
> >>>> valid rationale.  Till now all I got is "because that's the true size"
> >>>> which doesn't really make much sense to me.
> >>> Most of users of request structure care about only the real data
> >>> length, don't care about padding and drain length. Why do they bother
> >>> to use a helper function to get the real data length?
> >>> --
> >> Submitted is the right fix to this problem, as pointed out by TOMO.
> >> Please test it solves the CD burning problem.
> >> (The patch includes the revert of commit e97a294e)
> >> ---
> >> From: Boaz Harrosh <bharrosh@panasas.com>
> >> Date: Wed, 5 Mar 2008 12:07:12 +0200
> >> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
> >>
> >> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
> >> because scsi-ml supports the ability to split a request into smaller chunks,
> >> in which case scsi_bufflen() is smaller then request length. Then at completion
> >> time the remainder can be issued as a new scsi command. In that case the above
> >> commit is a data corruption.
> > 
> > We needed something for -rc4, so it had to be rushed a bit...
> > 
> >> Also in this fix all users of block layer are taken care of, and not only
> >> scsi devices.
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >> ---
> >>  block/blk-core.c    |    4 ++++
> >>  drivers/scsi/scsi.c |    2 +-
> >>  2 files changed, 5 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index 2a438a9..37fcccc 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
> >>  			     nr_bytes >> 9, req->sector);
> >>  	}
> >>  
> >> +	if (nr_bytes >= blk_rq_bytes(req))
> >> +		nr_bytes += req->extra_len;
> >> +
> >>  	total_bytes = bio_nbytes = 0;
> >>  	while ((bio = req->bio) != NULL) {
> >>  		int nbytes;
> >> @@ -1616,6 +1619,7 @@ static int __end_that_request_first(struct request *req, int error,
> >>  	if (!req->bio)
> >>  		return 0;
> >>  
> >> +	BUG_ON(total_bytes >= blk_rq_bytes(req));
> > 
> > Make that a WARN_ON() first please. It's indeed a bug, but it wont be
> > critical and it's not fair killing everything since this padding stuff
> > is so fresh and may still need a tweak or two.
> > 
> > I'd be fine with making it a BUG_ON() post 2.6.25.
> > 
> Updated, you are absolutely right, thanks.
> 
> Will you commit below patch for 2.6.25? I know that, at the time, I have 
> seen this scsi-ml-loop in action on a sata drive here in the lab, on an 
> x86_64 machine. The current solution will silently corrupt data, which 
> is very hard to find.

Yes, was just hoping you'd resend with the above corrected, so thanks!
I'll add it to the pending queue for 2.6.25.

-- 
Jens Axboe

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Tejun Heo]

Hello, Jens, Boaz.

Jens Axboe wrote:
>>>> From: Boaz Harrosh <bharrosh@panasas.com>
>>>> Date: Wed, 5 Mar 2008 12:07:12 +0200
>>>> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
>>>>
>>>> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
>>>> because scsi-ml supports the ability to split a request into smaller chunks,
>>>> in which case scsi_bufflen() is smaller then request length. Then at completion
>>>> time the remainder can be issued as a new scsi command. In that case the above
>>>> commit is a data corruption.

Thanks for catching the stupidity.  Did it actually happen?  PC commands
are not completed in pieces and padding / draining should only happen
for those.  qc->extra_len should be zero where commands can be splitted
for all current cases.

>>> We needed something for -rc4, so it had to be rushed a bit...
>>>
>>>> Also in this fix all users of block layer are taken care of, and not only
>>>> scsi devices.
>>>>
>>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>> ---
>>>>  block/blk-core.c    |    4 ++++
>>>>  drivers/scsi/scsi.c |    2 +-
>>>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 2a438a9..37fcccc 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>>>>  			     nr_bytes >> 9, req->sector);
>>>>  	}
>>>>  
>>>> +	if (nr_bytes >= blk_rq_bytes(req))
>>>> +		nr_bytes += req->extra_len;
>>>> +

This is getting insanely subtle.  Let's say there's PIO driver which
transfer certain sized chunks at a time and completes request partially
after completing each chunk and the driver uses draining to eat up
whatever excess data, which seems like a legit use case to me.  But it
won't work because __end_that_request_first() will terminate when it
reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,

Nacked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Jens Axboe]

On Wed, Mar 05 2008, Tejun Heo wrote:
> Hello, Jens, Boaz.
> 
> Jens Axboe wrote:
> >>>> From: Boaz Harrosh <bharrosh@panasas.com>
> >>>> Date: Wed, 5 Mar 2008 12:07:12 +0200
> >>>> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
> >>>>
> >>>> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
> >>>> because scsi-ml supports the ability to split a request into smaller chunks,
> >>>> in which case scsi_bufflen() is smaller then request length. Then at completion
> >>>> time the remainder can be issued as a new scsi command. In that case the above
> >>>> commit is a data corruption.
> 
> Thanks for catching the stupidity.  Did it actually happen?  PC commands
> are not completed in pieces and padding / draining should only happen
> for those.  qc->extra_len should be zero where commands can be splitted
> for all current cases.
> 
> >>> We needed something for -rc4, so it had to be rushed a bit...
> >>>
> >>>> Also in this fix all users of block layer are taken care of, and not only
> >>>> scsi devices.
> >>>>
> >>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >>>> ---
> >>>>  block/blk-core.c    |    4 ++++
> >>>>  drivers/scsi/scsi.c |    2 +-
> >>>>  2 files changed, 5 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>> index 2a438a9..37fcccc 100644
> >>>> --- a/block/blk-core.c
> >>>> +++ b/block/blk-core.c
> >>>> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
> >>>>  			     nr_bytes >> 9, req->sector);
> >>>>  	}
> >>>>  
> >>>> +	if (nr_bytes >= blk_rq_bytes(req))
> >>>> +		nr_bytes += req->extra_len;
> >>>> +
> 
> This is getting insanely subtle.  Let's say there's PIO driver which
> transfer certain sized chunks at a time and completes request partially
> after completing each chunk and the driver uses draining to eat up
> whatever excess data, which seems like a legit use case to me.  But it
> won't work because __end_that_request_first() will terminate when it
> reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> 
> Nacked-by: Tejun Heo <htejun@gmail.com>

Yeah, I think I may have gone a bit overboard in applying this so
quickly. It's just not a good interface, silently adding the extra
length if asked to complete more. It may even happen right now, for a
driver that does no padding (it probably wont do any harm here either,
but still).

I'll try and see if I can come up with something cleaner.

My basic design paradigm for this is that the _driver_ (or mid layer, if
SCSI wants to handle it) should care about the padding. So make it easy
for them to pad, but have it 'unrolled' by completion time. We should
NOT need any extra_len checks or additions in the block/ directory,
period.

-- 
Jens Axboe

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Tejun Heo]

Hello, Jens.

Jens Axboe wrote:
>> This is getting insanely subtle.  Let's say there's PIO driver which
>> transfer certain sized chunks at a time and completes request partially
>> after completing each chunk and the driver uses draining to eat up
>> whatever excess data, which seems like a legit use case to me.  But it
>> won't work because __end_that_request_first() will terminate when it
>> reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
>>
>> Nacked-by: Tejun Heo <htejun@gmail.com>
> 
> Yeah, I think I may have gone a bit overboard in applying this so
> quickly. It's just not a good interface, silently adding the extra
> length if asked to complete more. It may even happen right now, for a
> driver that does no padding (it probably wont do any harm here either,
> but still).

Unless it explicitly requests padding, it shouldn't be a problem
extra_len will always be zero and currently the only driver which uses
padding and draining is libata.

> I'll try and see if I can come up with something cleaner.
> 
> My basic design paradigm for this is that the _driver_ (or mid layer, if
> SCSI wants to handle it) should care about the padding. So make it easy
> for them to pad, but have it 'unrolled' by completion time. We should
> NOT need any extra_len checks or additions in the block/ directory,
> period.

Maybe I'm from Mars but I don't really understand all this fuss.  The
two patches I posted way back work perfectly fine and don't have any of
these problems and as I have said again and again that's because it
doesn't break the assumption which our internal mechanics depend on.

Can you please put the "true" size aside for a while and consider those
patches?  There's nothing fundamentally wrong with letting the
rq->data_len be sum(sg) which can differ from user requested data length
if and only if low level driver requests so.

If you can come up with something nicer, that will be great too but I
really don't think the current scheme will work.

Thanks.

-- 
tejun

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[James Bottomley]

On Wed, 2008-03-05 at 14:51 +0100, Jens Axboe wrote:
> On Wed, Mar 05 2008, Tejun Heo wrote:
> > This is getting insanely subtle.  Let's say there's PIO driver which
> > transfer certain sized chunks at a time and completes request partially
> > after completing each chunk and the driver uses draining to eat up
> > whatever excess data, which seems like a legit use case to me.  But it
> > won't work because __end_that_request_first() will terminate when it
> > reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> > 
> > Nacked-by: Tejun Heo <htejun@gmail.com>
> 
> Yeah, I think I may have gone a bit overboard in applying this so
> quickly. It's just not a good interface, silently adding the extra
> length if asked to complete more. It may even happen right now, for a
> driver that does no padding (it probably wont do any harm here either,
> but still).
> 
> I'll try and see if I can come up with something cleaner.
> 
> My basic design paradigm for this is that the _driver_ (or mid layer, if
> SCSI wants to handle it) should care about the padding. So make it easy
> for them to pad, but have it 'unrolled' by completion time. We should
> NOT need any extra_len checks or additions in the block/ directory,
> period.

Right, that's why my original proposal was to do nothing for padding
(other than ensure the driver could adjust the length if it wanted to)
and to add an extra element always for draining, which the driver could
ignore.  It basically pushed the use paradigm onto the driver.

If we want the use paradigm shared between block and driver, then I
think the best approach is to keep all the bios the same (so not adjust
for padding), but do adjust in the blk_rq_map_sg().  That way we have
the padding and draining unwind information by comparing with the bio.

For passing on to the driver:  req->data_len still needs to be the input
(bio) lenght.  req->extra_len can record how much padding and draining
was added.  The completion length also needs to be in terms of the true
(bio) length.  Now, here's the subtlety.  Because of the way transfers
work, we expect the padded length not to contribute to overrun (because
it represents transfers that were successfully completed at the correct
length), but we *do* expect drain usage to be recorded as overrun.
However, if we keep the bios intact, we have all the information to make
this determination in the block layer at completion time, with the
expectation that the lower layers report the exact amount they
transferred.

James

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[FUJITA Tomonori]

On Wed, 05 Mar 2008 09:21:24 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2008-03-05 at 14:51 +0100, Jens Axboe wrote:
> > On Wed, Mar 05 2008, Tejun Heo wrote:
> > > This is getting insanely subtle.  Let's say there's PIO driver which
> > > transfer certain sized chunks at a time and completes request partially
> > > after completing each chunk and the driver uses draining to eat up
> > > whatever excess data, which seems like a legit use case to me.  But it
> > > won't work because __end_that_request_first() will terminate when it
> > > reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> > > 
> > > Nacked-by: Tejun Heo <htejun@gmail.com>
> > 
> > Yeah, I think I may have gone a bit overboard in applying this so
> > quickly. It's just not a good interface, silently adding the extra
> > length if asked to complete more. It may even happen right now, for a
> > driver that does no padding (it probably wont do any harm here either,
> > but still).
> > 
> > I'll try and see if I can come up with something cleaner.
> > 
> > My basic design paradigm for this is that the _driver_ (or mid layer, if
> > SCSI wants to handle it) should care about the padding. So make it easy
> > for them to pad, but have it 'unrolled' by completion time. We should
> > NOT need any extra_len checks or additions in the block/ directory,
> > period.
> 
> Right, that's why my original proposal was to do nothing for padding
> (other than ensure the driver could adjust the length if it wanted to)
> and to add an extra element always for draining, which the driver could
> ignore.  It basically pushed the use paradigm onto the driver.
> 
> If we want the use paradigm shared between block and driver, then I
> think the best approach is to keep all the bios the same (so not adjust
> for padding), but do adjust in the blk_rq_map_sg().  That way we have
> the padding and draining unwind information by comparing with the bio.

Adjusting only sg in blk_rq_map_sg (like drain) looks much
better. This works with libata for me.

diff --git a/block/blk-map.c b/block/blk-map.c
index c07d9c8..e949969 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -140,26 +140,6 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 		ubuf += ret;
 	}
 
-	/*
-	 * __blk_rq_map_user() copies the buffers if starting address
-	 * or length isn't aligned to dma_pad_mask.  As the copied
-	 * buffer is always page aligned, we know that there's enough
-	 * room for padding.  Extend the last bio and update
-	 * rq->data_len accordingly.
-	 *
-	 * On unmap, bio_uncopy_user() will use unmodified
-	 * bio_map_data pointed to by bio->bi_private.
-	 */
-	if (len & q->dma_pad_mask) {
-		unsigned int pad_len = (q->dma_pad_mask & ~len) + 1;
-		struct bio *tail = rq->biotail;
-
-		tail->bi_io_vec[tail->bi_vcnt - 1].bv_len += pad_len;
-		tail->bi_size += pad_len;
-
-		rq->extra_len += pad_len;
-	}
-
 	rq->buffer = rq->data = NULL;
 	return 0;
 unmap_rq:
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 0f58616..2a81c87 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -220,6 +220,13 @@ new_segment:
 		bvprv = bvec;
 	} /* segments in rq */
 
+	if (sg && (q->dma_pad_mask & rq->data_len)) {
+		unsigned int pad_len = (q->dma_pad_mask & ~rq->data_len) + 1;
+
+		sg->length += pad_len;
+		rq->extra_len += pad_len;
+	}
+
 	if (q->dma_drain_size && q->dma_drain_needed(rq)) {
 		if (rq->cmd_flags & REQ_RW)
 			memset(q->dma_drain_buffer, 0, q->dma_drain_size);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..fecba05 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Jens Axboe]

On Thu, Mar 06 2008, FUJITA Tomonori wrote:
> On Wed, 05 Mar 2008 09:21:24 -0600
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2008-03-05 at 14:51 +0100, Jens Axboe wrote:
> > > On Wed, Mar 05 2008, Tejun Heo wrote:
> > > > This is getting insanely subtle.  Let's say there's PIO driver which
> > > > transfer certain sized chunks at a time and completes request partially
> > > > after completing each chunk and the driver uses draining to eat up
> > > > whatever excess data, which seems like a legit use case to me.  But it
> > > > won't work because __end_that_request_first() will terminate when it
> > > > reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> > > > 
> > > > Nacked-by: Tejun Heo <htejun@gmail.com>
> > > 
> > > Yeah, I think I may have gone a bit overboard in applying this so
> > > quickly. It's just not a good interface, silently adding the extra
> > > length if asked to complete more. It may even happen right now, for a
> > > driver that does no padding (it probably wont do any harm here either,
> > > but still).
> > > 
> > > I'll try and see if I can come up with something cleaner.
> > > 
> > > My basic design paradigm for this is that the _driver_ (or mid layer, if
> > > SCSI wants to handle it) should care about the padding. So make it easy
> > > for them to pad, but have it 'unrolled' by completion time. We should
> > > NOT need any extra_len checks or additions in the block/ directory,
> > > period.
> > 
> > Right, that's why my original proposal was to do nothing for padding
> > (other than ensure the driver could adjust the length if it wanted to)
> > and to add an extra element always for draining, which the driver could
> > ignore.  It basically pushed the use paradigm onto the driver.
> > 
> > If we want the use paradigm shared between block and driver, then I
> > think the best approach is to keep all the bios the same (so not adjust
> > for padding), but do adjust in the blk_rq_map_sg().  That way we have
> > the padding and draining unwind information by comparing with the bio.
> 
> Adjusting only sg in blk_rq_map_sg (like drain) looks much
> better. This works with libata for me.

Looks like a much better solution to me. Anyone have any valid
objections against moving the padding to the sg map time?

-- 
Jens Axboe

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Tejun Heo]

Jens Axboe wrote:
>>> If we want the use paradigm shared between block and driver, then I
>>> think the best approach is to keep all the bios the same (so not adjust
>>> for padding), but do adjust in the blk_rq_map_sg().  That way we have
>>> the padding and draining unwind information by comparing with the bio.
>> Adjusting only sg in blk_rq_map_sg (like drain) looks much
>> better. This works with libata for me.
> 
> Looks like a much better solution to me. Anyone have any valid
> objections against moving the padding to the sg map time?

Not necessarily objections but some concerns.

* As completion is done in bio terms, it makes completion from LLDs a
bit cumbersome, but this is unavoidable if we break sum(bio) == sum(sg).

* I've been wondering why we are not using sg chain / table or whatever
directly in bios and maybe rq_map_sg can go away in future.

How about separating out the padding / draining adjustment into a
separate interface?  Say, blk_rq_apply_extra() and blk_rq_undo_extra()
and make it the responsibility of the LLD which requested
padding/draining to apply and undo the adjustments?  It can undo the
adjustments when it returns the the request to its upper layer.  If rq
completion is handled by upper layer, it will do the right thing.  If rq
completion is handled by LLD, it can see the bio it wants to see.

Thanks.

-- 
tejun

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[FUJITA Tomonori]

On Fri, 07 Mar 2008 09:07:23 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Jens Axboe wrote:
> >>> If we want the use paradigm shared between block and driver, then I
> >>> think the best approach is to keep all the bios the same (so not adjust
> >>> for padding), but do adjust in the blk_rq_map_sg().  That way we have
> >>> the padding and draining unwind information by comparing with the bio.
> >> Adjusting only sg in blk_rq_map_sg (like drain) looks much
> >> better. This works with libata for me.
> > 
> > Looks like a much better solution to me. Anyone have any valid
> > objections against moving the padding to the sg map time?
> 
> Not necessarily objections but some concerns.
> 
> * As completion is done in bio terms, it makes completion from LLDs a
> bit cumbersome, but this is unavoidable if we break sum(bio) == sum(sg).

What do you mean? How does sub(bio) affect LLDs?


> * I've been wondering why we are not using sg chain / table or whatever
> directly in bios and maybe rq_map_sg can go away in future.

You mean that LLDs use bios directly? For me, sg and bio have very
different objectives and it's a clean layer separation.


> How about separating out the padding / draining adjustment into a
> separate interface?  Say, blk_rq_apply_extra() and blk_rq_undo_extra()
> and make it the responsibility of the LLD which requested
> padding/draining to apply and undo the adjustments?  It can undo the
> adjustments when it returns the the request to its upper layer.  If rq
> completion is handled by upper layer, it will do the right thing.  If rq
> completion is handled by LLD, it can see the bio it wants to see.

If possible, I'd like to avoid creating APIs for them. I think that
the current approach is much better than such APIs.

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Tejun Heo]

FUJITA Tomonori wrote:
> On Fri, 07 Mar 2008 09:07:23 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> Jens Axboe wrote:
>>>>> If we want the use paradigm shared between block and driver, then I
>>>>> think the best approach is to keep all the bios the same (so not adjust
>>>>> for padding), but do adjust in the blk_rq_map_sg().  That way we have
>>>>> the padding and draining unwind information by comparing with the bio.
>>>> Adjusting only sg in blk_rq_map_sg (like drain) looks much
>>>> better. This works with libata for me.
>>> Looks like a much better solution to me. Anyone have any valid
>>> objections against moving the padding to the sg map time?
>> Not necessarily objections but some concerns.
>>
>> * As completion is done in bio terms, it makes completion from LLDs a
>> bit cumbersome, but this is unavoidable if we break sum(bio) == sum(sg).
> 
> What do you mean? How does sub(bio) affect LLDs?

LLDs which loop over sg's trying to complete rq incrementally will see
rq going away sooner than it expected.

>> * I've been wondering why we are not using sg chain / table or whatever
>> directly in bios and maybe rq_map_sg can go away in future.
> 
> You mean that LLDs use bios directly? For me, sg and bio have very
> different objectives and it's a clean layer separation.

Actually the other way, block layer use sg instead of bio_vec in bio.
Layer separation doesn't necessarily require copying about the same
information to differently formatted data structure.  I'm not sure it
will be a clean win tho.  Requests hang longer in scheduler queue and
and bio_vec is smaller and scatterlist.

The thing is that, to me, blk_rq_map_sg() doesn't really look necessary,
it can be done just as well when the request is fetched from the queue
by block driver. (continued below...)

>> How about separating out the padding / draining adjustment into a
>> separate interface?  Say, blk_rq_apply_extra() and blk_rq_undo_extra()
>> and make it the responsibility of the LLD which requested
>> padding/draining to apply and undo the adjustments?  It can undo the
>> adjustments when it returns the the request to its upper layer.  If rq
>> completion is handled by upper layer, it will do the right thing.  If rq
>> completion is handled by LLD, it can see the bio it wants to see.
> 
> If possible, I'd like to avoid creating APIs for them. I think that
> the current approach is much better than such APIs.

And, so, I'm not too sure whether putting more mechanisms into it is a
good idea.

Thanks.

-- 
tejun

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] blk: missing add of padded bytes to io completion byte   count
Date: Thu, 6 Mar 2008 14:41:39 +0100

> On Thu, Mar 06 2008, FUJITA Tomonori wrote:
> > On Wed, 05 Mar 2008 09:21:24 -0600
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > On Wed, 2008-03-05 at 14:51 +0100, Jens Axboe wrote:
> > > > On Wed, Mar 05 2008, Tejun Heo wrote:
> > > > > This is getting insanely subtle.  Let's say there's PIO driver which
> > > > > transfer certain sized chunks at a time and completes request partially
> > > > > after completing each chunk and the driver uses draining to eat up
> > > > > whatever excess data, which seems like a legit use case to me.  But it
> > > > > won't work because __end_that_request_first() will terminate when it
> > > > > reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> > > > > 
> > > > > Nacked-by: Tejun Heo <htejun@gmail.com>
> > > > 
> > > > Yeah, I think I may have gone a bit overboard in applying this so
> > > > quickly. It's just not a good interface, silently adding the extra
> > > > length if asked to complete more. It may even happen right now, for a
> > > > driver that does no padding (it probably wont do any harm here either,
> > > > but still).
> > > > 
> > > > I'll try and see if I can come up with something cleaner.
> > > > 
> > > > My basic design paradigm for this is that the _driver_ (or mid layer, if
> > > > SCSI wants to handle it) should care about the padding. So make it easy
> > > > for them to pad, but have it 'unrolled' by completion time. We should
> > > > NOT need any extra_len checks or additions in the block/ directory,
> > > > period.
> > > 
> > > Right, that's why my original proposal was to do nothing for padding
> > > (other than ensure the driver could adjust the length if it wanted to)
> > > and to add an extra element always for draining, which the driver could
> > > ignore.  It basically pushed the use paradigm onto the driver.
> > > 
> > > If we want the use paradigm shared between block and driver, then I
> > > think the best approach is to keep all the bios the same (so not adjust
> > > for padding), but do adjust in the blk_rq_map_sg().  That way we have
> > > the padding and draining unwind information by comparing with the bio.
> > 
> > Adjusting only sg in blk_rq_map_sg (like drain) looks much
> > better. This works with libata for me.
> 
> Looks like a much better solution to me. Anyone have any valid
> objections against moving the padding to the sg map time?

What's the situation with this fix?

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Boaz Harrosh]

On Wed, Mar 05 2008 at 15:45 +0200, Tejun Heo <htejun@gmail.com> wrote:
> Hello, Jens, Boaz.
> 
> Jens Axboe wrote:
>>>>> From: Boaz Harrosh <bharrosh@panasas.com>
>>>>> Date: Wed, 5 Mar 2008 12:07:12 +0200
>>>>> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
>>>>>
>>>>> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
>>>>> because scsi-ml supports the ability to split a request into smaller chunks,
>>>>> in which case scsi_bufflen() is smaller then request length. Then at completion
>>>>> time the remainder can be issued as a new scsi command. In that case the above
>>>>> commit is a data corruption.
> 
> Thanks for catching the stupidity.  Did it actually happen?  PC commands
> are not completed in pieces and padding / draining should only happen
> for those.  qc->extra_len should be zero where commands can be splitted
> for all current cases.

So qc->extra_len == 0 and nothing is done in that case.

> 
>>>> We needed something for -rc4, so it had to be rushed a bit...
>>>>
>>>>> Also in this fix all users of block layer are taken care of, and not only
>>>>> scsi devices.
>>>>>
>>>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>>> ---
>>>>>  block/blk-core.c    |    4 ++++
>>>>>  drivers/scsi/scsi.c |    2 +-
>>>>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>> index 2a438a9..37fcccc 100644
>>>>> --- a/block/blk-core.c
>>>>> +++ b/block/blk-core.c
>>>>> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>>>>>  			     nr_bytes >> 9, req->sector);
>>>>>  	}
>>>>>  
>>>>> +	if (nr_bytes >= blk_rq_bytes(req))
>>>>> +		nr_bytes += req->extra_len;
>>>>> +
> 
> This is getting insanely subtle.  Let's say there's PIO driver which
> transfer certain sized chunks at a time and completes request partially
> after completing each chunk and the driver uses draining to eat up
> whatever excess data, which seems like a legit use case to me.  But it
> won't work because __end_that_request_first() will terminate when it
> reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
> 
> Nacked-by: Tejun Heo <htejun@gmail.com>
> 

I don't understand? Drivers can still do that. Do you mean That it wants
to also complete the draining portion in smaller chunks? I thought the draining
is always done at once, at most. Is that theoretical or is it so in any of the 
drivers.

Any way Nack from my side on the scsi_finish_command(), it makes too many 
assumptions that are unchecked anywhere. And it's a terrible layering violation.
scsi is a pass-threw block device, the fix should be in block or in using device
drivers (eg libata), that know what is going on.

Any way you are always saying req->data_len == sum(sg) but that  was certainly
never true for scsi_bufflen() == sum(sg) so leave that alone please.

Any other block layer fixes are welcome. But for now this is the best fix we have
that only breaks theoretical, yet to be submitted drivers.

Boaz

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[Tejun Heo]

Hello, Boaz.

Boaz Harrosh wrote:
>> This is getting insanely subtle.  Let's say there's PIO driver which
>> transfer certain sized chunks at a time and completes request partially
>> after completing each chunk and the driver uses draining to eat up
>> whatever excess data, which seems like a legit use case to me.  But it
>> won't work because __end_that_request_first() will terminate when it
>> reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,
>>
>> Nacked-by: Tejun Heo <htejun@gmail.com>
>>
> 
> I don't understand? Drivers can still do that. Do you mean That it wants
> to also complete the draining portion in smaller chunks? I thought the draining
> is always done at once, at most. Is that theoretical or is it so in any of the 
> drivers.

Ah... I wasn't really Nacking your patch specifically.  I was trying to
say "this scheme isn't gonna work".  Your patch does make good sense
given the situation (and I think I did acknowledge that above).  Sorry
about the miscommunication.

> Any way Nack from my side on the scsi_finish_command(), it makes too many 
> assumptions that are unchecked anywhere. And it's a terrible layering violation.
> scsi is a pass-threw block device, the fix should be in block or in using device
> drivers (eg libata), that know what is going on.

Yeap, completely agreed.  That one gets my big Nack-You-Idiot.

> Any way you are always saying req->data_len == sum(sg) but that  was certainly
> never true for scsi_bufflen() == sum(sg) so leave that alone please.

I don't really care about scsi_bufflen() and I'm not willing to change
any of that.  If SCSI LLDs are happy with scsi_bufflen() != sum(sg), no
problem at all.  What I'm against is pushing that into block layer,
which until now had "true" size == rq->data_len == sum(sg).

We're about to break one of the two equals if we're gonna do sg
manipulation in block layer (Jens seems to be planning something
different) and all I'm saying is we're far better off breaking the
former one.

First, I don't really think SCSI LLDs will make much use of explicit
padding or draining.  Secondly, even when such need arises, keeping
scsi_bufflen() at the "true" size is easy no matter which way we go with
rq->data_len.

Anyways, let's wait and see what Jens comes up with.

> Any other block layer fixes are welcome. But for now this is the best fix we have
> that only breaks theoretical, yet to be submitted drivers.

Yeap, given the current code, I agree.

Thanks.

-- 
tejun

Re: [PATCH] blk: missing add of padded bytes to io completion byte count

[FUJITA Tomonori]

On Wed, 05 Mar 2008 12:16:15 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Wed, Mar 05 2008 at 2:26 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Wed, 05 Mar 2008 08:33:05 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>> Hmm, does SCSI mid-layer need to care about how many bytes the block
> >>> layer allocates? I don't think that extra_len is NOT good_bytes.
> >>>
> >>> I think that the block layer had better take care about it (fix
> >>> __end_that_request_first?).
> >> Yeah, probably calling completion functions w/o bytes count is the right
> >> thing to do but what I was talking about was what could break when the
> >> semantics of rq->data_len changed.  If we keep rq->data_len() ==
> >> sum(sg), we keep it business as usual for all the rest except for the
> >> device application layer if we don't we do the reverse and SCSI midlayer
> >> completion was a good example, I think.
> > 
> > sglist is a low-level I/O representation for device drivers. SCSI
> > midlayer should not care about sglist. We should not fix SCSI midlayer
> > for rq->data_len != sum(sg) change (so I can't agree with your
> > diagrams in another mail).
> > 
> > When if we change a rule, we need to fix something.
> > 
> > If we keep rq->data_len == sum(sg), we need to fix the device
> > application layer. If we keep rq->data_len == the true data length, we
> > need to fix the low-level drivers.
> > 
> > Now I'm fine with the commit e97a294ef6938512b655b1abf17656cf2b26f709
> > since we are in -rc stages. But I plan to send a patch to revert it
> > and fix this issue in the block layer. I'd like to test it in -mm for
> > a while.
> 
> No this commit is a serious bug, and the only fix is like you suggested
> in __end_that_request_first. This is because it breaks that scsi-ml loop
> where scsi_bufflen() can be less then blk_rq_bytes(). In that case this 
> commit is a data corruption.

Ah, I knew that the patch doesn't work with partial completion but I
thought that it doesn't happen with PC commands... And touching
__end_that_request_first looked really hacky so I didn't send such
patch.

Moving the padding adjustment to blk_rq_map_sg (James' proposal) looks
fine. Maybe Jens will come up with something better.

Re: [PATCH] block: fix residual byte count handling

[Tejun Heo]

Hello, Jens.

Jens Axboe wrote:
> I completely agree with you, ->data_len meaning true data length is way
> cleaner imho. Only the driver should care for the padded length, all
> other parts of the kernel only need to know what they actually got.

Oh well, I guess I'm the one with strange taste he
re.  My logic is that the only thing below the block layer is the driver
which requested size adjustment.  This means residual bytes calculation
is pushed to low level drivers which isn't anything major but still.
Anyways, I'll review FUJITA's modified patch.

Thanks.

-- 
tejun