[PATCH] block/io_uring: resubmit when result is -EAGAIN

Fabian Ebner posted 1 patch 2 years, 9 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210728103518.1221195-1-f.ebner@proxmox.com
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Aarushi Mehta <mehta.aaru20@gmail.com>, Julia Suvorova <jusual@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
block/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] block/io_uring: resubmit when result is -EAGAIN
Posted by Fabian Ebner 2 years, 9 months ago
Quoting from [0]:

 Some setups, like SCSI, can throw spurious -EAGAIN off the softirq
 completion path. Normally we expect this to happen inline as part
 of submission, but apparently SCSI has a weird corner case where it
 can happen as part of normal completions.

Host kernels without patch [0] can panic when this happens [1], and
resubmitting makes the panic more likely. On the other hand, for
kernels with patch [0], resubmitting ensures that a block job is not
aborted just because of such spurious errors.

[0]: https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u

[1]:
  #9 [ffffb732000c8b70] asm_exc_page_fault at ffffffffa4800ade
 #10 [ffffb732000c8bf8] io_prep_async_work at ffffffffa3d89c16
 #11 [ffffb732000c8c50] io_rw_reissue at ffffffffa3d8b2e1
 #12 [ffffb732000c8c78] io_complete_rw at ffffffffa3d8baa8
 #13 [ffffb732000c8c98] blkdev_bio_end_io at ffffffffa3d62a80
 #14 [ffffb732000c8cc8] bio_endio at ffffffffa3f4e800
 #15 [ffffb732000c8ce8] dec_pending at ffffffffa432f854
 #16 [ffffb732000c8d30] clone_endio at ffffffffa433170c
 #17 [ffffb732000c8d70] bio_endio at ffffffffa3f4e800
 #18 [ffffb732000c8d90] blk_update_request at ffffffffa3f53a37
 #19 [ffffb732000c8dd0] scsi_end_request at ffffffffa4233a5c
 #20 [ffffb732000c8e08] scsi_io_completion at ffffffffa423432c
 #21 [ffffb732000c8e58] scsi_finish_command at ffffffffa422c527
 #22 [ffffb732000c8e88] scsi_softirq_done at ffffffffa42341e4

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

I'm new to this code and io_uring, so I don't know what all the
implications are, but retrying upon EAGAIN does not sound like
a bad thing to my inexperienced ears.

Some more context, leading up to this patch:

We had some users reporting issues after we switched to using io_uring
by default. Namely, kernel panics [2] for some, and failing block jobs
[3] (with a custom backup mechanism we implemented on top of QEMU's
block layer) for others.

I had luck and managed to reprouce the issue, and it was a failed
block job about half of the time and a kernel panic the other half.
When using a host kernel with [0], it's a failed block job all the
time, and this patch attempts to fix that, by resubmitting instead
of bubbling up the error.

[2]: https://forum.proxmox.com/threads/kernel-panic-whole-server-crashes-about-every-day.91803/post-404382
[3]: https://forum.proxmox.com/threads/backup-job-failed-with-err-11-on-2-of-6-vms.92568/

 block/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io_uring.c b/block/io_uring.c
