From: Xiaoguang Wang <[email protected]>
To: Jens Axboe <[email protected]>, [email protected]
Cc: joseph qi <[email protected]>
Subject: Re: bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"
Date: Tue, 17 Mar 2020 20:13:49 +0800 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
hi,
> On 3/16/20 6:14 AM, Xiaoguang Wang wrote:
>> hi,
>>
>> While diving into iouring file register/unregister/update codes, seems that
>> there is one bug in __io_sqe_files_update():
>> if (ref_switch)
>> percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
>>
>> The initial fixed_file_data's refs is 1, assume there are no requests
>> to get/put this refs, and we firstly register 10 files and later update
>> these 10 files, and no memory allocations fails, then above two line of
>> codes in __io_sqe_files_update() will be called, before entering
>> percpu_ref_switch_to_atomic(), the count of refs is still one, and
>> |--> percpu_ref_switch_to_atomic
>> |----> __percpu_ref_switch_mode
>> |------> __percpu_ref_switch_to_atomic
>> |-------- > percpu_ref_get(ref), # now the count of refs will be 2.
>>
>> a while later
>> |--> percpu_ref_switch_to_atomic_rcu
>> |----> percpu_ref_call_confirm_rcu
>> |------ > confirm_switch(), # calls io_atomic_switch, note that the count of refs is 2.
>> |------ > percpu_ref_put # drop one ref
>>
>> static void io_atomic_switch(struct percpu_ref *ref)
>> {
>> struct fixed_file_data *data;
>>
>> /*
>> * Juggle reference to ensure we hit zero, if needed, so we can
>> * switch back to percpu mode
>> */
>> data = container_of(ref, struct fixed_file_data, refs);
>> percpu_ref_put(&data->refs);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> After this operation, the count of refs is 1 now, still not zero, so
>> io_file_data_ref_zero won't be called, then io_ring_file_ref_flush()
>> won't be called, this fixed_file_data's refs will always be in atomic mode,
>> which is bad.
>>
>> percpu_ref_get(&data->refs);
>> }
>>
>> To confirm this bug, I did a hack to kernel:
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5812,7 +5812,10 @@ static bool io_queue_file_removal(struct fixed_file_data *data,
>> * If we fail allocating the struct we need for doing async reomval
>> * of this file, just punt to sync and wait for it.
>> */
>> + /*
>> pfile = kzalloc(sizeof(*pfile), GFP_KERNEL);
>> + */
>> + pfile = NULL;
>> if (!pfile) {
>> pfile = &pfile_stack;
>> pfile->done = &done;
>> To simulate memory allocation failures, then run liburing/test/file-update,
>>
>> [lege@localhost test]$ sudo cat /proc/2091/stack
>> [sudo] password for lege:
>> [<0>] __io_sqe_files_update.isra.85+0x175/0x330
>> [<0>] __io_uring_register+0x178/0xe20
>> [<0>] __x64_sys_io_uring_register+0xa0/0x160
>> [<0>] do_syscall_64+0x55/0x1b0
>> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> (gdb) list * __io_sqe_files_update+0x175
>> 0xffffffff812ec255 is in __io_sqe_files_update (fs/io_uring.c:5830).
>> 5825 llist_add(&pfile->llist, &data->put_llist);
>> 5826
>> 5827 if (pfile == &pfile_stack) {
>> 5828 percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
>> 5829 wait_for_completion(&done);
>> 5830 flush_work(&data->ref_work);
>> 5831 return false;
>>
>> file-update will always hang in wait_for_completion(&done), it's because
>> io_ring_file_ref_flush never has a chance to run.
>>
>> I think how to fix this issue a while, doesn't find a elegant method yet.
>> And applications may issue requests continuously, then fixed_file_data's refs
>> may never have a chance to reach zero, refs will always be in atomic mode.
>> Or the simplest method is to use percpu_ref per registered file :)
>
> For the "oh crap I can't allocate data" stack path, I think the below
> should fix it. Might not be a bad idea to re-think the live updates in
> general, though.
I'm not a native english speaker and afraid that I may misread your replies :)
So I'd like to confirm that do you mind that I implement a percpu_ref per
registered file to track every registered file's status?
Regards,
Xiaoguang Wang
>
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b1fbc4424aa6..3f0c8291a17c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5612,10 +5612,12 @@ static void io_ring_file_ref_flush(struct fixed_file_data *data)
> while ((node = llist_del_all(&data->put_llist)) != NULL) {
> llist_for_each_entry_safe(pfile, tmp, node, llist) {
> io_ring_file_put(data->ctx, pfile->file);
> - if (pfile->done)
> + if (pfile->done) {
> + percpu_ref_get(&data->refs);
> complete(pfile->done);
> - else
> + } else {
> kfree(pfile);
> + }
> }
> }
> }
> @@ -5830,6 +5836,7 @@ static bool io_queue_file_removal(struct fixed_file_data *data,
> llist_add(&pfile->llist, &data->put_llist);
>
> if (pfile == &pfile_stack) {
> + percpu_ref_put(&data->refs);
> percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
> wait_for_completion(&done);
> flush_work(&data->ref_work);
>
next prev parent reply other threads:[~2020-03-17 12:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-16 12:14 bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update" Xiaoguang Wang
2020-03-16 15:24 ` Jens Axboe
2020-03-17 12:13 ` Xiaoguang Wang [this message]
2020-03-17 14:38 ` Jens Axboe
2020-03-16 15:46 ` Xiaoguang Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=78180591-3f3e-f4e8-7618-de7250b479f0@linux.alibaba.com \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox