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

[Tejun Heo <htejun@gmail.com>]

Hello, Boaz.

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

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

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

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

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

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

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

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

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

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

Yeap, given the current code, I agree.

Thanks.

-- 
tejun