bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

[Xiaoguang Wang]

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 :)

Regards,
Xiaoguang Wang



















Regards,
Xiaoguang Wang

Re: bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

[Jens Axboe]

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.


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);

-- 
Jens Axboe

Re: bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

[Xiaoguang Wang]

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);
> 

Re: bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

[Jens Axboe]

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

Re: bug report about patch "io_uring: avoid ring quiesce for fixed file set unregister and update"

[Xiaoguang Wang]

hi,

> 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);
> }
Even don't consider below pfile memory allocation failure, because before entering
io_atomic_switch(), the count of refs is 2, the percpu_ref_put calls in io_atomic_switch
makes the count of refs to be 1, so io_file_data_ref_zero  still won't be called, and
refs will always be in atomic mode? Normal reqs just add/dec 1 to the refs, so the cout
of refs will never have a chance to reach zero.

What I worry is that when we finish __io_sqe_files_update, whether fixed_file_data's refs
will always be in atomic mode.

Regards,
Xiaoguang Wang
> 
> 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 :)
> 
> Regards,
> Xiaoguang Wang
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> Regards,
> Xiaoguang Wang