Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [GIT PULL] io_uring fixes for 5.6-rc2
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>
References: <d72d51a9-488d-c75b-4daf-bb74960c7531@kernel.dk>
 <CAHk-=wixEw+wKJzwfEFnBYLNt5zU6zA2kpNVu_36e33_zsawKA@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <ba8f3ef7-a696-0e47-eadb-7772e6aba725@kernel.dk>
Date:   Fri, 14 Feb 2020 18:20:53 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <CAHk-=wixEw+wKJzwfEFnBYLNt5zU6zA2kpNVu_36e33_zsawKA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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