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

[Tejun Heo <htejun@gmail.com>]

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