io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: io-uring <io-uring@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCHSET 0/3] Add ability to save/restore iov_iter state
Date: Mon, 13 Sep 2021 16:23:03 -0700	[thread overview]
Message-ID: <CAHk-=wj3Lu=mJ8L7iE0RQXGZVdoSMz6rnPmrWoVNJhTaObOqkA@mail.gmail.com> (raw)
In-Reply-To: <8a278aa1-81ed-72e0-dec7-b83997e5d801@kernel.dk>

On Mon, Sep 13, 2021 at 3:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Al, Linus, are you OK with this? I think we should get this in for 5.15.
> I didn't resend the whole series, just a v2 of patch 1/3 to fix that bvec
> vs iovec issue. Let me know if you want the while thing resent.

So I'm ok with the iov_iter side, but the io_uring side seems still
positively buggy, and very confused.

It also messes with the state in bad ways and has internal knowledge.
And some of it looks completely bogus.

For example, I see

        state->count -= ret;
        rw->bytes_done += ret;

and I go "that's BS". There's no way it's ok to start messing with the
byte count inside the state like that. That just means that the state
is now no longer the saved state, and it's some random garbage.

I also think that the "bytes_done += ret" is a big hint there: any
time you restore the iovec state, you should then forward it by
"bytes_done". But that's not what the code does.

Instead, it will now restore the iovec styate with the *wrong* number
of bytes remaining, but will start from the beginning of the iovec.

So I think the fs/io_uring.c use of this state buffer is completely wrong.

What *may* be the right thing to do is to

 (a) not mess with state->count

 (b) when you restore the state you always use

        iov_iter_restore(iter, state, bytes_done);

to actually restore the *correct* state.

Because modifying the iovec save state like that cannot be right, and
if it's right it's still too ugly and fragile for words. That save
state should be treated as a snapshot, not as a random buffer that you
can make arbitrary changes to.

See what I'm saying?

I'd like Al to take a look at the io_uring.c usage too, since this was
just my reaction from looking at that diff a bit more.

           Linus

  reply	other threads:[~2021-09-13 23:23 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 18:25 Jens Axboe
2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe
2021-09-10 18:50   ` Al Viro
2021-09-10 19:15     ` [PATCH v2 " Jens Axboe
2021-09-10 18:25 ` [PATCH 2/3] io_uring: use iov_iter state save/restore helpers Jens Axboe
2021-09-10 18:25 ` [PATCH 3/3] Revert "iov_iter: track truncated size" Jens Axboe
2021-09-13 22:43 ` [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe
2021-09-13 23:23   ` Linus Torvalds [this message]
2021-09-14  1:54     ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wj3Lu=mJ8L7iE0RQXGZVdoSMz6rnPmrWoVNJhTaObOqkA@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCHSET 0/3] Add ability to save/restore iov_iter state' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).