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

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

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

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

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

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

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


     \ /
      Last update: 2008-03-05 15:55    [W:0.025 / U:39.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site