Re: [PATCH 1/2] fs: add support for LOOKUP_NONBLOCK

[Jens Axboe <axboe@kernel.dk>]

On 12/11/20 10:20 AM, Al Viro wrote:
> On Fri, Dec 11, 2020 at 09:05:26AM -0700, Jens Axboe wrote:
> 
>>> Finally, I really wonder what is that for; if you are in conditions when
>>> you really don't want to risk going to sleep, you do *NOT* want to
>>> do mnt_want_write().  Or ->open().  Or truncate().  Or, for Cthulhu
>>> sake, IMA hash calculation.
>>
>> I just want to do the RCU side lookup, that is all. That's my fast path.
>> If that doesn't work, then we'll go through the motions of pushing this
>> to a context that allows blocking open.
> 
> Explain, please.  What's the difference between blocking in a lookup and
> blocking in truncate?  Either your call site is fine with a potentially
> long sleep, or it is not; I don't understand what makes one source of
> that behaviour different from another.

I could filter on O_TRUNC (and O_CREAT) in the caller from the io_uring
side, and in fact we may want to do that in general for RESOLVE_LOOKUP
as well. Or handle it in do_open(), which probably makes a lot more
sense. In reality, io_uring would check this upfront and just not bother
with an inline attempt if O_TRUNC is set, as we know we'll wind up with
-EAGAIN at the end of it.  I don't think the combined semantics make any
sense, as you very well may block if you're doing truncate on the file
as part of open. So that should get added to the patch adding
RESOLVE_LOOKUP.

> "Fast path" in context like "we can't sleep here, but often enough we
> won't need to; here's a function that will bail out rather than blocking,
> let's call that and go through offload to helper thread in rare case
> when it does bail out" does make sense; what you are proposing to do
> here is rather different and AFAICS saying "that's my fast path" is
> meaningless here.

What you're describing is exactly what it is - and in my terminology,
O_TRUNC is not part of my fast path. It may be for the application, but
I cannot support it as we don't know if it'll block. We just have to
assume that it might, and that means it'll be handled from a different
context.

> I really do not understand what it is that you are trying to achieve;
> fastpath lookup part would be usable on its own, but mixed with
> the rest of do_open() (as well as the open_last_lookups(), BTW)
> it does not give the caller any useful warranties.

open_last_lookups() will end up bailing us out early, as we end the RCU
lookup side of things and hence would terminate a LOOKUP_NONBLOCK with
-EAGAIN at that point anyway.

> Theoretically it could be amended into something usable, but you
> would need to make do_open(), open_last_lookups() (as well as
> do_tmpfile()) honour your flag, with similar warranties provided
> to caller.
> 
> AFAICS, without that part it is pretty much worthless.  And details
> of what you are going to do in the missing bits *do* matter - unlike the
> pathwalk side (which is trivial) it has potential for being very
> messy.  I want to see _that_ before we commit to going there, and
> a user-visible flag to openat2() makes a very strong commitment.

Fair enough. In terms of patch flow, do you want that as an addon before
we do RESOLVE_NONBLOCK, or do you want it as part of the core
LOOKUP_NONBLOCK patch?

> PS: just to make sure we are on the same page - O_NDELAY will *NOT*
> suffice here.  I apologize if that's obvious to you, but I think
> it's worth spelling out explicitly.
> 
> PPS: regarding unlazy_walk() change...  If we go that way, I would
> probably changed the name to "try_to_unlazy" and inverted the meaning
> of return value.  0 for success, -E... for failure is fine, but
> false for success, true for failure is asking for recurring confusion.
> IOW, I would rather do something like (completely untested)

Agree, if we're going bool, we should make it the more usually followed
success-on-false instead. And I'm happy to see you drop those
likely/unlikely as well, not a huge fan. I'll fold this into what I had
for that and include your naming change.

-- 
Jens Axboe