Re: [PATCH 1/2] io_uring: add uring_lock as an argument to io_sqe_files_unregister()

[Jens Axboe <axboe@kernel.dk>]

On 2/4/21 4:11 AM, Pavel Begunkov wrote:
> On 04/02/2021 03:34, Hao Xu wrote:
>> 在 2021/2/4 上午12:33, Pavel Begunkov 写道:
>>> On 03/02/2021 14:57, Hao Xu wrote:
>>>> io_sqe_files_unregister is currently called from several places:
>>>>      - syscall io_uring_register (with uring_lock)
>>>>      - io_ring_ctx_wait_and_kill() (without uring_lock)
>>>>
>>>> There is a AA type deadlock in io_sqe_files_unregister(), thus we need
>>>> to know if we hold uring_lock in io_sqe_files_unregister() to fix the
>>>> issue.
>>>
>>> It's ugly, just take the lock and kill the patch. There can't be any
>>> contention during io_ring_ctx_free anyway.
>> Hi Pavel, I don't get it, do you mean this patch isn't needed, and we can just unlock(&uring_lock) before io_run_task_work_sig() and lock(&uring_lock) after it? I knew there won't be contention during io_ring_ctx_free that's why there is no uring_lock in it.
>> I tried to just do unlock(&uring_lock) before io_run_task_sig() without if(locked) check, it reports something like "there are unpaired mutex lock/unlock" since we cannot just unlock if it's from io_ring_ctx_free.
> 
> 
> The ugly part is @locked. I know that there is already similar stuff
> around, but I may go long why and how much I don't like it.
> 
> io_ring_ctx_free()
> {
>     ...
>     lock(uring_lock);
>     files_unregister();
>     unlock(uring_lock);
>     ...
> }
> 
> With this you'll always have the mutex locked in unregister, so
> can drop it unconditionally (if that will ever be needed). It's
> also cleaner from the synchronisation perspective.

Yes that's a much better approach - passing around a 'locked' flag
is not very pretty, and the fewer we have of those the better.

-- 
Jens Axboe