index 00a3ee9fb8..77d162cb24 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -165,7 +165,7 @@ static void luring_process_completions(LuringState *s)
         total_bytes = ret + luringcb->total_read;
 
         if (ret < 0) {
-            if (ret == -EINTR) {
+            if (ret == -EINTR || ret == -EAGAIN) {
                 luring_resubmit(s, luringcb);
                 continue;
             }
-- 
2.30.2



Re: [PATCH] block/io_uring: resubmit when result is -EAGAIN
Posted by Stefan Hajnoczi 2 years, 9 months ago
On Wed, Jul 28, 2021 at 12:35:18PM +0200, Fabian Ebner wrote:
> Quoting from [0]:
> 
>  Some setups, like SCSI, can throw spurious -EAGAIN off the softirq
>  completion path. Normally we expect this to happen inline as part
>  of submission, but apparently SCSI has a weird corner case where it
>  can happen as part of normal completions.
> 
> Host kernels without patch [0] can panic when this happens [1], and
> resubmitting makes the panic more likely. On the other hand, for
> kernels with patch [0], resubmitting ensures that a block job is not
> aborted just because of such spurious errors.
> 
> [0]: https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
> 
> [1]:
>   #9 [ffffb732000c8b70] asm_exc_page_fault at ffffffffa4800ade
>  #10 [ffffb732000c8bf8] io_prep_async_work at ffffffffa3d89c16
>  #11 [ffffb732000c8c50] io_rw_reissue at ffffffffa3d8b2e1
>  #12 [ffffb732000c8c78] io_complete_rw at ffffffffa3d8baa8
>  #13 [ffffb732000c8c98] blkdev_bio_end_io at ffffffffa3d62a80
>  #14 [ffffb732000c8cc8] bio_endio at ffffffffa3f4e800
>  #15 [ffffb732000c8ce8] dec_pending at ffffffffa432f854
>  #16 [ffffb732000c8d30] clone_endio at ffffffffa433170c
>  #17 [ffffb732000c8d70] bio_endio at ffffffffa3f4e800
>  #18 [ffffb732000c8d90] blk_update_request at ffffffffa3f53a37
>  #19 [ffffb732000c8dd0] scsi_end_request at ffffffffa4233a5c
>  #20 [ffffb732000c8e08] scsi_io_completion at ffffffffa423432c
>  #21 [ffffb732000c8e58] scsi_finish_command at ffffffffa422c527
>  #22 [ffffb732000c8e88] scsi_softirq_done at ffffffffa42341e4
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> I'm new to this code and io_uring, so I don't know what all the
> implications are, but retrying upon EAGAIN does not sound like
> a bad thing to my inexperienced ears.
> 
> Some more context, leading up to this patch:
> 
> We had some users reporting issues after we switched to using io_uring
> by default. Namely, kernel panics [2] for some, and failing block jobs
> [3] (with a custom backup mechanism we implemented on top of QEMU's
> block layer) for others.
> 
> I had luck and managed to reprouce the issue, and it was a failed
> block job about half of the time and a kernel panic the other half.
> When using a host kernel with [0], it's a failed block job all the
> time, and this patch attempts to fix that, by resubmitting instead
> of bubbling up the error.

Great, thanks for the patch!

Some of the relevant information is below the '---' line and won't be
included in the git log. Please tweak the commit description to focus
on:
- io_uring may return spurious -EAGAIN with Linux SCSI
  (The details of how io_uring internally resubmits are not important
  but the fact that spurious -EAGAIN is visible to userspace matters.)
- Resubmitting such requests solves the problem

The kernel panic is not as important in the commit description since
this patch works the same regardless of whether the kernel panics
sometimes or not.

> 
> [2]: https://forum.proxmox.com/threads/kernel-panic-whole-server-crashes-about-every-day.91803/post-404382
> [3]: https://forum.proxmox.com/threads/backup-job-failed-with-err-11-on-2-of-6-vms.92568/
> 
>  block/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/io_uring.c b/block/io_uring.c
> index 00a3ee9fb8..77d162cb24 100644
> --- a/block/io_uring.c
> +++ b/block/io_uring.c
> @@ -165,7 +165,7 @@ static void luring_process_completions(LuringState *s)
>          total_bytes = ret + luringcb->total_read;
>  
>          if (ret < 0) {
> -            if (ret == -EINTR) {
> +            if (ret == -EINTR || ret == -EAGAIN) {
>                  luring_resubmit(s, luringcb);
>                  continue;
>              }

Please add a comment:

 /*
  * Only writev/readv/fsync requests on regular files or host block
  * devices are submitted. Therefore -EAGAIN is not expected but it's
  * known to happen sometimes with Linux SCSI. Submit again and hope the
  * request completes successfully.
  *
  * For more information, see:
  * https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
  *
  * If the code is changed to submit other types of requests in the
  * future, then this workaround may need to be extended to deal with
  * genuine -EAGAIN results that should not be resubmitted immediately.
  */

Stefan
Re: [PATCH] block/io_uring: resubmit when result is -EAGAIN
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago
Cc'ing the maintainers for you. See
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer

$ ./scripts/get_maintainer.pl -f block/io_uring.c
Aarushi Mehta <mehta.aaru20@gmail.com> (maintainer:Linux io_uring)
Julia Suvorova <jusual@redhat.com> (maintainer:Linux io_uring)
Stefan Hajnoczi <stefanha@redhat.com> (maintainer:Linux io_uring)
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Max Reitz <mreitz@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:Linux io_uring)
qemu-devel@nongnu.org (open list:All patches CC here)

Also Cc'ing Stefano for commit b4e44c9944e ("io_uring: retry
io_uring_submit() if it fails with errno=EINTR").
(Stefano, you might want to add yourself a R: tag in MAINTAINERS).

On 7/28/21 12:35 PM, Fabian Ebner wrote:
> Quoting from [0]:
> 
>  Some setups, like SCSI, can throw spurious -EAGAIN off the softirq
>  completion path. Normally we expect this to happen inline as part
>  of submission, but apparently SCSI has a weird corner case where it
>  can happen as part of normal completions.
> 
> Host kernels without patch [0] can panic when this happens [1], and
> resubmitting makes the panic more likely. On the other hand, for
> kernels with patch [0], resubmitting ensures that a block job is not
> aborted just because of such spurious errors.
> 
> [0]: https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
> 
> [1]:
>   #9 [ffffb732000c8b70] asm_exc_page_fault at ffffffffa4800ade
>  #10 [ffffb732000c8bf8] io_prep_async_work at ffffffffa3d89c16
>  #11 [ffffb732000c8c50] io_rw_reissue at ffffffffa3d8b2e1
>  #12 [ffffb732000c8c78] io_complete_rw at ffffffffa3d8baa8
>  #13 [ffffb732000c8c98] blkdev_bio_end_io at ffffffffa3d62a80
>  #14 [ffffb732000c8cc8] bio_endio at ffffffffa3f4e800
>  #15 [ffffb732000c8ce8] dec_pending at ffffffffa432f854
>  #16 [ffffb732000c8d30] clone_endio at ffffffffa433170c
>  #17 [ffffb732000c8d70] bio_endio at ffffffffa3f4e800
>  #18 [ffffb732000c8d90] blk_update_request at ffffffffa3f53a37
>  #19 [ffffb732000c8dd0] scsi_end_request at ffffffffa4233a5c
>  #20 [ffffb732000c8e08] scsi_io_completion at ffffffffa423432c
>  #21 [ffffb732000c8e58] scsi_finish_command at ffffffffa422c527
>  #22 [ffffb732000c8e88] scsi_softirq_done at ffffffffa42341e4
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> I'm new to this code and io_uring, so I don't know what all the
> implications are, but retrying upon EAGAIN does not sound like
> a bad thing to my inexperienced ears.
> 
> Some more context, leading up to this patch:
> 
> We had some users reporting issues after we switched to using io_uring
> by default. Namely, kernel panics [2] for some, and failing block jobs
> [3] (with a custom backup mechanism we implemented on top of QEMU's
> block layer) for others.
> 
> I had luck and managed to reprouce the issue, and it was a failed
> block job about half of the time and a kernel panic the other half.
> When using a host kernel with [0], it's a failed block job all the
> time, and this patch attempts to fix that, by resubmitting instead
> of bubbling up the error.
> 
> [2]: https://forum.proxmox.com/threads/kernel-panic-whole-server-crashes-about-every-day.91803/post-404382
> [3]: https://forum.proxmox.com/threads/backup-job-failed-with-err-11-on-2-of-6-vms.92568/
> 
>  block/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/io_uring.c b/block/io_uring.c
> index 00a3ee9fb8..77d162cb24 100644
> --- a/block/io_uring.c
> +++ b/block/io_uring.c
> @@ -165,7 +165,7 @@ static void luring_process_completions(LuringState *s)
>          total_bytes = ret + luringcb->total_read;
>  
>          if (ret < 0) {
> -            if (ret == -EINTR) {
> +            if (ret == -EINTR || ret == -EAGAIN) {
>                  luring_resubmit(s, luringcb);
>                  continue;
>              }
> 


Re: [PATCH] block/io_uring: resubmit when result is -EAGAIN
Posted by Stefano Garzarella 2 years, 9 months ago
On Wed, Jul 28, 2021 at 02:09:40PM +0200, Philippe Mathieu-Daudé wrote:
>Cc'ing the maintainers for you. See
>https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer
>
>$ ./scripts/get_maintainer.pl -f block/io_uring.c
>Aarushi Mehta <mehta.aaru20@gmail.com> (maintainer:Linux io_uring)
>Julia Suvorova <jusual@redhat.com> (maintainer:Linux io_uring)
>Stefan Hajnoczi <stefanha@redhat.com> (maintainer:Linux io_uring)
>Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
>Max Reitz <mreitz@redhat.com> (supporter:Block layer core)
>qemu-block@nongnu.org (open list:Linux io_uring)
>qemu-devel@nongnu.org (open list:All patches CC here)
>
>Also Cc'ing Stefano for commit b4e44c9944e ("io_uring: retry
>io_uring_submit() if it fails with errno=EINTR").

Thanks Phil!

>(Stefano, you might want to add yourself a R: tag in MAINTAINERS).

Yep, I'll send a patch for that.

>
>On 7/28/21 12:35 PM, Fabian Ebner wrote:
>> Quoting from [0]:
>>
>>  Some setups, like SCSI, can throw spurious -EAGAIN off the softirq
>>  completion path. Normally we expect this to happen inline as part
>>  of submission, but apparently SCSI has a weird corner case where it
>>  can happen as part of normal completions.
>>
>> Host kernels without patch [0] can panic when this happens [1], and
>> resubmitting makes the panic more likely. On the other hand, for
>> kernels with patch [0], resubmitting ensures that a block job is not
>> aborted just because of such spurious errors.
>>
>> [0]: https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
>>
>> [1]:
>>   #9 [ffffb732000c8b70] asm_exc_page_fault at ffffffffa4800ade
>>  #10 [ffffb732000c8bf8] io_prep_async_work at ffffffffa3d89c16
>>  #11 [ffffb732000c8c50] io_rw_reissue at ffffffffa3d8b2e1
>>  #12 [ffffb732000c8c78] io_complete_rw at ffffffffa3d8baa8
>>  #13 [ffffb732000c8c98] blkdev_bio_end_io at ffffffffa3d62a80
>>  #14 [ffffb732000c8cc8] bio_endio at ffffffffa3f4e800
>>  #15 [ffffb732000c8ce8] dec_pending at ffffffffa432f854
>>  #16 [ffffb732000c8d30] clone_endio at ffffffffa433170c
>>  #17 [ffffb732000c8d70] bio_endio at ffffffffa3f4e800
>>  #18 [ffffb732000c8d90] blk_update_request at ffffffffa3f53a37
>>  #19 [ffffb732000c8dd0] scsi_end_request at ffffffffa4233a5c
>>  #20 [ffffb732000c8e08] scsi_io_completion at ffffffffa423432c
>>  #21 [ffffb732000c8e58] scsi_finish_command at ffffffffa422c527
>>  #22 [ffffb732000c8e88] scsi_softirq_done at ffffffffa42341e4
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> I'm new to this code and io_uring, so I don't know what all the
>> implications are, but retrying upon EAGAIN does not sound like
>> a bad thing to my inexperienced ears.

Yeah, that doesn't sound bad.

For kernels that don't have the patch applied, I don't think there's 
much we can do about it, so this change seems okay to me:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>