IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: io-uring <io-uring@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] io_uring fixes for 5.6-rc2
Date: Fri, 14 Feb 2020 18:20:53 -0700
Message-ID: <ba8f3ef7-a696-0e47-eadb-7772e6aba725@kernel.dk> (raw)
In-Reply-To: <CAHk-=wixEw+wKJzwfEFnBYLNt5zU6zA2kpNVu_36e33_zsawKA@mail.gmail.com>

On 2/14/20 3:07 PM, Linus Torvalds wrote:
> On Fri, Feb 14, 2020 at 8:45 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Here's a set of fixes for io_uring that should go into this release.
> 
> Whaa?
> 
>           for_each_node(node) {
> +                if (!node_online(node))
> +                        continue;
> 
> that's just silly.
> 
> We have 'for_each_online_node()' for this.
> 
> There's something like four patterns of that pointless thing.

Sorry, that definitely should have been for_each_online_node() for
those, guess I didn't think of that when making the change.

> And in io_wq_create(), do you really want to allocate that wqe for
> nodes that aren't online? Right now you _allocate_ the node data for
> them (using a non-node-specific allocation), but then you won't
> actually create the thread for them io_wq_manager().

I was thinking about this a bit, and as far as I know there's no good
way to get notified of nodes coming and going. And I'd really like
to avoid having to add that to the fast path.

So this seemed like the lesser of evils, we setup the wqe just in
case the node does come online, and then rely on the manager
creating the thread when we need it. Not sure what setup was run
to create it, I haven't come across any boxes where we have nodes
that are present but not online.

> Plus if the node online status changes, it looks like you'll mess up
> _anyway_, in that  io_wq_manager() will first create the workers on
> one set of nodes, but then perhaps set the state flags for a
> completely different set of nodes if some onlining/offlining has
> happened.

We'll look into making this more clear and bullet proof.

> I've pulled this, but Jens, you need to be more careful. This all
> looks like completely random state that nobody spent any time thinking
> about.
> 
> Seriously, this "io_uring FIXES ONLY" needs to be stricter than what
> you seem to be doing here. This "fix" is opening up a lot of new
> possibilities for inconsistencies in the data structures.

We'll get it sorted for 5.6. Thanks for pulling.

-- 
Jens Axboe


  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 16:45 Jens Axboe
2020-02-14 22:07 ` Linus Torvalds
2020-02-15  1:20   ` Jens Axboe [this message]
2020-02-15 18:40 ` pr-tracker-bot

Reply instructions:

You may reply publically 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=ba8f3ef7-a696-0e47-eadb-7772e6aba725@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /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

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git