[PATCHSET,v6,0/11] Turn single segment imports into ITER_UBUF

Message

Jens Axboe March 29, 2023, 6:40 p.m. UTC

Hi,

Rather than repeat the same blurb again, see the v2 posting here:

https://lore.kernel.org/linux-fsdevel/[e-mail]/

tldr - turn single segment iovecs into ITER_UBUF rather than ITER_IOVEC,
because they are more efficient.

Attempt 2 at doing the overlay. The series starts by adding an
iter_iov() helper, which simply returns iter->iov. At the same time we
rename it, to catch anyone using it and to further signify that future
direct uses of it should be discouraged. There are a few manual bits
left, I'll clean those up if we agree this is moving in the right
direction.

Then we get rid of returning an iovec copy with iov_iter_iovec(), and
killing off that function. Two helpers are added to return the current
segment address and length.

Then the usual few iter_is_iovec() -> iter->user_backed changes. For
the alsa part, Takashi did say that single segments could be valid.
But with the rest of the changes, this is no longer interesting as we
don't have to deal with it separately.

Finally, do the last two patches that turn single iovec segments into
ITER_UBUF at import time.

Passes testing, and verified we do the right thing for 1 and multi
segments.

Also viewable here:

https://git.kernel.dk/cgit/linux-block/log/?h=iter-ubuf.2

Comments

Linus Torvalds March 29, 2023, 7:44 p.m. UTC | #1

On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <[e-mail]> wrote:
>
> Passes testing, and verified we do the right thing for 1 and multi
> segments.

Apart from the pointer casting rant, this looks sane to me.

I feel like 02/11 has a few potential cleanups:

 (a) it feels like a few too many "iter.__iov" uses remaining, but
they mostly (all?) look like assignments.

I do get the feeling that any time you assign __iov, you should also
assign "nr_segs", and it worries me a bit that I see one without the
other. Maybe room for another helper that enforces a "if you set the
__iov pointer, you must be setting nr_segs too"?

And maybe I'm just being difficult.

 (b) I see at least one "iov = iter_iov(from)" that precedes a later
check for "iter_is_iovec()", which again means that *if* we add some
debug sanity test to "iter_iov()", it might trigger when it shouldn't?

The one I see is in snd_pcm_writev(), but I th ink the same thing
happens in snd_pcm_readv() but just isn't visible in the patch due to
not having the context lines.

            Linus

Jens Axboe March 29, 2023, 7:55 p.m. UTC | #2

On 3/29/23 1:44 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <[e-mail]> wrote:
>>
>> Passes testing, and verified we do the right thing for 1 and multi
>> segments.
> 
> Apart from the pointer casting rant, this looks sane to me.
> 
> I feel like 02/11 has a few potential cleanups:
> 
>  (a) it feels like a few too many "iter.__iov" uses remaining, but
> they mostly (all?) look like assignments.
> 
> I do get the feeling that any time you assign __iov, you should also
> assign "nr_segs", and it worries me a bit that I see one without the
> other. Maybe room for another helper that enforces a "if you set the
> __iov pointer, you must be setting nr_segs too"?
> 
> And maybe I'm just being difficult.

No, I think that's valid, and the cover letter does touch upon that.
The thought of doing an iov assign helper has occurred to me as well.
I just wanted to get general feelings on the direction first, then
do a round of polish when prudent rather than prematurely.

>  (b) I see at least one "iov = iter_iov(from)" that precedes a later
> check for "iter_is_iovec()", which again means that *if* we add some
> debug sanity test to "iter_iov()", it might trigger when it shouldn't?
> 
> The one I see is in snd_pcm_writev(), but I th ink the same thing
> happens in snd_pcm_readv() but just isn't visible in the patch due to
> not having the context lines.

I think that's mostly a patch ordering issue. Should probably just
push the sound and IB patches to the front of the series.