public inbox for [email protected]
 help / color / mirror / Atom feed
* [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 <[email protected]>
---
 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 <[email protected]>
> ---
>  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 <[email protected]>
>> ---
>>   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