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 12:43?PM, Linus Torvalds wrote:
> On Tue, Mar 28, 2023 at 10:36?AM Jens Axboe <axboe@kernel.dk> 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