On Thu, Jun 17, 2021 at 4:51 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote:
>
> This is essentially copy_struct_from_user() but for an iov_iter.
So I continue to think that this series looks fine - if we want this
interface at all.
I do note a few issues with this iov patch, though - partly probably
because I have been reading Al's cleanup patches that had some
optimizations in place.
And in particular, I now react to this:
> + iov_iter_advance(i, usize);
at the end of copy_struct_from_iter().
It's very wasteful to use the generic iov_iter_advance() function,
when you just had special functions for each of the iterator cases.
Because that generic function will now just end up re-testing that
whole "what kind was it" and then do each kind separately.
So it would actually be a lot simpler and m,ore efficient to just do
that "advance" part as you go through the cases, iow just do
iov_iter_iovec_advance(i, usize);
at the end of the iter_is_iovec/iter_is_kvec cases, and
iov_iter_bvec_advance(i, usize)
for the bvec case.
I think that you may need it to be based on Al's series for that to
work, which might be inconvenient, though.
One other non-code issue: particularly since you only handle a subset
of the iov_iter cases, it would be nice to have an explanation for
_why_ those particular cases.
IOW, have some trivial explanation for each of the cases. "iovec" is
for regular read/write, what triggers the kvec and bvec cases?
But also, the other way around. Why doesn't the pipe case trigger? No
splice support?
Linus
