Re: [PATCH 5/8] IB/hfi1: make hfi1_write_iter() deal with ITER_UBUF iov_iter

[Jens Axboe <axboe@kernel.dk>]

On 3/28/23 3:38 PM, Jens Axboe wrote:
> On 3/28/23 3:21?PM, Jens Axboe wrote:
>> On 3/28/23 1:16?PM, Linus Torvalds wrote:
>>> On Tue, Mar 28, 2023 at 12:05?PM Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>>
>>>> But it's not like adding a 'struct iovec' explicitly to the members
>>>> just as extra "code documentation" would be wrong.
>>>>
>>>> I don't think it really helps, though, since you have to have that
>>>> other explicit structure there anyway to get the member names right.
>>>
>>> Actually, thinking a bit more about it, adding a
>>>
>>>     const struct iovec xyzzy;
>>>
>>> member might be a good idea just to avoid a cast. Then that
>>> iter_ubuf_to_iov() macro becomes just
>>>
>>>    #define iter_ubuf_to_iov(iter) (&(iter)->xyzzy)
>>>
>>> and that looks much nicer (plus still acts kind of as a "code comment"
>>> to clarify things).
>>
>> I went down this path, and it _mostly_ worked out. You can view the
>> series here, I'll send it out when I've actually tested it:
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=iter-ubuf
>>
>> A few mental notes I made along the way:
>>
>> - The IB/sound changes are now just replacing an inappropriate
>>   iter_is_iovec() with iter->user_backed. That's nice and simple.
>>
>> - The iov_iter_iovec() case becomes a bit simpler. Or so I thought,
>>   because we still need to add in the offset so we can't just use
>>   out embedded iovec for that. The above branch is just using the
>>   iovec, but I don't think this is right.
>>
>> - Looks like it exposed a block bug, where the copy in
>>   bio_alloc_map_data() was obvious garbage but happened to work
>>   before.
>>
>> I'm still inclined to favor this approach over the previous, even if the
>> IB driver is a pile of garbage and lighting it a bit more on fire would
>> not really hurt.
>>
>> Opinions? Or do you want me to just send it out for easier reading
> 
> While cleaning up that stuff, we only have a few users of iov_iter_iovec().
> Why don't we just kill them off and the helper too? That drops that
> part of it and it kind of works out nicely beyond that.

Ugh that won't work obviously, as we can't factor in per-vec
offsets... So it has to be a copy.

-- 
Jens Axboe