* [PATCH] io_uring: reset -EBUSY error when io sq thread is waken up
@ 2020-05-20 4:40 Xiaoguang Wang
2020-05-20 13:03 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Xiaoguang Wang @ 2020-05-20 4:40 UTC (permalink / raw)
To: io-uring; +Cc: axboe, joseph.qi, Xiaoguang Wang
In io_sq_thread(), currently if we get an -EBUSY error, we will
won't clear it again, which will result in io_sq_thread() will
never have a chance to submit sqes again. Below test program test.c
can reveal this bug:
int main(int argc, char *argv[])
{
struct io_uring ring;
int i, fd, ret;
struct io_uring_sqe *sqe;
struct io_uring_cqe *cqe;
struct iovec *iovecs;
void *buf;
struct io_uring_params p;
if (argc < 2) {
printf("%s: file\n", argv[0]);
return 1;
}
memset(&p, 0, sizeof(p));
p.flags = IORING_SETUP_SQPOLL;
ret = io_uring_queue_init_params(4, &ring, &p);
if (ret < 0) {
fprintf(stderr, "queue_init: %s\n", strerror(-ret));
return 1;
}
fd = open(argv[1], O_RDONLY | O_DIRECT);
if (fd < 0) {
perror("open");
return 1;
}
iovecs = calloc(10, sizeof(struct iovec));
for (i = 0; i < 10; i++) {
if (posix_memalign(&buf, 4096, 4096))
return 1;
iovecs[i].iov_base = buf;
iovecs[i].iov_len = 4096;
}
ret = io_uring_register_files(&ring, &fd, 1);
if (ret < 0) {
fprintf(stderr, "%s: register %d\n", __FUNCTION__, ret);
return ret;
}
for (i = 0; i < 10; i++) {
sqe = io_uring_get_sqe(&ring);
if (!sqe)
break;
io_uring_prep_readv(sqe, 0, &iovecs[i], 1, 0);
sqe->flags |= IOSQE_FIXED_FILE;
ret = io_uring_submit(&ring);
sleep(1);
printf("submit %d\n", i);
}
for (i = 0; i < 10; i++) {
io_uring_wait_cqe(&ring, &cqe);
printf("receive: %d\n", i);
if (cqe->res != 4096) {
fprintf(stderr, "ret=%d, wanted 4096\n", cqe->res);
ret = 1;
}
io_uring_cqe_seen(&ring, cqe);
}
close(fd);
io_uring_queue_exit(&ring);
return 0;
}
sudo ./test testfile
above command will hang on the tenth request, to fix this bug, when io
sq_thread is waken up, we reset the variable 'ret' to be zero.
Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
fs/io_uring.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6e51140a5722..747de5cdf38a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6045,6 +6045,8 @@ static int io_sq_thread(void *data)
finish_wait(&ctx->sqo_wait, &wait);
ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+ if (ret == -EBUSY)
+ ret = 0;
continue;
}
finish_wait(&ctx->sqo_wait, &wait);
--
2.17.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] io_uring: reset -EBUSY error when io sq thread is waken up
2020-05-20 4:40 [PATCH] io_uring: reset -EBUSY error when io sq thread is waken up Xiaoguang Wang
@ 2020-05-20 13:03 ` Jens Axboe
2020-05-20 13:16 ` Xiaoguang Wang
0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2020-05-20 13:03 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: joseph.qi
On 5/19/20 10:40 PM, Xiaoguang Wang wrote:
> In io_sq_thread(), currently if we get an -EBUSY error, we will
> won't clear it again, which will result in io_sq_thread() will
> never have a chance to submit sqes again. Below test program test.c
> can reveal this bug:
>
> int main(int argc, char *argv[])
> {
> struct io_uring ring;
> int i, fd, ret;
> struct io_uring_sqe *sqe;
> struct io_uring_cqe *cqe;
> struct iovec *iovecs;
> void *buf;
> struct io_uring_params p;
>
> if (argc < 2) {
> printf("%s: file\n", argv[0]);
> return 1;
> }
>
> memset(&p, 0, sizeof(p));
> p.flags = IORING_SETUP_SQPOLL;
> ret = io_uring_queue_init_params(4, &ring, &p);
> if (ret < 0) {
> fprintf(stderr, "queue_init: %s\n", strerror(-ret));
> return 1;
> }
>
> fd = open(argv[1], O_RDONLY | O_DIRECT);
> if (fd < 0) {
> perror("open");
> return 1;
> }
>
> iovecs = calloc(10, sizeof(struct iovec));
> for (i = 0; i < 10; i++) {
> if (posix_memalign(&buf, 4096, 4096))
> return 1;
> iovecs[i].iov_base = buf;
> iovecs[i].iov_len = 4096;
> }
>
> ret = io_uring_register_files(&ring, &fd, 1);
> if (ret < 0) {
> fprintf(stderr, "%s: register %d\n", __FUNCTION__, ret);
> return ret;
> }
>
> for (i = 0; i < 10; i++) {
> sqe = io_uring_get_sqe(&ring);
> if (!sqe)
> break;
>
> io_uring_prep_readv(sqe, 0, &iovecs[i], 1, 0);
> sqe->flags |= IOSQE_FIXED_FILE;
>
> ret = io_uring_submit(&ring);
> sleep(1);
> printf("submit %d\n", i);
> }
>
> for (i = 0; i < 10; i++) {
> io_uring_wait_cqe(&ring, &cqe);
> printf("receive: %d\n", i);
> if (cqe->res != 4096) {
> fprintf(stderr, "ret=%d, wanted 4096\n", cqe->res);
> ret = 1;
> }
> io_uring_cqe_seen(&ring, cqe);
> }
>
> close(fd);
> io_uring_queue_exit(&ring);
> return 0;
> }
> sudo ./test testfile
> above command will hang on the tenth request, to fix this bug, when io
> sq_thread is waken up, we reset the variable 'ret' to be zero.
>
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
> fs/io_uring.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6e51140a5722..747de5cdf38a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6045,6 +6045,8 @@ static int io_sq_thread(void *data)
> finish_wait(&ctx->sqo_wait, &wait);
>
> ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
> + if (ret == -EBUSY)
> + ret = 0;
> continue;
> }
> finish_wait(&ctx->sqo_wait, &wait);
How about just unconditionally clearing it? There's nothing we
need to preserve at this point, it should just get set to zero
regardless of the value.
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] io_uring: reset -EBUSY error when io sq thread is waken up
2020-05-20 13:03 ` Jens Axboe
@ 2020-05-20 13:16 ` Xiaoguang Wang
0 siblings, 0 replies; 3+ messages in thread
From: Xiaoguang Wang @ 2020-05-20 13:16 UTC (permalink / raw)
To: Jens Axboe, io-uring; +Cc: joseph.qi
hi,
> On 5/19/20 10:40 PM, Xiaoguang Wang wrote:
>> In io_sq_thread(), currently if we get an -EBUSY error, we will
>> won't clear it again, which will result in io_sq_thread() will
>> never have a chance to submit sqes again. Below test program test.c
>> can reveal this bug:
>>
>> int main(int argc, char *argv[])
>> {
>> struct io_uring ring;
>> int i, fd, ret;
>> struct io_uring_sqe *sqe;
>> struct io_uring_cqe *cqe;
>> struct iovec *iovecs;
>> void *buf;
>> struct io_uring_params p;
>>
>> if (argc < 2) {
>> printf("%s: file\n", argv[0]);
>> return 1;
>> }
>>
>> memset(&p, 0, sizeof(p));
>> p.flags = IORING_SETUP_SQPOLL;
>> ret = io_uring_queue_init_params(4, &ring, &p);
>> if (ret < 0) {
>> fprintf(stderr, "queue_init: %s\n", strerror(-ret));
>> return 1;
>> }
>>
>> fd = open(argv[1], O_RDONLY | O_DIRECT);
>> if (fd < 0) {
>> perror("open");
>> return 1;
>> }
>>
>> iovecs = calloc(10, sizeof(struct iovec));
>> for (i = 0; i < 10; i++) {
>> if (posix_memalign(&buf, 4096, 4096))
>> return 1;
>> iovecs[i].iov_base = buf;
>> iovecs[i].iov_len = 4096;
>> }
>>
>> ret = io_uring_register_files(&ring, &fd, 1);
>> if (ret < 0) {
>> fprintf(stderr, "%s: register %d\n", __FUNCTION__, ret);
>> return ret;
>> }
>>
>> for (i = 0; i < 10; i++) {
>> sqe = io_uring_get_sqe(&ring);
>> if (!sqe)
>> break;
>>
>> io_uring_prep_readv(sqe, 0, &iovecs[i], 1, 0);
>> sqe->flags |= IOSQE_FIXED_FILE;
>>
>> ret = io_uring_submit(&ring);
>> sleep(1);
>> printf("submit %d\n", i);
>> }
>>
>> for (i = 0; i < 10; i++) {
>> io_uring_wait_cqe(&ring, &cqe);
>> printf("receive: %d\n", i);
>> if (cqe->res != 4096) {
>> fprintf(stderr, "ret=%d, wanted 4096\n", cqe->res);
>> ret = 1;
>> }
>> io_uring_cqe_seen(&ring, cqe);
>> }
>>
>> close(fd);
>> io_uring_queue_exit(&ring);
>> return 0;
>> }
>> sudo ./test testfile
>> above command will hang on the tenth request, to fix this bug, when io
>> sq_thread is waken up, we reset the variable 'ret' to be zero.
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> ---
>> fs/io_uring.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6e51140a5722..747de5cdf38a 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6045,6 +6045,8 @@ static int io_sq_thread(void *data)
>> finish_wait(&ctx->sqo_wait, &wait);
>>
>> ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
>> + if (ret == -EBUSY)
>> + ret = 0;
>> continue;
>> }
>> finish_wait(&ctx->sqo_wait, &wait);
>
> How about just unconditionally clearing it? There's nothing we
> need to preserve at this point, it should just get set to zero
> regardless of the value.
OK, agree, will send a new version soon, thanks.
Regards,
Xiaoguang Wang
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-20 13:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 4:40 [PATCH] io_uring: reset -EBUSY error when io sq thread is waken up Xiaoguang Wang
2020-05-20 13:03 ` Jens Axboe
2020-05-20 13:16 ` Xiaoguang Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).