Re: [PATCH RESEND number 2] libata: eliminate the home grown dma padding in favour of that provided by the block layer

Tejun Heo
2008-02-05T00:43:08Z

Hello,

James Bottomley wrote:
>> No, it doesn't. Drain needs to extend the sg table too
>
> The patches do extend the sg table.

Hmm... Where?

>> and it makes controllers lax about buffer overruns for commands they shouldn't be.
>
> So adjust the qc->nbytes to show no drain element in libata ... although
> the extra element gets mapped, the HBA will be none the wiser if there's
> zero length allocated to it. Thus the entire system behaves exactly
> like the drain element isn't present. The block layer is really just
> making the drain element available ... you don't have to use it.

Adjusting qc->nbytes isn't enough. libata will have to trim at the end
of sglist. With both draining and padding, it means libata will have to
trim one sg entry and a few more bytes. Block layer is behaving
differently and in a very noticeable way, it's sending inconsistent
values in data_len and sg list.

>> Gee.. What's the deal with extra four bytes? If you're worried about
>> the size of struct request, chopping off PC request part into a separate
>> struct would be far better strategy than making what's already confusing
>> more confusing.
>
> Well, we are trying to slim down the request structure and scsi_cmnd
> structure. However, there's no inherent advantage to counting this all
> in the block layer. Think of it just as a transparent passthrough.
> You've told the block layer to make sure you have room for a drain
> element when it constructs the SG list, which it does. Whether you use
> it (by adding blk_rq_drain_size() on to the actual length you hand down)
> or not is entirely up to you ... there's no need to push the decision on
> that one into the block layer, since use or not is pretty much a cost
> free thing.

Okay, I see your point. The biggest problem is that by doing that blk
layer breaks a pretty important rule - keeping data len consistent with
sg list and the bugs it's gonna cause will be very subtle and dangerous.
This is definitely where we can spend four extra bytes.

Thanks.

--
tejun