On 3/17/20 6:13 AM, Xiaoguang Wang wrote:
> 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?
That'd be great, as long as we're ensuring that memory bloat doesn't
become a problem. But never doubt that you can send patches to improve
things - even if they sometimes don't get applied, they may help spark
discussion that will end up leading to a great fix.
--
Jens Axboe
