Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [GIT PULL] io_uring fixes for 5.10-rc
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        io-uring <io-uring@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <6535286b-2532-dc86-3c6e-1b1e9bce358f@kernel.dk>
 <CAHk-=wjrayP=rOB+v+2eTP8micykkM76t=6vp-hyy+vWYkL8=A@mail.gmail.com>
 <4bcf3012-a4ad-ac2d-e70b-17f17441eea9@kernel.dk>
 <CAHk-=wimYoUtY4ygMNknkKZHqgYBZbkU4Koo5cE6ar8XjHkzGg@mail.gmail.com>
 <ad8db5d0-2fac-90b6-b9e4-746a52b8ac57@kernel.dk>
 <d7095e1d-0363-0aad-5c13-6d9bb189b2c8@kernel.dk>
 <CAHk-=wgyRpBW_NOCKpJ1rZGD9jVOX80EWqKwwZxFeief2Khotg@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <87f88614-3045-89bb-8051-b545f5b1180a@kernel.dk>
Date:   Sat, 21 Nov 2020 15:58:48 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAHk-=wgyRpBW_NOCKpJ1rZGD9jVOX80EWqKwwZxFeief2Khotg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/21/20 11:07 AM, Linus Torvalds wrote:
> On Fri, Nov 20, 2020 at 7:00 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Actually, I think we can do even better. How about just having
>> do_filp_open() exit after LOOKUP_RCU fails, if LOOKUP_RCU was already
>> set in the lookup flags? Then we don't need to change much else, and
>> most of it falls out naturally.
> 
> So I was thinking doing the RCU lookup unconditionally, and then doing
> the nn-RCU lookup if that fails afterwards.
> 
> But your patch looks good to me.
> 
> Except for the issue you noticed.

After having taken a closer look, I think the saner approach is
LOOKUP_NONBLOCK instead of using LOOKUP_RCU which is used more as
a state than lookup flag. I'll try and hack something up that looks
passable.

>> Except it seems that should work, except LOOKUP_RCU does not guarantee
>> that we're not going to do IO:
> 
> Well, almost nothing guarantees lack of IO, since allocations etc can
> still block, but..

Sure, and we can't always avoid that - but blatant block on waiting
for IO should be avoided.

>> [   20.463195]  schedule+0x5f/0xd0
>> [   20.463444]  io_schedule+0x45/0x70
>> [   20.463712]  bit_wait_io+0x11/0x50
>> [   20.463981]  __wait_on_bit+0x2c/0x90
>> [   20.464264]  out_of_line_wait_on_bit+0x86/0x90
>> [   20.464611]  ? var_wake_function+0x30/0x30
>> [   20.464932]  __ext4_find_entry+0x2b5/0x410
>> [   20.465254]  ? d_alloc_parallel+0x241/0x4e0
>> [   20.465581]  ext4_lookup+0x51/0x1b0
>> [   20.465855]  ? __d_lookup+0x77/0x120
>> [   20.466136]  path_openat+0x4e8/0xe40
>> [   20.466417]  do_filp_open+0x79/0x100
> 
> Hmm. Is this perhaps an O_CREAT case? I think we only do the dcache
> lookups under RCU, not the final path component creation.

It's just a basic test that opens all files under a directory. So
no O_CREAT, it's all existing files. I think this is just a case of not
aborting early enough for LOOKUP_NONBLOCK, and we've obviously already
dropped LOOKUP_RCU (and done rcu_read_unlock() again) at this point.

> And there are probably lots of other situations where we finish with
> LOOKUP_RCU (with unlazy_walk()), and then continue.> 
> Example: look at "may_lookup()" - if inode_permission() says "I can't
> do this without blocking" the logic actually just tries to validate
> the current state (that "unlazy_walk()" thing), and then continue
> without RCU.
> 
> It obviously hasn't been about lockless semantics, it's been about
> really being lockless. So LOOKUP_RCU has been a "try to do this
> locklessly" rather than "you cannot take any locks".
> 
> I guess we would have to add a LOOKUP_NOBLOCK thing to actually then
> say "if the RCU lookup fails, return -EAGAIN".
> 
> That's probably not a huge undertaking, but yeah, I didn't think of
> it. I think this is a "we need to have Al tell us if it's reasonable".

Definitely. I did have a weak attempt at LOOKUP_NONBLOCK earlier, I'll
try and resurrect it and see what that leads to. Outside of just pure
lookup, the d_revalidate() was a bit interesting as it may block for
certain cases, but those should be (hopefully) detectable upfront.

-- 
Jens Axboe