On 3/28/23 12:43?PM, Linus Torvalds wrote:
> On Tue, Mar 28, 2023 at 10:36?AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>>
>> Don't assume that a user backed iterator is always of the type
>> ITER_IOVEC. Handle the single segment case separately, then we can
>> use the same logic for ITER_UBUF and ITER_IOVEC.
>
> Ugh. This is ugly.
>
> Yes,. the original code is ugly too, but this makes it worse.
Hah I know, I did feel dirty writing that patch... The existing code is
pretty ugly as-is, but it sure didn't get better.
> You have that helper for "give me the number of iovecs" and that just
> works automatically with the ITER_UBUF case. But this code (and the
> sound driver code in the previous patch), really lso wants a helper to
> just return the 'iov' array.
>
> And I think you should just do exactly that. The problem with
> 'iov_iter_iovec()' is that it doesn't return the array, it just
> returns the first entry, so it's unusable for this case, and then you
> have all these special "do something else for the single-entry
> situation" cases.
>
> And iov_iter_iovec() actually tries to be nice and clever and add the
> iov_offset, so that you can actually do the proper iov_iter_advance()
> on it etc, but again, this is not what any of this code wants, it just
> wants the raw iov array, and the base will always be zero, because
> this code just doesn't *work* on the iter level, and never advances
> the iterator, it just advances the array index.
>
> And the thing is, I think you could easily just add a
>
> const struct iovec *iov_iter_iovec_array(iter);
>
> helper that just always returns a valid array of iov's.
>
> For a ITER_IOV, it would just return the raw iov pointer.
>
> And for a ITER_UBUF, we could either
>
> (a) just always pass in a single-entry auto iov that gets filled in
> and the pointer to it returned
>
> (b) be *really* clever (or ugly, depending on how you want to see
> it), and do something like this:
>
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -49,14 +49,23 @@ struct iov_iter {
> size_t iov_offset;
> int last_offset;
> };
> - size_t count;
> - union {
> - const struct iovec *iov;
> - const struct kvec *kvec;
> - const struct bio_vec *bvec;
> - struct xarray *xarray;
> - struct pipe_inode_info *pipe;
> - void __user *ubuf;
> +
> + /*
> + * This has the same layout as 'struct iovec'!
> + * In particular, the ITER_UBUF form can create
> + * a single-entry 'struct iovec' by casting the
> + * address of the 'ubuf' member to that.
> + */
> + struct {
> + union {
> + const struct iovec *iov;
> + const struct kvec *kvec;
> + const struct bio_vec *bvec;
> + struct xarray *xarray;
> + struct pipe_inode_info *pipe;
> + void __user *ubuf;
> + };
> + size_t count;
> };
> union {
> unsigned long nr_segs;
>
> and if you accept the above, then you can do
>
> #define iter_ubuf_to_iov(iter) ((const struct iovec *)&(iter)->ubuf)
>
> which I will admit is not *pretty*, but it's kind of clever, I think.
>
> So now you can trivially turn a user-backed iov_iter into the related
> 'struct iovec *' by just doing
>
> #define iov_iter_iovec_array(iter) \
> ((iter)->type == ITER_UBUF ? iter_ubuf_to_iov(iter) : (iter)->iov)
>
> or something like that.
>
> And no, the above is NOT AT ALL TESTED. Caveat emptor.
>
> And if you go blind from looking at that patch, I will not accept
> responsibility.
I pondered something like that too, but balked at adding to iov_iter and
then didn't pursue that any further.
But bundled nicely, it should work out quite fine in the union. So I
like the suggestion, and then just return a pointer to the vec rather
than the copy, unifying the two cases.
Thanks for the review and suggestion, I'll make that change.
--
Jens Axboe
