RE: [PATCH] io_uring: simplify the SQPOLL thread check when cancelling requests

[lizetao <lizetao1@huawei.com>]

Hi,

> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Monday, January 13, 2025 5:16 AM
> To: Bui Quang Minh <minhquangbui99@gmail.com>; lizetao
> <lizetao1@huawei.com>; linux-kernel@vger.kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>; io-uring@vger.kernel.org;
> syzbot+3c750be01dab672c513d@syzkaller.appspotmail.com
> Subject: Re: [PATCH] io_uring: simplify the SQPOLL thread check when
> cancelling requests
> 
> On 1/12/25 16:14, Bui Quang Minh wrote:
> ...
> >>> @@ -2898,7 +2899,12 @@ static __cold void io_ring_exit_work(struct
> >>> work_struct *work)
> >>>           if (ctx->flags & IORING_SETUP_DEFER_TASKRUN)
> >>>               io_move_task_work_from_local(ctx);
> >>>
> >>> -        while (io_uring_try_cancel_requests(ctx, NULL, true))
> >>> +        /*
> >>> +         * Even if SQPOLL thread reaches this path, don't force
> >>> +         * iopoll here, let the io_uring_cancel_generic handle
> >>> +         * it.
> >>
> >> Just curious, will sq_thread enter this io_ring_exit_work path?
> >
> > AFAIK, yes. The SQPOLL thread is created with create_io_thread, this function
> creates a new task with CLONE_FILES. So all the open files is shared. There will
> be case that the parent closes its io_uring file and SQPOLL thread become the
> only owner of that file. So it can reach this path when terminating.
> 
> The function is run by a separate kthread, the sqpoll task doesn't call it directly.

I also think so, the sqpoll task may not call io_ring_exit_work().
> 
> [...]
> >>>> io_uring,
> >>> -                                     cancel_all);
> >>> +                                     cancel_all,
> >>> +                                     true);
> >>>           }
> >>>
> >>>           if (loop) {
> >>> --
> >>> 2.43.0
> >>>
> >>
> >> Maybe you miss something, just like Begunkov mentioned in your last
> version patch:
> >>
> >>    io_uring_cancel_generic
> >>      WARN_ON_ONCE(sqd && sqd->thread != current);
> >>
> >> This WARN_ON_ONCE will never be triggered, so you could remove it.
> >
> > He meant that we don't need to annotate sqd->thread access in this debug
> check. The io_uring_cancel_generic function has assumption that the sgd is not
> NULL only when it's called by a SQPOLL thread. So the check means to ensure
> this assumption. A data race happens only when this function is called by other
> tasks than the SQPOLL thread, so it can race with the SQPOLL termination.
> However, the sgd is not NULL only when this function is called by SQPOLL
> thread. In normal situation following the io_uring_cancel_generic's assumption,
> the data race cannot happen. And in case the assumption is broken, the
> warning almost always is triggered even if data race happens. So we can ignore
> the race here.
> 
> Right. And that's the point of warnings, they're supposed to be untriggerable,
> otherwise there is a problem with the code that needs to be fixed.

Okay, I understand the meaning of this WARN.
> 
> --
> Pavel Begunkov

---
Li Zetao