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

From: Jens Axboe
Date: Mon Aug 03 2020 - 19:56:05 EST


On 8/3/20 5:49 PM, Linus Torvalds wrote:
> On Mon, Aug 3, 2020 at 4:31 PM Jens Axboe <axboe@xxxxxxxxx> 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