Re: [PATCH v5 4/4] io_uring: remove ring quiesce for io_uring_register

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/3/22 23:34, Usama Arif wrote:
> Ring quiesce is currently only used for 2 opcodes
> IORING_REGISTER_ENABLE_RINGS and IORING_REGISTER_RESTRICTIONS.
> IORING_SETUP_R_DISABLED prevents submitting requests and
> so there will be no requests until IORING_REGISTER_ENABLE_RINGS
> is called. And IORING_REGISTER_RESTRICTIONS works only before
> IORING_REGISTER_ENABLE_RINGS is called. Hence ring quiesce is
> not needed for these opcodes and therefore io_uring_register.

I think I'd prefer to retain quiesce code than reverting this
patch later.



> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
> ---
>   fs/io_uring.c | 69 ---------------------------------------------------
>   1 file changed, 69 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 5ae51ea12f0f..89e4dd7e8995 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -11022,64 +11022,6 @@ static __cold int io_register_iowq_max_workers(struct io_ring_ctx *ctx,
>   	return ret;
>   }
>   
> -static bool io_register_op_must_quiesce(int op)
> -{
> -	switch (op) {
> -	case IORING_REGISTER_BUFFERS:
> -	case IORING_UNREGISTER_BUFFERS:
> -	case IORING_REGISTER_FILES:
> -	case IORING_UNREGISTER_FILES:
> -	case IORING_REGISTER_FILES_UPDATE:
> -	case IORING_REGISTER_EVENTFD:
> -	case IORING_REGISTER_EVENTFD_ASYNC:
> -	case IORING_UNREGISTER_EVENTFD:
> -	case IORING_REGISTER_PROBE:
> -	case IORING_REGISTER_PERSONALITY:
> -	case IORING_UNREGISTER_PERSONALITY:
> -	case IORING_REGISTER_FILES2:
> -	case IORING_REGISTER_FILES_UPDATE2:
> -	case IORING_REGISTER_BUFFERS2:
> -	case IORING_REGISTER_BUFFERS_UPDATE:
> -	case IORING_REGISTER_IOWQ_AFF:
> -	case IORING_UNREGISTER_IOWQ_AFF:
> -	case IORING_REGISTER_IOWQ_MAX_WORKERS:
> -		return false;
> -	default:
> -		return true;
> -	}
> -}
> -
> -static __cold int io_ctx_quiesce(struct io_ring_ctx *ctx)
> -{
> -	long ret;
> -
> -	percpu_ref_kill(&ctx->refs);
> -
> -	/*
> -	 * Drop uring mutex before waiting for references to exit. If another
> -	 * thread is currently inside io_uring_enter() it might need to grab the
> -	 * uring_lock to make progress. If we hold it here across the drain
> -	 * wait, then we can deadlock. It's safe to drop the mutex here, since
> -	 * no new references will come in after we've killed the percpu ref.
> -	 */
> -	mutex_unlock(&ctx->uring_lock);
> -	do {
> -		ret = wait_for_completion_interruptible_timeout(&ctx->ref_comp, HZ);
> -		if (ret) {
> -			ret = min(0L, ret);
> -			break;
> -		}
> -
> -		ret = io_run_task_work_sig();
> -		io_req_caches_free(ctx);
> -	} while (ret >= 0);
> -	mutex_lock(&ctx->uring_lock);
> -
> -	if (ret)
> -		io_refs_resurrect(&ctx->refs, &ctx->ref_comp);
> -	return ret;
> -}
> -
>   static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>   			       void __user *arg, unsigned nr_args)
>   	__releases(ctx->uring_lock)
> @@ -11103,12 +11045,6 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>   			return -EACCES;
>   	}
>   
> -	if (io_register_op_must_quiesce(opcode)) {
> -		ret = io_ctx_quiesce(ctx);
> -		if (ret)
> -			return ret;
> -	}
> -
>   	switch (opcode) {
>   	case IORING_REGISTER_BUFFERS:
>   		ret = io_sqe_buffers_register(ctx, arg, nr_args, NULL);
> @@ -11213,11 +11149,6 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>   		break;
>   	}
>   
> -	if (io_register_op_must_quiesce(opcode)) {
> -		/* bring the ctx back to life */
> -		percpu_ref_reinit(&ctx->refs);
> -		reinit_completion(&ctx->ref_comp);
> -	}
>   	return ret;
>   }
>   

-- 
Pavel Begunkov