Re: [bug report] WARNING: CPU: 5 PID: 679 at io_uring/io_uring.c:2835 io_ring_exit_work+0x2b6/0x2e0

[Ming Lei <ming.lei@redhat.com>]

On Tue, Apr 16, 2024 at 06:35:14AM -0600, Jens Axboe wrote:
> On 4/16/24 5:38 AM, Jens Axboe wrote:
> > On 4/16/24 4:00 AM, Ming Lei wrote:
> >> On Tue, Apr 16, 2024 at 10:26:16AM +0800, Changhui Zhong wrote:
> >>>>>
> >>>>> I can't reproduce this here, fwiw. Ming, something you've seen?
> >>>>
> >>>> I just test against the latest for-next/block(-rc4 based), and still can't
> >>>> reproduce it. There was such RH internal report before, and maybe not
> >>>> ublk related.
> >>>>
> >>>> Changhui, if the issue can be reproduced in your machine, care to share
> >>>> your machine for me to investigate a bit?
> >>>>
> >>>> Thanks,
> >>>> Ming
> >>>>
> >>>
> >>> I still can reproduce this issue on my machine?
> >>> and I shared machine to Ming?he can do more investigation for this issue?
> >>>
> >>> [ 1244.207092] running generic/006
> >>> [ 1246.456896] blk_print_req_error: 77 callbacks suppressed
> >>> [ 1246.456907] I/O error, dev ublkb1, sector 2395864 op 0x1:(WRITE)
> >>> flags 0x8800 phys_seg 1 prio class 0
> >>
> >> The failure is actually triggered in recovering qcow2 target in generic/005,
> >> since ublkb0 isn't removed successfully in generic/005.
> >>
> >> git-bisect shows that the 1st bad commit is cca6571381a0 ("io_uring/rw:
> >> cleanup retry path").
> >>
> >> And not see any issue in uring command side, so the trouble seems
> >> in normal io_uring rw side over XFS file, and not related with block
> >> device.
> > 
> > Indeed, I can reproduce it on XFS as well. I'll take a look.
> 
> Can you try this one?
> 
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 3c9087f37c43..c67ae6e36c4f 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -527,6 +527,19 @@ static void io_queue_iowq(struct io_kiocb *req)
>  		io_queue_linked_timeout(link);
>  }
>  
> +static void io_tw_requeue_iowq(struct io_kiocb *req, struct io_tw_state *ts)
> +{
> +	req->flags &= ~REQ_F_REISSUE;
> +	io_queue_iowq(req);
> +}
> +
> +void io_tw_queue_iowq(struct io_kiocb *req)
> +{
> +	req->flags |= REQ_F_REISSUE | REQ_F_BL_NO_RECYCLE;
> +	req->io_task_work.func = io_tw_requeue_iowq;
> +	io_req_task_work_add(req);
> +}
> +
>  static __cold void io_queue_deferred(struct io_ring_ctx *ctx)
>  {
>  	while (!list_empty(&ctx->defer_list)) {
> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> index 624ca9076a50..b83a719c5443 100644
> --- a/io_uring/io_uring.h
> +++ b/io_uring/io_uring.h
> @@ -75,6 +75,7 @@ struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
>  void __io_req_task_work_add(struct io_kiocb *req, unsigned flags);
>  bool io_alloc_async_data(struct io_kiocb *req);
>  void io_req_task_queue(struct io_kiocb *req);
> +void io_tw_queue_iowq(struct io_kiocb *req);
>  void io_req_task_complete(struct io_kiocb *req, struct io_tw_state *ts);
>  void io_req_task_queue_fail(struct io_kiocb *req, int ret);
>  void io_req_task_submit(struct io_kiocb *req, struct io_tw_state *ts);
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 3134a6ece1be..4fed829fe97c 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -455,7 +455,7 @@ static bool __io_complete_rw_common(struct io_kiocb *req, long res)
>  			 * current cycle.
>  			 */
>  			io_req_io_end(req);
> -			req->flags |= REQ_F_REISSUE | REQ_F_BL_NO_RECYCLE;
> +			io_tw_queue_iowq(req);
>  			return true;
>  		}
>  		req_set_fail(req);
> @@ -521,7 +521,7 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
>  		io_req_end_write(req);
>  	if (unlikely(res != req->cqe.res)) {
>  		if (res == -EAGAIN && io_rw_should_reissue(req)) {
> -			req->flags |= REQ_F_REISSUE | REQ_F_BL_NO_RECYCLE;
> +			io_tw_queue_iowq(req);
>  			return;
>  		}
>  		req->cqe.res = res;
> @@ -839,7 +839,8 @@ static int __io_read(struct io_kiocb *req, unsigned int issue_flags)
>  	ret = io_iter_do_read(rw, &io->iter);
>  
>  	if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) {
> -		req->flags &= ~REQ_F_REISSUE;
> +		if (req->flags & REQ_F_REISSUE)
> +			return IOU_ISSUE_SKIP_COMPLETE;
>  		/* If we can poll, just do that. */
>  		if (io_file_can_poll(req))
>  			return -EAGAIN;
> @@ -1034,10 +1035,8 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
>  	else
>  		ret2 = -EINVAL;
>  
> -	if (req->flags & REQ_F_REISSUE) {
> -		req->flags &= ~REQ_F_REISSUE;
> -		ret2 = -EAGAIN;
> -	}
> +	if (req->flags & REQ_F_REISSUE)
> +		return IOU_ISSUE_SKIP_COMPLETE;
>  
>  	/*
>  	 * Raw bdev writes will return -EOPNOTSUPP for IOCB_NOWAIT. Just
> 

With above patch against for-next of block tree, ublksrv 'make test T=generic/005'
can pass now, please feel free to add:

Tested-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming