* [PATCH block#for-2.6.31 3/3] bio: always copy back data for copied kernel requests
[not found] ` <4A0D87D2.7090806@gmail.com>
@ 2009-05-15 15:19 ` Tejun Heo
2009-05-15 15:38 ` [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue Sergei Shtylyov
` (4 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-15 15:19 UTC (permalink / raw)
To: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel, linux-s
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);
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0D87D2.7090806@gmail.com>
2009-05-15 15:19 ` [PATCH block#for-2.6.31 3/3] bio: always copy back data for copied kernel requests Tejun Heo
@ 2009-05-15 15:38 ` Sergei Shtylyov
2009-05-15 22:18 ` Tejun Heo
2009-05-15 17:29 ` Pete Zaitcev
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Sergei Shtylyov @ 2009-05-15 15:38 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-15 15:38 ` [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue Sergei Shtylyov
@ 2009-05-15 22:18 ` Tejun Heo
2009-05-16 12:29 ` Sergei Shtylyov
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2009-05-15 22:18 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-15 22:18 ` Tejun Heo
@ 2009-05-16 12:29 ` Sergei Shtylyov
2009-05-16 13:48 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Sergei Shtylyov @ 2009-05-16 12:29 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-16 12:29 ` Sergei Shtylyov
@ 2009-05-16 13:48 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-16 13:48 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0D87D2.7090806@gmail.com>
2009-05-15 15:19 ` [PATCH block#for-2.6.31 3/3] bio: always copy back data for copied kernel requests Tejun Heo
2009-05-15 15:38 ` [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue Sergei Shtylyov
@ 2009-05-15 17:29 ` Pete Zaitcev
2009-05-15 22:14 ` Tejun Heo
2009-05-16 0:18 ` [PATCH UPDATED " Tejun Heo
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Pete Zaitcev @ 2009-05-15 17:29 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Sergei Shtylyov, Eric Moore, Darrick J. Wong,
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-15 17:29 ` Pete Zaitcev
@ 2009-05-15 22:14 ` Tejun Heo
2009-05-15 23:16 ` Pete Zaitcev
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2009-05-15 22:14 UTC (permalink / raw)
To: Pete Zaitcev
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Sergei Shtylyov, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-15 22:14 ` Tejun Heo
@ 2009-05-15 23:16 ` Pete Zaitcev
2009-05-16 0:14 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Pete Zaitcev @ 2009-05-15 23:16 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Sergei Shtylyov, Eric Moore, Darrick J. Wong,
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-15 23:16 ` Pete Zaitcev
@ 2009-05-16 0:14 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-16 0:14 UTC (permalink / raw)
To: Pete Zaitcev
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Sergei Shtylyov, Eric Moore, Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0D87D2.7090806@gmail.com>
` (2 preceding siblings ...)
2009-05-15 17:29 ` Pete Zaitcev
@ 2009-05-16 0:18 ` Tejun Heo
2009-05-16 7:13 ` [PATCH " Borislav Petkov
[not found] ` <4A0E0650.1020500@gmail.com>
5 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-16 0:18 UTC (permalink / raw)
To: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel, linux-s
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) {
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0D87D2.7090806@gmail.com>
` (3 preceding siblings ...)
2009-05-16 0:18 ` [PATCH UPDATED " Tejun Heo
@ 2009-05-16 7:13 ` Borislav Petkov
2009-05-16 13:52 ` Tejun Heo
[not found] ` <4A0E0650.1020500@gmail.com>
5 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2009-05-16 7:13 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel,
linux-scsi, IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-16 7:13 ` [PATCH " Borislav Petkov
@ 2009-05-16 13:52 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-16 13:52 UTC (permalink / raw)
To: Borislav Petkov, Tejun Heo, Jens Axboe, James Bottomley,
Boaz Harrosh <bharrosh>
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
^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <4A0E0650.1020500@gmail.com>]
* Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0E0650.1020500@gmail.com>
@ 2009-05-17 8:48 ` Boaz Harrosh
2009-05-17 11:32 ` Tejun Heo
2009-05-17 12:05 ` [PATCH UPDATED2 " Tejun Heo
1 sibling, 1 reply; 22+ messages in thread
From: Boaz Harrosh @ 2009-05-17 8:48 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, James Bottomley, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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) {
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-17 8:48 ` [PATCH UPDATED " Boaz Harrosh
@ 2009-05-17 11:32 ` Tejun Heo
2009-05-17 11:41 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2009-05-17 11:32 UTC (permalink / raw)
To: Boaz Harrosh
Cc: Jens Axboe, James Bottomley, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-17 11:32 ` Tejun Heo
@ 2009-05-17 11:41 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2009-05-17 11:41 UTC (permalink / raw)
To: Boaz Harrosh
Cc: Jens Axboe, James Bottomley, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
[not found] ` <4A0E0650.1020500@gmail.com>
2009-05-17 8:48 ` [PATCH UPDATED " Boaz Harrosh
@ 2009-05-17 12:05 ` Tejun Heo
2009-05-18 12:49 ` Jens Axboe
1 sibling, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2009-05-17 12:05 UTC (permalink / raw)
To: Jens Axboe, James Bottomley, Boaz Harrosh, Linux Kernel, linux-s
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) {
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-17 12:05 ` [PATCH UPDATED2 " Tejun Heo
@ 2009-05-18 12:49 ` Jens Axboe
2009-05-19 9:14 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2009-05-18 12:49 UTC (permalink / raw)
To: Tejun Heo
Cc: James Bottomley, Boaz Harrosh, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-18 12:49 ` Jens Axboe
@ 2009-05-19 9:14 ` Tejun Heo
2009-05-19 9:17 ` Jens Axboe
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2009-05-19 9:14 UTC (permalink / raw)
To: Jens Axboe
Cc: James Bottomley, Boaz Harrosh, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue
2009-05-19 9:14 ` Tejun Heo
@ 2009-05-19 9:17 ` Jens Axboe
0 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2009-05-19 9:17 UTC (permalink / raw)
To: Tejun Heo
Cc: James Bottomley, Boaz Harrosh, Linux Kernel, linux-scsi,
IDE/ATA development list, Bartlomiej Zolnierkiewicz,
Borislav Petkov, Pete Zaitcev, Sergei Shtylyov, Eric Moore,
Darrick J. Wong
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
^ permalink raw reply [flat|nested] 22+ messages in thread