On 11/21/20 11:07 AM, Linus Torvalds wrote: > On Fri, Nov 20, 2020 at 7:00 PM Jens Axboe <axboe@xxxxxxxxx> 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
