| Message ID | 20220912165325.2858746-1-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] block: blk_queue_enter() / __bio_queue_enter() must return -EAGAIN for nowait | expand |
On 9/12/22 09:53, Stefan Roesch wrote: > Today blk_queue_enter() and __bio_queue_enter() return -EBUSY for the > nowait code path. This is not correct: they should return -EAGAIN > instead. Why is this considered incorrect? Please explain. Since this patch also affects other code, e.g. NVMe pass-through, can this patch break existing user space code by changing the value returned to user space? Thanks, Bart.
On 9/12/22 11:14 AM, Bart Van Assche wrote: > On 9/12/22 09:53, Stefan Roesch wrote: >> Today blk_queue_enter() and __bio_queue_enter() return -EBUSY for the >> nowait code path. This is not correct: they should return -EAGAIN >> instead. > > Why is this considered incorrect? Please explain. > > Since this patch also affects other code, e.g. NVMe pass-through, can > this patch break existing user space code by changing the value > returned to user space? It is currently broken because we always return EAGAIN/EWOULDBLOCK for these cases, if a non-block flag is set of some sort. I strongly suspect that nobody has really being doing non-blocking IO to devices before io_uring made it a lot more common. EAGAIN means "try again later, or without nonblock said". We should be consistent here.
On Mon, 12 Sep 2022 09:53:25 -0700, Stefan Roesch wrote: > Today blk_queue_enter() and __bio_queue_enter() return -EBUSY for the > nowait code path. This is not correct: they should return -EAGAIN > instead. > > This problem was detected by fio. The following command exposed the > above problem: > > [...] Applied, thanks! [1/1] block: blk_queue_enter() / __bio_queue_enter() must return -EAGAIN for nowait commit: 56f99b8d06ef1ed1c9730948f9f05ac2b930a20b Best regards,
diff --git a/block/blk-core.c b/block/blk-core.c index a0d1104c5590..651057c4146b 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -295,7 +295,7 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) while (!blk_try_enter_queue(q, pm)) { if (flags & BLK_MQ_REQ_NOWAIT) - return -EBUSY; + return -EAGAIN; /* * read pair of barrier in blk_freeze_queue_start(), we need to @@ -325,7 +325,7 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio) if (test_bit(GD_DEAD, &disk->state)) goto dead; bio_wouldblock_error(bio); - return -EBUSY; + return -EAGAIN; } /*
Today blk_queue_enter() and __bio_queue_enter() return -EBUSY for the nowait code path. This is not correct: they should return -EAGAIN instead. This problem was detected by fio. The following command exposed the above problem: t/io_uring -p0 -d128 -b4096 -s32 -c32 -F1 -B0 -R0 -X1 -n24 -P1 -u1 -O0 /dev/ng0n1 By applying the patch, the retry case is handled correctly in the slow path. Signed-off-by: Stefan Roesch <shr@fb.com> --- block/blk-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf