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@xxxxxxxxxxxxxxxxxxxx> 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
