Message-ID: <CAHk-=wibPKfv7mpReMj5PjKBQi4OsAQ8uwW_7=6VCVnaM-p_Dw@mail.gmail.com>
Date:   Fri, 28 Oct 2022 09:41:35 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Christoph Hellwig <hch@...radead.org>,
        David Howells <dhowells@...hat.com>, willy@...radead.org,
        dchinner@...hat.com, Steve French <smfrench@...il.com>,
        Shyam Prasad N <nspmangalore@...il.com>,
        Rohith Surabattula <rohiths.msft@...il.com>,
        Jeff Layton <jlayton@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>, linux-cifs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 12/12] use less confusing names for iov_iter direction initializers

On Thu, Oct 27, 2022 at 7:34 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> READ/WRITE proved to be actively confusing

I agree, we had the same issue with rw_verify_area()

However:

> Call them ITER_DEST and ITER_SOURCE - at least that is harder
> to misinterpret...

I'm not sure this really helps, or is less likely to cause issues.

The old naming at least had some advantages (yes, yes, this is the
_source_ of the old naming):

> @@ -243,7 +243,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
>         struct iov_iter i;
>         ssize_t bw;
>
> -       iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len);
>
>         file_start_write(file);
>         bw = vfs_iter_write(file, &i, ppos, 0);
> @@ -286,7 +286,7 @@ static int lo_read_simple(struct loop_device *lo, struct request *rq,
>         ssize_t len;
>
>         rq_for_each_segment(bvec, rq, iter) {
> -               iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len);
>                 len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0);
>                 if (len < 0)
>                         return len;

where WRITE is used in the 'write()' function, and READ is used in the
read() function.

So that naming is not great, but it has a fairly obvious pattern in a
lot of code.

Not all code, no, as clearly shown by the other eleven patches in this
series, but still..

The new naming doesn't strike me as being obviously less confusing.
It's not horrible, but I'm also not seeing it as being any less likely
in the long run to then cause the same issues we had with READ/WRITE.
It's not like

                iov_iter_bvec(&i, ITER_DEST, &bvec, 1, bvec.bv_len);

is somehow obviously really clear.

I can see the logic: "the destination is the iter, so the source is
the bvec". I understand. But that was pretty much exactly the logic
behind READ too: "this is a read from the device, so the source is the
bvec". I can well imagine that the new one is clearer for some cases,
and in the context of seeing all these other changes it's all quite
straightforward, but I'm trying to think as a driver writer that is
dealing with one random case at a time, and ITER_DEST doesn't strike
me as hugely intuitive either.

I think the real fix for this is your 11/12, which at least makes the
iter movement helpers warn about mis-use. That said, I hate 11/12 too,
but for a minor technicality: please make the WARN_ON() be a
WARN_ON_ONCE(), and please don't make it abort.

Because otherwise somebody who has a random - but important enough -
driver that does this wrong will just have an unbootable machine.

So your 11/12 is conceptually the right thing, but practically
horribly wrong. While this 12/12 mainly makes me go "If we have a
patch this big, I think we should be able to do better than change
from one ambiguous name to another possibly slightly less ambiguous".

Honestly, I think the *real* fix would be a type-based one. Don't do

        iov_iter_kvec(&iter, ITER_DEST, ...

at all, but instead have two different kinds of 'struct iov_iter': one
as a destination (iov_iter_dst), and one as a source (iov_iter_src),
and then just force all the use-cases to use the right version. The
actual *underlying" struct could still be the same
(iov_iter_implementation), but you'd force people to always use the
right version - kind of the same way a 'const void *' is always a
source, and a 'void *' is always a destination for things like memcpy.

That would catch mis-uses much earlier.

That would also make the patch much bigger, but I do think 99.9% of
all users are very distinct. When you pass a iter source around, that
'iov_iter_src' is basically *always* a source of the data through the
whole call-chain. No?

Maybe I'm 100% wrong and that type-based one has some fundamental
problem in it, but it really feels to me like your dynamic WARN_ON()
calls in 11/12 could have been type-based. Because they are entirely
static based on 'data_source'.

In fact, in a perfect world, 'data_source' as a dynamic flag goes away
entirely, and becomes the compile-time static type. If anything really
needs to change the data_source, it would be done as an inline
function that does a type-cast instead.

And yes, yes, I'm sure we have lots of code that currently is of the
type "just pass it an iov_iter, and depending on data_source it does
something different. I'm looking at __blkdev_direct_IO_simple(), which
seems to be exactly that. So I guess the whole "->direct_IO interface
breaks this, because - as usual - DIRECT_IO is a steaming pile of sh*t
that couldn't do separate read/write functions, but had to be
"special".

Oh well.

I still think that a type-based interface would be better, maybe
together with the bad paths having a "iov_iter_confused" thing that
then needs the runtime checking of ->data_source (aka iov_iter_rw()).
But maybe DIRECT_IO isn't the only thing that thought that it's a good
idea to use the same function for both reads and writes.

                         Linus

Powered by blists - more mailing lists