Re: [PATCH 4/6] iomap: add struct iomap_ctx

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, Dec 17, 2019 at 6:40 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> We pass a lot of arguments to iomap_apply(), and subsequently to the
> actors that it calls. In preparation for adding one more argument,
> switch them to using a struct iomap_ctx instead. The actor gets a const
> version of that, they are not supposed to change anything in it.

Looks generally like what I expected, but when looking at the patch I
notice that the type of 'len' is crazy and wrong.

It was wrong before too, though:

> -dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,

'loff_t length' is not right.

> +       loff_t pos = data->pos;
> +       loff_t length = pos + data->len;

And WTH is that? "pos + data->len" is not "length", that's end. And this:

>         loff_t end = pos + length, done = 0;

What? Now 'end' is 'pos+length', which is 'pos+pos+data->len'.

WHAA?

> @@ -1197,22 +1200,26 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
>  {
> +       loff_t ret = 0, done = 0;

More insanity. "ret" shouldn't be loff_t.

dax_iomap_rw() returns a ssize_t.

> +       loff_t count = data->len;

More of this crazy things.

>  iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,

This was wrong before.

> +struct iomap_ctx {
> +       struct inode    *inode;
> +       loff_t          pos;
> +       loff_t          len;
> +       void            *priv;
> +       unsigned        flags;
> +};

Please make 'len' be 'size_t' or something.

If you're on a 32-bit architecture, you shouldn't be writing more than
4GB in a go anyway.

Is there some reason for this horrible case of "let's allow 64-bit sizes?"

Because even if there is, it shouldn't be "loff_t". That's an
_offset_. Not a length.

            Linus