Re: [GIT PULL] io_uring changes for 5.9-rc1

[Jens Axboe <axboe@kernel.dk>]

On 8/3/20 5:49 PM, Linus Torvalds wrote:
> On Mon, Aug 3, 2020 at 4:31 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Updated to honor exclusive return value as well:
> 
> See my previous email, You're just adding code that makes no sense,
> because your wait entry fundamentally isn't an exclusive one.

Right, I get that now, it's just dead code for my use case. It was sent
out before your previous email.

> So all that code is a no-op and only makes it more confusing to read.
> 
> Your wakeup handler has _nothing_ to do with the generic
> wake_page_function(). There is _zero_ overlap. Your wakeup handler
> gets called only for the wait entries _you_ created.
> 
> Trying to use the wakeup logic from wake_page_function() makes no
> sense, because the rules for wake_page_function() are entirely
> different. Yes, they are called for the same thing (somebody unlocked
> a page and is waking up waiters), but it's using a completely
> different sleeping logic.
> 
> See? When wake_page_function() does that
> 
>         wait->flags |= WQ_FLAG_WOKEN;
> 
> and does something different (and returns different values) depending
> on whether WQ_FLAG_EXCLUSIVE was set, that is all because
> wait_on_page_bit_common() entry set yo that wait entry (on its stack)
> with those exact rules in mind.
> 
> So the wakeup function is 1:1 tied to the code that registers the wait
> entry. wait_on_page_bit_common() has one set of rules, that are then
> honored by the wakeup function it uses. But those rules have _zero_
> impact on your use. You can have - and you *do* have - different sets
> of rules.
> 
> For example, none of your wakeups are ever exclusive. All you do is
> make a work runnable - that doesn't mean that other people shouldn't
> do other things when they get a "page was unlocked" wakeup
> notification.
> 
> Also, for you "list_del_init()" is fine, because you never do the
> unlocked "list_empty_careful()" on that wait entry.  All the waitqueue
> operations run under the queue head lock.
> 
> So what I think you _should_ do is just something like this:
> 
>     diff --git a/fs/io_uring.c b/fs/io_uring.c
>     index 2a3af95be4ca..1e243f99643b 100644
>     --- a/fs/io_uring.c
>     +++ b/fs/io_uring.c
>     @@ -2965,10 +2965,10 @@ static int io_async_buf_func(struct
> wait_queue_entry *wait, unsigned mode,
>             if (!wake_page_match(wpq, key))
>                     return 0;
> 
>     -       /* Stop waking things up if the page is locked again */
>     -       if (test_bit(key->bit_nr, &key->page->flags))
>     -              return -1;
>     -
>     +       /*
>     +        * Somebody unlocked the page. Unqueue the wait entry
>     +        * and run the task_work
>     +        */
>              list_del_init(&wait->entry);
> 
>              init_task_work(&req->task_work, io_req_task_submit);
> 
> because that matches what you're actually doing.
> 
> There's no reason to stop waking up others because the page is locked,
> because you don't know what others want.
> 
> And there's never any reason for the exclusive thing, b3ecause none of
> what you do guarantees that you take exclusive ownership of the page
> lock. Running the work *may* end up doing a "lock_page()", but you
> don't actually guarantee that.

What I ended up with after the last email was just removing the test
bit:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.9&id=cbd287c09351f1d3a4b3cb9167a2616a11390d32

and I clarified the comments on the io_async_buf_func() to add more
hints on how everything is triggered instead of just a vague "handler"
reference:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.9&id=c1dd91d16246b168b80af9b64c5cc35a66410455

-- 
Jens Axboe