From: Linus Torvalds <firstname.lastname@example.org> To: Jens Axboe <email@example.com> Cc: io-uring <firstname.lastname@example.org>, linux-fsdevel <email@example.com>, Al Viro <firstname.lastname@example.org> 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: <email@example.com> On Mon, Sep 13, 2021 at 3:43 PM Jens Axboe <firstname.lastname@example.org> 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
next prev parent 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' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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).