[PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
rq->resid_len, the default value of residue count was changed from
full count to zero.  The conversion was done under the assumption that
when a request fails residue count wasn't defined.  However, Boaz and
James pointed out that this wasn't true and the residue count should
be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len
to blk_rq_bytes(rq) on issue and restoring explicit clearing in
affected drivers.  While at it, take advantage of the fact that
rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
  advantage of initial full count to simplify code

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Borislav Petkov <petkovbb@googlemail.com>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Eric Moore <Eric.Moore@lsi.com>
Cc: Darrick J. Wong <djwong@us.ibm.com>
---
 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    3 +--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 13 insertions(+), 11 deletions(-)

Index: block/block/blk-core.c
===================================================================
--- block.orig/block/blk-core.c
+++ block/block/blk-core.c
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
Index: block/drivers/ide/ide-cd.c
===================================================================
--- block.orig/drivers/ide/ide-cd.c
+++ block/drivers/ide/ide-cd.c
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {
@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
Index: block/drivers/ide/ide-tape.c
===================================================================
--- block.orig/drivers/ide/ide-tape.c
+++ block/drivers/ide/ide-tape.c
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
Index: block/drivers/message/fusion/mptsas.c
===================================================================
--- block.orig/drivers/message/fusion/mptsas.c
+++ block/drivers/message/fusion/mptsas.c
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_expander.c
+++ block/drivers/scsi/libsas/sas_expander.c
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_host_smp.c
+++ block/drivers/scsi/libsas/sas_host_smp.c
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
--- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
+++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
Index: block/drivers/block/ub.c
===================================================================
--- block.orig/drivers/block/ub.c
+++ block/drivers/block/ub.c
@@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			rq->resid_len -= min(cmd->act_len, rq->resid_len);
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {

[PATCH block#for-2.6.31 3/3] bio: always copy back data for copied kernel requests

[Tejun Heo]

When a read bio_copy_kern() request fails, the content of the bounce
buffer is not copied back.  However, as request failure doesn't
necessarily mean complete failure, the buffer state can be useful.
This behavior is also inconsistent with the user map counterpart and
causes the subtle difference between bounced and unbounced IO causes
confusion.

This patch makes bio_copy_kern_endio() ignore @err and always copy
back data on request completion.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/bio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: block/fs/bio.c
===================================================================
--- block.orig/fs/bio.c
+++ block/fs/bio.c
@@ -1198,7 +1198,7 @@ static void bio_copy_kern_endio(struct b
 		char *addr = page_address(bvec->bv_page);
 		int len = bmd->iovecs[i].bv_len;
 
-		if (read && !err)
+		if (read)
 			memcpy(p, addr, len);
 
 		__free_page(bvec->bv_page);

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
> rq->resid_len, the default value of residue count was changed from
> full count to zero.  The conversion was done under the assumption that
> when a request fails residue count wasn't defined.  However, Boaz and
> James pointed out that this wasn't true and the residue count should
> be preserved for failed requests too.

> This patchset restores the original behavior by setting rq->resid_len
> to blk_rq_bytes(rq) on issue and restoring explicit clearing in
> affected drivers.  While at it, take advantage of the fact that
> rq->resid_len is set to full count where applicable.

> * ide-cd: rq->resid_len cleared on pc success

> * mptsas: req->resid_len cleared on success

> * sas_expander: rsp/req->resid_len cleared on success

> * mpt2sas_transport: req->resid_len cleared on success

> * ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
>   advantage of initial full count to simplify code

> Signed-off-by: Tejun Heo <tj@kernel.org>

    The patch looks er... strange at some places.

> Index: block/drivers/message/fusion/mptsas.c
> ===================================================================
> --- block.orig/drivers/message/fusion/mptsas.c
> +++ block/drivers/message/fusion/mptsas.c
> @@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
>  		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
>  		memcpy(req->sense, smprep, sizeof(*smprep));
>  		req->sense_len = sizeof(*smprep);
> -		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= smprep->ResponseDataLength;

    Is negative resid_len intended here? If so, shouldn't it be simply:

		rsp->resid_len = -smprep->ResponseDataLength;


> Index: block/drivers/scsi/libsas/sas_expander.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_expander.c
> +++ block/drivers/scsi/libsas/sas_expander.c
> @@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
>  	if (ret > 0) {
>  		/* positive number is the untransferred residual */
>  		rsp->resid_len = ret;
> +		req->resid_len = 0;
>  		ret = 0;
> +	} else if (ret == 0) {
> +		rsp->resid_len = 0;
> +		req->resid_len = 0;

    ???

> Index: block/drivers/scsi/libsas/sas_host_smp.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_host_smp.c
> +++ block/drivers/scsi/libsas/sas_host_smp.c
> @@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
>  	resp_data[1] = req_data[1];
>  	resp_data[2] = SMP_RESP_FUNC_UNK;
>  
> -	req->resid_len = blk_rq_bytes(req);
> -	rsp->resid_len = blk_rq_bytes(rsp);
> -

    ???

>  	switch (req_data[1]) {
>  	case SMP_REPORT_GENERAL:
>  		req->resid_len -= 8;
> Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> ===================================================================
> --- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
> +++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> @@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
>  
>  		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
>  		req->sense_len = sizeof(*mpi_reply);
> -		rsp->resid_len = blk_rq_bytes(rsp) -
> -				 mpi_reply->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= mpi_reply->ResponseDataLength;

    Again, is negative resid_len intended?

MBR, Sergei

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Pete Zaitcev]

On Sat, 16 May 2009 00:18:42 +0900, Tejun Heo <htejun@gmail.com> wrote:

> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
> rq->resid_len, the default value of residue count was changed from
> full count to zero. []

So it's not a residue anymore, right? You should've renamed it to
rq->count or something, then. Now we have this:

> +++ block/drivers/block/ub.c
> @@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
>  
>  	if (cmd->error == 0) {
>  		if (blk_pc_request(rq)) {
> -			if (cmd->act_len < blk_rq_bytes(rq))
> -				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
> +			rq->resid_len -= min(cmd->act_len, rq->resid_len);
>  			scsi_status = 0;

You are subtracting resid_len from itself. Just how in the world
can this be correct?

Even it if is, in fact, correct, it's such an eggregious violation
of good style, that your good programmer's card is going to lose
a big coupon and have a hole punched in it.

This is not in Linus' tree yet, but I'm going to take a hard look
at this once it shows up.

-- Pete

Re: [PATCH block#for-2.6.31 1/3] ub: use __blk_end_request_all()

[Pete Zaitcev]

On Sat, 16 May 2009 00:14:35 +0900, Tejun Heo <tj@kernel.org> wrote:

> @@ -834,12 +829,7 @@ static void ub_end_rq(struct request *rq
>  		error = -EIO;
>  		rq->errors = scsi_status;
>  	}
> -	rqlen = blk_rq_bytes(rq);    /* Oddly enough, this is the residue. */
> -	if (__blk_end_request(rq, error, cmd_len)) {
> -		printk(KERN_WARNING DRV_NAME
> -		    ": __blk_end_request blew, %s-cmd total %u rqlen %ld\n",
> -		    blk_pc_request(rq)? "pc": "fs", cmd_len, rqlen);
> -	}
> +	__blk_end_request_all(rq, error);
>  }

I applaud this. We needed blk_end_this_mudafakin_request() for a long time.
Why two underscores?

-- Pete

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Pete Zaitcev wrote:
> On Sat, 16 May 2009 00:18:42 +0900, Tejun Heo <htejun@gmail.com> wrote:
> 
>> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
>> rq->resid_len, the default value of residue count was changed from
>> full count to zero. []
> 
> So it's not a residue anymore, right? You should've renamed it to
> rq->count or something, then. Now we have this:

It still is.  It just is restoring the original behavior.

>> +++ block/drivers/block/ub.c
>> @@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
>>  
>>  	if (cmd->error == 0) {
>>  		if (blk_pc_request(rq)) {
>> -			if (cmd->act_len < blk_rq_bytes(rq))
>> -				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
>> +			rq->resid_len -= min(cmd->act_len, rq->resid_len);
>>  			scsi_status = 0;
> 
> You are subtracting resid_len from itself. Just how in the world
> can this be correct?
>
> Even it if is, in fact, correct, it's such an eggregious violation
> of good style, that your good programmer's card is going to lose
> a big coupon and have a hole punched in it.

The original code was

 if (cmd->act_len >= rq->data_len)
	rq->data_len = 0;
 else
	rq->data_len -= cmd->act_len

So, I could have written

 if (cmd->act_len >= rq->resid_len)
	rq->resid_len = 0;
 else
	rq->resid_len -= cmd->act_len

Instead I wrote

 rq->resid_len -= min(cmd->act_len, rq->resid_len);

It's just capping the amount to be subtracted so that resid_len
doesn't underflow.  What is so wrong or bad style about that?

> This is not in Linus' tree yet, but I'm going to take a hard look
> at this once it shows up.

It would be great if you do before it hits Linus's tree.

Thanks.

-- 
tejun

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello,

Sergei Shtylyov wrote:
>> Index: block/drivers/message/fusion/mptsas.c
>> ===================================================================
>> --- block.orig/drivers/message/fusion/mptsas.c
>> +++ block/drivers/message/fusion/mptsas.c
>> @@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
>>          smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
>>          memcpy(req->sense, smprep, sizeof(*smprep));
>>          req->sense_len = sizeof(*smprep);
>> -        rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
>> +        req->resid_len = 0;
>> +        rsp->resid_len -= smprep->ResponseDataLength;
> 
>    Is negative resid_len intended here? If so, shouldn't it be simply:
> 
>         rsp->resid_len = -smprep->ResponseDataLength;

>From patch description.

 This patchset restores the original behavior by setting rq->resid_len
 to blk_rq_bytes(rq) on issue and restoring explicit clearing in
 affected drivers.

So, rsp->resid_len equals the initial request length before the
subtraction and after subtraction it becomes the residue count.  The
original code was

	req->data_len = 0;
	rsp->data_len -= smprep->ResponseDataLength;

>> Index: block/drivers/scsi/libsas/sas_expander.c
>> ===================================================================
>> --- block.orig/drivers/scsi/libsas/sas_expander.c
>> +++ block/drivers/scsi/libsas/sas_expander.c
>> @@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
>>      if (ret > 0) {
>>          /* positive number is the untransferred residual */
>>          rsp->resid_len = ret;
>> +        req->resid_len = 0;
>>          ret = 0;
>> +    } else if (ret == 0) {
>> +        rsp->resid_len = 0;
>> +        req->resid_len = 0;

Heh... there's a reason I mentioned the original commit.  The original
code was

	if (ret > 0) {
		/* positive number is the untransferred residual */
		rsp->data_len = ret;
		req->data_len = 0;
		ret = 0;
	} else if (ret == 0) {
		rsp->data_len = 0;
		req->data_len = 0;
	}

>> Index: block/drivers/scsi/libsas/sas_host_smp.c
>> ===================================================================
>> --- block.orig/drivers/scsi/libsas/sas_host_smp.c
>> +++ block/drivers/scsi/libsas/sas_host_smp.c
>> @@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
>>      resp_data[1] = req_data[1];
>>      resp_data[2] = SMP_RESP_FUNC_UNK;
>>  
>> -    req->resid_len = blk_rq_bytes(req);
>> -    rsp->resid_len = blk_rq_bytes(rsp);
>> -

Cuz it's already set to blk_rq_bytes().

>>      switch (req_data[1]) {
>>      case SMP_REPORT_GENERAL:
>>          req->resid_len -= 8;
>> Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
>> ===================================================================
>> --- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
>> +++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
>> @@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
>>  
>>          memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
>>          req->sense_len = sizeof(*mpi_reply);
>> -        rsp->resid_len = blk_rq_bytes(rsp) -
>> -                 mpi_reply->ResponseDataLength;
>> +        req->resid_len = 0;
>> +        rsp->resid_len -= mpi_reply->ResponseDataLength;
> 
>    Again, is negative resid_len intended?

Ditto.

Thanks.

-- 
tejun

Re: [PATCH block#for-2.6.31 1/3] ub: use __blk_end_request_all()

[Tejun Heo]

Pete Zaitcev wrote:
> On Sat, 16 May 2009 00:14:35 +0900, Tejun Heo <tj@kernel.org> wrote:
> 
>> @@ -834,12 +829,7 @@ static void ub_end_rq(struct request *rq
>>  		error = -EIO;
>>  		rq->errors = scsi_status;
>>  	}
>> -	rqlen = blk_rq_bytes(rq);    /* Oddly enough, this is the residue. */
>> -	if (__blk_end_request(rq, error, cmd_len)) {
>> -		printk(KERN_WARNING DRV_NAME
>> -		    ": __blk_end_request blew, %s-cmd total %u rqlen %ld\n",
>> -		    blk_pc_request(rq)? "pc": "fs", cmd_len, rqlen);
>> -	}
>> +	__blk_end_request_all(rq, error);
>>  }
> 
> I applaud this. We needed blk_end_this_mudafakin_request() for a long time.
> Why two underscores?

The ones with two underscores are to be called with queue lock held.
Ones without grab queue lock themselves.

Thanks.

-- 
tejun

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Pete Zaitcev]

On Sat, 16 May 2009 07:14:44 +0900, Tejun Heo <htejun@gmail.com> wrote:

> So, I could have written
> 
>  if (cmd->act_len >= rq->resid_len)
> 	rq->resid_len = 0;
>  else
> 	rq->resid_len -= cmd->act_len
> 
> Instead I wrote
> 
>  rq->resid_len -= min(cmd->act_len, rq->resid_len);
> 
> It's just capping the amount to be subtracted so that resid_len
> doesn't underflow.  What is so wrong or bad style about that?

Curse of the gifted, I guess. To use a subtraction instead of zero
this way looks like a pointless, even mischievous obfuscation to me.

Also, we probably want a stack_dump or a printk when actual length
exceeds the requested length, don't we? If it ever happens, we
might be overwriting some I/O buffer somewhere.

-- Pete

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello,

Pete Zaitcev wrote:
>> So, I could have written
>>
>>  if (cmd->act_len >= rq->resid_len)
>> 	rq->resid_len = 0;
>>  else
>> 	rq->resid_len -= cmd->act_len
>>
>> Instead I wrote
>>
>>  rq->resid_len -= min(cmd->act_len, rq->resid_len);
>>
>> It's just capping the amount to be subtracted so that resid_len
>> doesn't underflow.  What is so wrong or bad style about that?
> 
> Curse of the gifted, I guess. To use a subtraction instead of zero
> this way looks like a pointless, even mischievous obfuscation to me.

Ummm... I don't know.  I prefer min/max over if/else when capping
values.  To me, it makes the intention clearer but you're the
maintainer and don't like the style, so I'll update the patch so that
it has the if/else clause.  :-)

> Also, we probably want a stack_dump or a printk when actual length
> exceeds the requested length, don't we? If it ever happens, we
> might be overwriting some I/O buffer somewhere.

It depends on particular implementation.  Transport overflow doesn't
necessarily become actual buffer overflow depending on hardware and
driver implementation.  If you think the user needs to be warned about
transport overflow, please go ahead and add a warning there.

Thanks.

-- 
tejun

[PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
rq->resid_len, the default value of residue count was changed from
full count to zero.  The conversion was done under the assumption that
when a request fails residue count wasn't defined.  However, Boaz and
James pointed out that this wasn't true and the residue count should
be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len
to blk_rq_bytes(rq) on issue and restoring explicit clearing in
affected drivers.  While at it, take advantage of the fact that
rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
  advantage of initial full count to simplify code

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Borislav Petkov <petkovbb@googlemail.com>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Eric Moore <Eric.Moore@lsi.com>
Cc: Darrick J. Wong <djwong@us.ibm.com>
---
ub updated per Pete's comment.

Thanks.

 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 16 insertions(+), 11 deletions(-)

Index: block/block/blk-core.c
===================================================================
--- block.orig/block/blk-core.c
+++ block/block/blk-core.c
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
Index: block/drivers/ide/ide-cd.c
===================================================================
--- block.orig/drivers/ide/ide-cd.c
+++ block/drivers/ide/ide-cd.c
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {
@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
Index: block/drivers/ide/ide-tape.c
===================================================================
--- block.orig/drivers/ide/ide-tape.c
+++ block/drivers/ide/ide-tape.c
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
Index: block/drivers/message/fusion/mptsas.c
===================================================================
--- block.orig/drivers/message/fusion/mptsas.c
+++ block/drivers/message/fusion/mptsas.c
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_expander.c
+++ block/drivers/scsi/libsas/sas_expander.c
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_host_smp.c
+++ block/drivers/scsi/libsas/sas_host_smp.c
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
--- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
+++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
Index: block/drivers/block/ub.c
===================================================================
--- block.orig/drivers/block/ub.c
+++ block/drivers/block/ub.c
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Borislav Petkov]

On Sat, May 16, 2009 at 12:18:42AM +0900, Tejun Heo wrote:
> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
> rq->resid_len, the default value of residue count was changed from
> full count to zero.  The conversion was done under the assumption that
> when a request fails residue count wasn't defined.  However, Boaz and
> James pointed out that this wasn't true and the residue count should
> be preserved for failed requests too.
> 
> This patchset restores the original behavior by setting rq->resid_len
> to blk_rq_bytes(rq) on issue and restoring explicit clearing in
> affected drivers.  While at it, take advantage of the fact that
> rq->resid_len is set to full count where applicable.
> 
> * ide-cd: rq->resid_len cleared on pc success
> 
> * mptsas: req->resid_len cleared on success
> 
> * sas_expander: rsp/req->resid_len cleared on success
> 
> * mpt2sas_transport: req->resid_len cleared on success
> 
> * ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
>   advantage of initial full count to simplify code

ACK the ide-{cd,tape} bits.

Acked-by: Borislav Petkov <petkovbb@gmail.com>

I've converted the ide-atapi part to rq->resid_len and from what I see,
the changes are compatible with the latest semantics of rq->resid_len
but it won't hurt if you could take a look too :) :

http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commit;h=077e6dba20e74a455a0452379d2a965c7e1b01ad

On a side note, ide_pc_intr() does some transfer padding but that
doesn't affect rq->resid_len returning the remaining bytecount back up
to block layer.

[..]

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

>>> Index: block/drivers/message/fusion/mptsas.c
>>> ===================================================================
>>> --- block.orig/drivers/message/fusion/mptsas.c
>>> +++ block/drivers/message/fusion/mptsas.c
>>> @@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
>>>          smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
>>>          memcpy(req->sense, smprep, sizeof(*smprep));
>>>          req->sense_len = sizeof(*smprep);
>>> -        rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
>>> +        req->resid_len = 0;
>>> +        rsp->resid_len -= smprep->ResponseDataLength;
>>>       
>>    Is negative resid_len intended here? If so, shouldn't it be simply:
>>
>>         rsp->resid_len = -smprep->ResponseDataLength;
>>     
>
> From patch description.
>
>  This patchset restores the original behavior by setting rq->resid_len
>  to blk_rq_bytes(rq) on issue and restoring explicit clearing in
>  affected drivers.
>
> So, rsp->resid_len equals the initial request length before the
> subtraction and after subtraction it becomes the residue count.  The
> original code was
>
> 	req->data_len = 0;
> 	rsp->data_len -= smprep->ResponseDataLength;
>   

  Ah, I've confused up 'rsp' and 'req' as being one thing. Nevermind 
then. :-<

>>> Index: block/drivers/scsi/libsas/sas_expander.c
>>> ===================================================================
>>> --- block.orig/drivers/scsi/libsas/sas_expander.c
>>> +++ block/drivers/scsi/libsas/sas_expander.c
>>> @@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
>>>      if (ret > 0) {
>>>          /* positive number is the untransferred residual */
>>>          rsp->resid_len = ret;
>>> +        req->resid_len = 0;
>>>          ret = 0;
>>> +    } else if (ret == 0) {
>>> +        rsp->resid_len = 0;
>>> +        req->resid_len = 0;
>>>       
>
> Heh... there's a reason I mentioned the original commit.  The original
> code was
>
> 	if (ret > 0) {
> 		/* positive number is the untransferred residual */
> 		rsp->data_len = ret;
> 		req->data_len = 0;
> 		ret = 0;
> 	} else if (ret == 0) {
> 		rsp->data_len = 0;
> 		req->data_len = 0;
> 	}
>   

   But still,

                req->data_len = 0;

is common between both branches, so could be moved after the *if* statement.

>>> Index: block/drivers/scsi/libsas/sas_host_smp.c
>>> ===================================================================
>>> --- block.orig/drivers/scsi/libsas/sas_host_smp.c
>>> +++ block/drivers/scsi/libsas/sas_host_smp.c
>>> @@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
>>>      resp_data[1] = req_data[1];
>>>      resp_data[2] = SMP_RESP_FUNC_UNK;
>>>  
>>> -    req->resid_len = blk_rq_bytes(req);
>>> -    rsp->resid_len = blk_rq_bytes(rsp);
>>> -
>>>       
>
> Cuz it's already set to blk_rq_bytes().
>   

   Mixed them up again... :-<

>>>      switch (req_data[1]) {
>>>      case SMP_REPORT_GENERAL:
>>>          req->resid_len -= 8;
>>> Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
>>> ===================================================================
>>> --- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
>>> +++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
>>> @@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
>>>  
>>>          memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
>>>          req->sense_len = sizeof(*mpi_reply);
>>> -        rsp->resid_len = blk_rq_bytes(rsp) -
>>> -                 mpi_reply->ResponseDataLength;
>>> +        req->resid_len = 0;
>>> +        rsp->resid_len -= mpi_reply->ResponseDataLength;
>>>       
>>    Again, is negative resid_len intended?
>>     
>
> Ditto.
>   

   ... and again.

> Thanks.
>   

WBR, Sergei

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello, Sergei.

Sergei Shtylyov wrote:
>> Heh... there's a reason I mentioned the original commit.  The original
>> code was
>>
>>     if (ret > 0) {
>>         /* positive number is the untransferred residual */
>>         rsp->data_len = ret;
>>         req->data_len = 0;
>>         ret = 0;
>>     } else if (ret == 0) {
>>         rsp->data_len = 0;
>>         req->data_len = 0;
>>     }
>>   
> 
>   But still,
> 
>                req->data_len = 0;
> 
> is common between both branches, so could be moved after the *if*
> statement.

Yeah, sure, feel free to submit a patch, but I don't know.  Does it
even matter at all as long as the intention is clear?

Thanks.

-- 
tejun

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello, Borislav.

Borislav Petkov wrote:
> On Sat, May 16, 2009 at 12:18:42AM +0900, Tejun Heo wrote:
>> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
>> rq->resid_len, the default value of residue count was changed from
>> full count to zero.  The conversion was done under the assumption that
>> when a request fails residue count wasn't defined.  However, Boaz and
>> James pointed out that this wasn't true and the residue count should
>> be preserved for failed requests too.
>>
>> This patchset restores the original behavior by setting rq->resid_len
>> to blk_rq_bytes(rq) on issue and restoring explicit clearing in
>> affected drivers.  While at it, take advantage of the fact that
>> rq->resid_len is set to full count where applicable.
>>
>> * ide-cd: rq->resid_len cleared on pc success
>>
>> * mptsas: req->resid_len cleared on success
>>
>> * sas_expander: rsp/req->resid_len cleared on success
>>
>> * mpt2sas_transport: req->resid_len cleared on success
>>
>> * ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
>>   advantage of initial full count to simplify code
> 
> ACK the ide-{cd,tape} bits.
> 
> Acked-by: Borislav Petkov <petkovbb@gmail.com>
> 
> I've converted the ide-atapi part to rq->resid_len and from what I see,

Sorry it changed underneath your changes.

> the changes are compatible with the latest semantics of rq->resid_len
> but it won't hurt if you could take a look too :) :
>
> http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commit;h=077e6dba20e74a455a0452379d2a965c7e1b01ad

Yeap, I'll take a look.

> On a side note, ide_pc_intr() does some transfer padding but that
> doesn't affect rq->resid_len returning the remaining bytecount back up
> to block layer.

Hmmm... telling the issuer the actual number of transferred bytes sans
padding is the correct behavior, I think.

Thanks.

-- 
tejun

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Boaz Harrosh]

On 05/16/2009 03:18 AM, Tejun Heo wrote:
> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
> rq->resid_len, the default value of residue count was changed from
> full count to zero.  The conversion was done under the assumption that
> when a request fails residue count wasn't defined.  However, Boaz and
> James pointed out that this wasn't true and the residue count should
> be preserved for failed requests too.
> 
> This patchset restores the original behavior by setting rq->resid_len
> to blk_rq_bytes(rq) on issue and restoring explicit clearing in
> affected drivers.  While at it, take advantage of the fact that
> rq->resid_len is set to full count where applicable.
> 
> * ide-cd: rq->resid_len cleared on pc success
> 
> * mptsas: req->resid_len cleared on success
> 
> * sas_expander: rsp/req->resid_len cleared on success
> 
> * mpt2sas_transport: req->resid_len cleared on success
> 
> * ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
>   advantage of initial full count to simplify code
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Boaz Harrosh <bharrosh@panasas.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Pete Zaitcev <zaitcev@redhat.com>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Cc: Borislav Petkov <petkovbb@googlemail.com>
> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Cc: Eric Moore <Eric.Moore@lsi.com>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> ---
> ub updated per Pete's comment.
> 
> Thanks.
> 
>  block/blk-core.c                         |    1 +
>  drivers/block/ub.c                       |    6 ++++--
>  drivers/ide/ide-cd.c                     |    4 ++--
>  drivers/ide/ide-tape.c                   |    2 +-
>  drivers/message/fusion/mptsas.c          |    3 ++-
>  drivers/scsi/libsas/sas_expander.c       |    4 ++++
>  drivers/scsi/libsas/sas_host_smp.c       |    3 ---
>  drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
>  8 files changed, 16 insertions(+), 11 deletions(-)
> 
> Index: block/block/blk-core.c
> ===================================================================
> --- block.orig/block/blk-core.c
> +++ block/block/blk-core.c
> @@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
>  		rq->buffer = bio_data(bio);
>  	}
>  	rq->__data_len = bio->bi_size;
> +	rq->resid_len = bio->bi_size;
>  	rq->bio = rq->biotail = bio;
>  

Hi Tejun.
Surly this is not enough. This is not the only place rq->__data_len gets a value.
Specifically it can be added to in blk_rq_append_bio. You must then update
all places that set rq->__data_len to also set rq->resid_len.

Alternatively you could do a single:
+	rq->resid_len = blk_rq_bytes(rq);

inside  blk_fetch_request(q) or before the call to prep_fn, or even at
__elv_add_request().

Currently this is a bug with chained bios

Thanks
Boaz

>  	if (bio->bi_bdev)
> Index: block/drivers/ide/ide-cd.c
> ===================================================================
> --- block.orig/drivers/ide/ide-cd.c
> +++ block/drivers/ide/ide-cd.c
> @@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
>  
>  out_end:
>  	if (blk_pc_request(rq) && rc == 0) {
> +		rq->resid_len = 0;
>  		blk_end_request_all(rq, 0);
>  		hwif->rq = NULL;
>  	} else {
> @@ -718,8 +719,7 @@ out_end:
>  
>  		/* make sure it's fully ended */
>  		if (blk_fs_request(rq) == 0) {
> -			rq->resid_len = blk_rq_bytes(rq) -
> -				(cmd->nbytes - cmd->nleft);
> +			rq->resid_len -= cmd->nbytes - cmd->nleft;
>  			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
>  				rq->resid_len += cmd->last_xfer_len;
>  		}
> Index: block/drivers/ide/ide-tape.c
> ===================================================================
> --- block.orig/drivers/ide/ide-tape.c
> +++ block/drivers/ide/ide-tape.c
> @@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
>  		}
>  
>  		tape->first_frame += blocks;
> -		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
> +		rq->resid_len -= blocks * tape->blk_size;
>  
>  		if (pc->error) {
>  			uptodate = 0;
> Index: block/drivers/message/fusion/mptsas.c
> ===================================================================
> --- block.orig/drivers/message/fusion/mptsas.c
> +++ block/drivers/message/fusion/mptsas.c
> @@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
>  		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
>  		memcpy(req->sense, smprep, sizeof(*smprep));
>  		req->sense_len = sizeof(*smprep);
> -		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= smprep->ResponseDataLength;
>  	} else {
>  		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
>  		    ioc->name, __func__);
> Index: block/drivers/scsi/libsas/sas_expander.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_expander.c
> +++ block/drivers/scsi/libsas/sas_expander.c
> @@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
>  	if (ret > 0) {
>  		/* positive number is the untransferred residual */
>  		rsp->resid_len = ret;
> +		req->resid_len = 0;
>  		ret = 0;
> +	} else if (ret == 0) {
> +		rsp->resid_len = 0;
> +		req->resid_len = 0;
>  	}
>  
>  	return ret;
> Index: block/drivers/scsi/libsas/sas_host_smp.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_host_smp.c
> +++ block/drivers/scsi/libsas/sas_host_smp.c
> @@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
>  	resp_data[1] = req_data[1];
>  	resp_data[2] = SMP_RESP_FUNC_UNK;
>  
> -	req->resid_len = blk_rq_bytes(req);
> -	rsp->resid_len = blk_rq_bytes(rsp);
> -
>  	switch (req_data[1]) {
>  	case SMP_REPORT_GENERAL:
>  		req->resid_len -= 8;
> Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> ===================================================================
> --- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
> +++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> @@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
>  
>  		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
>  		req->sense_len = sizeof(*mpi_reply);
> -		rsp->resid_len = blk_rq_bytes(rsp) -
> -				 mpi_reply->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= mpi_reply->ResponseDataLength;
>  	} else {
>  		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
>  		    "%s - no reply\n", ioc->name, __func__));
> Index: block/drivers/block/ub.c
> ===================================================================
> --- block.orig/drivers/block/ub.c
> +++ block/drivers/block/ub.c
> @@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
>  
>  	if (cmd->error == 0) {
>  		if (blk_pc_request(rq)) {
> -			if (cmd->act_len < blk_rq_bytes(rq))
> -				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
> +			if (cmd->act_len >= rq->resid_len)
> +				rq->resid_len = 0;
> +			else
> +				rq->resid_len -= cmd->act_len;
>  			scsi_status = 0;
>  		} else {
>  			if (cmd->act_len != cmd->len) {

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello, Boaz.

Boaz Harrosh wrote:
> Surly this is not enough. This is not the only place rq->__data_len
> gets a value.  Specifically it can be added to in blk_rq_append_bio.
> You must then update all places that set rq->__data_len to also set
> rq->resid_len.

Right, nice spotting, but I think those are the only two places.  You
now see why I don't like that append thing.  :-)

> Alternatively you could do a single:
> +	rq->resid_len = blk_rq_bytes(rq);
> 
> inside  blk_fetch_request(q) or before the call to prep_fn, or even at
> __elv_add_request().

I think I'll just add resid_len adjustment there right after
__data_len manipulation.  That code path should really go away
eventually.

Thanks.

-- 
tejun

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Tejun Heo wrote:
> I think I'll just add resid_len adjustment there right after
> __data_len manipulation.  That code path should really go away
> eventually.

Aieee... crap, I also forgot the merging path.  I'll change as you
suggested.  Thanks.

-- 
tejun

[PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
rq->resid_len, the default value of residue count was changed from
full count to zero.  The conversion was done under the assumption that
when a request fails residue count wasn't defined.  However, Boaz and
James pointed out that this wasn't true and the residue count should
be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len
to blk_rq_bytes(rq) on request start and restoring explicit clearing
in affected drivers.  While at it, take advantage of the fact that
rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
  advantage of initial full count to simplify code

Boaz Harrosh spotted bug in resid_len initialization.  Fixed as
suggested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Borislav Petkov <petkovbb@googlemail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Eric Moore <Eric.Moore@lsi.com>
Cc: Darrick J. Wong <djwong@us.ibm.com>
---
Fixed as Boaz suggested.  Thanks Boaz.

 block/blk-core.c                         |    5 +++--
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 18 insertions(+), 13 deletions(-)

Index: block/block/blk-core.c
===================================================================
--- block.orig/block/blk-core.c
+++ block/block/blk-core.c
@@ -1783,9 +1783,10 @@ void blk_start_request(struct request *r
 	blk_dequeue_request(req);
 
 	/*
-	 * We are now handing the request to the hardware, add the
-	 * timeout handler.
+	 * We are now handing the request to the hardware, initialize
+	 * resid_len to full count and add the timeout handler.
 	 */
+	req->resid_len = blk_rq_bytes(req);
 	blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);
Index: block/drivers/ide/ide-cd.c
===================================================================
--- block.orig/drivers/ide/ide-cd.c
+++ block/drivers/ide/ide-cd.c
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {
@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
Index: block/drivers/ide/ide-tape.c
===================================================================
--- block.orig/drivers/ide/ide-tape.c
+++ block/drivers/ide/ide-tape.c
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
Index: block/drivers/message/fusion/mptsas.c
===================================================================
--- block.orig/drivers/message/fusion/mptsas.c
+++ block/drivers/message/fusion/mptsas.c
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_expander.c
+++ block/drivers/scsi/libsas/sas_expander.c
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
--- block.orig/drivers/scsi/libsas/sas_host_smp.c
+++ block/drivers/scsi/libsas/sas_host_smp.c
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
--- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
+++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
Index: block/drivers/block/ub.c
===================================================================
--- block.orig/drivers/block/ub.c
+++ block/drivers/block/ub.c
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Jens Axboe]

On Sun, May 17 2009, Tejun Heo wrote:
> In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing
> rq->resid_len, the default value of residue count was changed from
> full count to zero.  The conversion was done under the assumption that
> when a request fails residue count wasn't defined.  However, Boaz and
> James pointed out that this wasn't true and the residue count should
> be preserved for failed requests too.
> 
> This patchset restores the original behavior by setting rq->resid_len
> to blk_rq_bytes(rq) on request start and restoring explicit clearing
> in affected drivers.  While at it, take advantage of the fact that
> rq->resid_len is set to full count where applicable.
> 
> * ide-cd: rq->resid_len cleared on pc success
> 
> * mptsas: req->resid_len cleared on success
> 
> * sas_expander: rsp/req->resid_len cleared on success
> 
> * mpt2sas_transport: req->resid_len cleared on success
> 
> * ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take
>   advantage of initial full count to simplify code

Tejun, you have to stop replying with updated patches in an existing
thread. It's virtually impossible to sort the resulting mess into
patches worth applying.

If you update patches in a series, or post a new patch, do it in a new
thread. And use explicit naming (like "foo patchset version bar") so
it's easy to grasp the chronology of the patches. Otherwise I have to
spend lots of time extracting and comparing patches, which is just a
waste of time.

I haven't applied anything in this thread. Please repost the pending
patches so they can be reviewed and applied! Thanks.

> 
> Boaz Harrosh spotted bug in resid_len initialization.  Fixed as
> suggested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Acked-by: Borislav Petkov <petkovbb@googlemail.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Boaz Harrosh <bharrosh@panasas.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Pete Zaitcev <zaitcev@redhat.com>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Cc: Eric Moore <Eric.Moore@lsi.com>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> ---
> Fixed as Boaz suggested.  Thanks Boaz.
> 
>  block/blk-core.c                         |    5 +++--
>  drivers/block/ub.c                       |    6 ++++--
>  drivers/ide/ide-cd.c                     |    4 ++--
>  drivers/ide/ide-tape.c                   |    2 +-
>  drivers/message/fusion/mptsas.c          |    3 ++-
>  drivers/scsi/libsas/sas_expander.c       |    4 ++++
>  drivers/scsi/libsas/sas_host_smp.c       |    3 ---
>  drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
>  8 files changed, 18 insertions(+), 13 deletions(-)
> 
> Index: block/block/blk-core.c
> ===================================================================
> --- block.orig/block/blk-core.c
> +++ block/block/blk-core.c
> @@ -1783,9 +1783,10 @@ void blk_start_request(struct request *r
>  	blk_dequeue_request(req);
>  
>  	/*
> -	 * We are now handing the request to the hardware, add the
> -	 * timeout handler.
> +	 * We are now handing the request to the hardware, initialize
> +	 * resid_len to full count and add the timeout handler.
>  	 */
> +	req->resid_len = blk_rq_bytes(req);
>  	blk_add_timer(req);
>  }
>  EXPORT_SYMBOL(blk_start_request);
> Index: block/drivers/ide/ide-cd.c
> ===================================================================
> --- block.orig/drivers/ide/ide-cd.c
> +++ block/drivers/ide/ide-cd.c
> @@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
>  
>  out_end:
>  	if (blk_pc_request(rq) && rc == 0) {
> +		rq->resid_len = 0;
>  		blk_end_request_all(rq, 0);
>  		hwif->rq = NULL;
>  	} else {
> @@ -718,8 +719,7 @@ out_end:
>  
>  		/* make sure it's fully ended */
>  		if (blk_fs_request(rq) == 0) {
> -			rq->resid_len = blk_rq_bytes(rq) -
> -				(cmd->nbytes - cmd->nleft);
> +			rq->resid_len -= cmd->nbytes - cmd->nleft;
>  			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
>  				rq->resid_len += cmd->last_xfer_len;
>  		}
> Index: block/drivers/ide/ide-tape.c
> ===================================================================
> --- block.orig/drivers/ide/ide-tape.c
> +++ block/drivers/ide/ide-tape.c
> @@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
>  		}
>  
>  		tape->first_frame += blocks;
> -		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
> +		rq->resid_len -= blocks * tape->blk_size;
>  
>  		if (pc->error) {
>  			uptodate = 0;
> Index: block/drivers/message/fusion/mptsas.c
> ===================================================================
> --- block.orig/drivers/message/fusion/mptsas.c
> +++ block/drivers/message/fusion/mptsas.c
> @@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
>  		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
>  		memcpy(req->sense, smprep, sizeof(*smprep));
>  		req->sense_len = sizeof(*smprep);
> -		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= smprep->ResponseDataLength;
>  	} else {
>  		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
>  		    ioc->name, __func__);
> Index: block/drivers/scsi/libsas/sas_expander.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_expander.c
> +++ block/drivers/scsi/libsas/sas_expander.c
> @@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
>  	if (ret > 0) {
>  		/* positive number is the untransferred residual */
>  		rsp->resid_len = ret;
> +		req->resid_len = 0;
>  		ret = 0;
> +	} else if (ret == 0) {
> +		rsp->resid_len = 0;
> +		req->resid_len = 0;
>  	}
>  
>  	return ret;
> Index: block/drivers/scsi/libsas/sas_host_smp.c
> ===================================================================
> --- block.orig/drivers/scsi/libsas/sas_host_smp.c
> +++ block/drivers/scsi/libsas/sas_host_smp.c
> @@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
>  	resp_data[1] = req_data[1];
>  	resp_data[2] = SMP_RESP_FUNC_UNK;
>  
> -	req->resid_len = blk_rq_bytes(req);
> -	rsp->resid_len = blk_rq_bytes(rsp);
> -
>  	switch (req_data[1]) {
>  	case SMP_REPORT_GENERAL:
>  		req->resid_len -= 8;
> Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> ===================================================================
> --- block.orig/drivers/scsi/mpt2sas/mpt2sas_transport.c
> +++ block/drivers/scsi/mpt2sas/mpt2sas_transport.c
> @@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
>  
>  		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
>  		req->sense_len = sizeof(*mpi_reply);
> -		rsp->resid_len = blk_rq_bytes(rsp) -
> -				 mpi_reply->ResponseDataLength;
> +		req->resid_len = 0;
> +		rsp->resid_len -= mpi_reply->ResponseDataLength;
>  	} else {
>  		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
>  		    "%s - no reply\n", ioc->name, __func__));
> Index: block/drivers/block/ub.c
> ===================================================================
> --- block.orig/drivers/block/ub.c
> +++ block/drivers/block/ub.c
> @@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
>  
>  	if (cmd->error == 0) {
>  		if (blk_pc_request(rq)) {
> -			if (cmd->act_len < blk_rq_bytes(rq))
> -				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
> +			if (cmd->act_len >= rq->resid_len)
> +				rq->resid_len = 0;
> +			else
> +				rq->resid_len -= cmd->act_len;
>  			scsi_status = 0;
>  		} else {
>  			if (cmd->act_len != cmd->len) {

-- 
Jens Axboe

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Tejun Heo]

Hello, Jens.

Jens Axboe wrote:
> Tejun, you have to stop replying with updated patches in an existing
> thread. It's virtually impossible to sort the resulting mess into
> patches worth applying.

Heh.. yeah, I usually post minor updates as UPDATED patches for review
then repost the whole series after accumulating some of them.  This
patchset's size was on the borderline for proper patchset and I was
feeling lazy.  Sorry about that.  Will repost.

Thanks.

-- 
tejun

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

[Jens Axboe]

On Tue, May 19 2009, Tejun Heo wrote:
> Hello, Jens.
> 
> Jens Axboe wrote:
> > Tejun, you have to stop replying with updated patches in an existing
> > thread. It's virtually impossible to sort the resulting mess into
> > patches worth applying.
> 
> Heh.. yeah, I usually post minor updates as UPDATED patches for review
> then repost the whole series after accumulating some of them.  This
> patchset's size was on the borderline for proper patchset and I was
> feeling lazy.  Sorry about that.  Will repost.

Thanks, appreciated :-)

-- 
Jens Axboe