[PATCH 0/3] files cancellation cleanup

[PATCH 0/3] files cancellation cleanup

[Pavel Begunkov]

Carefully remove leftovers from removing cancellations by files

Pavel Begunkov (3):
  io_uring: remove cancel_files and inflight tracking
  io_uring: cleanup iowq cancellation files matching
  io_uring: don't pass files for cancellation

 fs/io_uring.c | 168 ++++++++++----------------------------------------
 1 file changed, 32 insertions(+), 136 deletions(-)

-- 
2.24.0

[PATCH 1/3] io_uring: remove cancel_files and inflight tracking

[Pavel Begunkov]

We don't do files-specific cancellations anymore, so we can kill
io_uring_cancel_files() and old tracking scheme where we keep all such
requests in ->inflight_list and synchronise it by ->inflight_lock.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 63 ++-------------------------------------------------
 1 file changed, 2 insertions(+), 61 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5bfcb72c916e..0c886ef49920 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -389,9 +389,6 @@ struct io_ring_ctx {
 		struct hlist_head	*cancel_hash;
 		unsigned		cancel_hash_bits;
 		bool			poll_multi_file;
-
-		spinlock_t		inflight_lock;
-		struct list_head	inflight_list;
 	} ____cacheline_aligned_in_smp;
 
 	struct delayed_work		rsrc_put_work;
@@ -734,10 +731,7 @@ struct io_kiocb {
 	struct io_kiocb			*link;
 	struct percpu_ref		*fixed_rsrc_refs;
 
-	/*
-	 * 1. used with ctx->iopoll_list with reads/writes
-	 * 2. to track reqs with ->files (see io_op_def::file_table)
-	 */
+	/* tracks iopoll requests, see ctx->iopoll_list */
 	struct list_head		inflight_entry;
 	struct callback_head		task_work;
 	/* for polled requests, i.e. IORING_OP_POLL_ADD and async armed poll */
@@ -1331,8 +1325,6 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	INIT_LIST_HEAD(&ctx->iopoll_list);
 	INIT_LIST_HEAD(&ctx->defer_list);
 	INIT_LIST_HEAD(&ctx->timeout_list);
-	spin_lock_init(&ctx->inflight_lock);
-	INIT_LIST_HEAD(&ctx->inflight_list);
 	spin_lock_init(&ctx->rsrc_ref_lock);
 	INIT_LIST_HEAD(&ctx->rsrc_ref_list);
 	INIT_DELAYED_WORK(&ctx->rsrc_put_work, io_rsrc_put_work);
@@ -1451,7 +1443,6 @@ static bool io_grab_identity(struct io_kiocb *req)
 {
 	const struct io_op_def *def = &io_op_defs[req->opcode];
 	struct io_identity *id = req->work.identity;
-	struct io_ring_ctx *ctx = req->ctx;
 
 	if (def->work_flags & IO_WQ_WORK_FSIZE) {
 		if (id->fsize != rlimit(RLIMIT_FSIZE))
@@ -1508,10 +1499,6 @@ static bool io_grab_identity(struct io_kiocb *req)
 		atomic_inc(&id->files->count);
 		get_nsproxy(id->nsproxy);
 		req->flags |= REQ_F_INFLIGHT;
-
-		spin_lock_irq(&ctx->inflight_lock);
-		list_add(&req->inflight_entry, &ctx->inflight_list);
-		spin_unlock_irq(&ctx->inflight_lock);
 		req->work.flags |= IO_WQ_WORK_FILES;
 	}
 	if (!(req->work.flags & IO_WQ_WORK_MM) &&
@@ -6155,15 +6142,10 @@ static int io_req_defer(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 static void io_req_drop_files(struct io_kiocb *req)
 {
-	struct io_ring_ctx *ctx = req->ctx;
 	struct io_uring_task *tctx = req->task->io_uring;
-	unsigned long flags;
 
 	put_files_struct(req->work.identity->files);
 	put_nsproxy(req->work.identity->nsproxy);
-	spin_lock_irqsave(&ctx->inflight_lock, flags);
-	list_del(&req->inflight_entry);
-	spin_unlock_irqrestore(&ctx->inflight_lock, flags);
 	req->flags &= ~REQ_F_INFLIGHT;
 	req->work.flags &= ~IO_WQ_WORK_FILES;
 	if (atomic_read(&tctx->in_idle))
@@ -8919,43 +8901,6 @@ static void io_cancel_defer_files(struct io_ring_ctx *ctx,
 	}
 }
 
-static void io_uring_cancel_files(struct io_ring_ctx *ctx,
-				  struct task_struct *task,
-				  struct files_struct *files)
-{
-	while (!list_empty_careful(&ctx->inflight_list)) {
-		struct io_task_cancel cancel = { .task = task, .files = files };
-		struct io_kiocb *req;
-		DEFINE_WAIT(wait);
-		bool found = false;
-
-		spin_lock_irq(&ctx->inflight_lock);
-		list_for_each_entry(req, &ctx->inflight_list, inflight_entry) {
-			if (req->task != task ||
-			    req->work.identity->files != files)
-				continue;
-			found = true;
-			break;
-		}
-		if (found)
-			prepare_to_wait(&task->io_uring->wait, &wait,
-					TASK_UNINTERRUPTIBLE);
-		spin_unlock_irq(&ctx->inflight_lock);
-
-		/* We need to keep going until we don't find a matching req */
-		if (!found)
-			break;
-
-		io_wq_cancel_cb(ctx->io_wq, io_cancel_task_cb, &cancel, true);
-		io_poll_remove_all(ctx, task, files);
-		io_kill_timeouts(ctx, task, files);
-		/* cancellations _may_ trigger task work */
-		io_run_task_work();
-		schedule();
-		finish_wait(&task->io_uring->wait, &wait);
-	}
-}
-
 static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 					    struct task_struct *task)
 {
@@ -9019,11 +8964,7 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 
 	io_cancel_defer_files(ctx, task, files);
 	io_cqring_overflow_flush(ctx, true, task, files);
-
-	if (!files)
-		__io_uring_cancel_task_requests(ctx, task);
-	else
-		io_uring_cancel_files(ctx, task, files);
+	__io_uring_cancel_task_requests(ctx, task);
 
 	if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) {
 		atomic_dec(&task->io_uring->in_idle);
-- 
2.24.0

[PATCH 2/3] io_uring: cleanup iowq cancellation files matching

[Pavel Begunkov]

Clean up io_cancel_task_cb() and related io-wq cancellation after
renouncing from files cancellations. No need to drag files anymore, just
pass task directly.

io_match_task() guarantees to not walk through linked request when
files==NULL, so we can also get rid of ugly conditional synchronisation
there.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 0c886ef49920..8d181ef44398 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8850,29 +8850,12 @@ static int io_uring_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-struct io_task_cancel {
-	struct task_struct *task;
-	struct files_struct *files;
-};
-
 static bool io_cancel_task_cb(struct io_wq_work *work, void *data)
 {
 	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
-	struct io_task_cancel *cancel = data;
-	bool ret;
-
-	if (cancel->files && (req->flags & REQ_F_LINK_TIMEOUT)) {
-		unsigned long flags;
-		struct io_ring_ctx *ctx = req->ctx;
+	struct task_struct *tsk = data;
 
-		/* protect against races with linked timeouts */
-		spin_lock_irqsave(&ctx->completion_lock, flags);
-		ret = io_match_task(req, cancel->task, cancel->files);
-		spin_unlock_irqrestore(&ctx->completion_lock, flags);
-	} else {
-		ret = io_match_task(req, cancel->task, cancel->files);
-	}
-	return ret;
+	return io_match_task(req, tsk, NULL);
 }
 
 static void io_cancel_defer_files(struct io_ring_ctx *ctx,
@@ -8905,13 +8888,12 @@ static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 					    struct task_struct *task)
 {
 	while (1) {
-		struct io_task_cancel cancel = { .task = task, .files = NULL, };
 		enum io_wq_cancel cret;
 		bool ret = false;
 
 		if (ctx->io_wq) {
 			cret = io_wq_cancel_cb(ctx->io_wq, io_cancel_task_cb,
-					       &cancel, true);
+					       task, true);
 			ret |= (cret != IO_WQ_CANCEL_NOTFOUND);
 		}
 
-- 
2.24.0

[PATCH 3/3] io_uring: don't pass files for cancellation

[Pavel Begunkov]

Remove files from io_uring_cancel_task_requests() and propagate it down,
we don't need as only task is used for it.

One trickier change is removing NULL'ing identity->files in
io_uring_cancel_task_requests(), which is possible because the SQPOLL
task is guaranteed now to be drained and submission-disabled before
cancellation ends, so it's equivalent.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 83 +++++++++++++++++----------------------------------
 1 file changed, 28 insertions(+), 55 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8d181ef44398..91e54d936dd4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1072,24 +1072,9 @@ static inline void io_set_resource_node(struct io_kiocb *req)
 	}
 }
 
-static bool io_match_task(struct io_kiocb *head,
-			  struct task_struct *task,
-			  struct files_struct *files)
+static inline bool io_match_task(struct io_kiocb *req, struct task_struct *task)
 {
-	struct io_kiocb *req;
-
-	if (task && head->task != task)
-		return false;
-	if (!files)
-		return true;
-
-	io_for_each_link(req, head) {
-		if ((req->flags & REQ_F_WORK_INITIALIZED) &&
-		    (req->work.flags & IO_WQ_WORK_FILES) &&
-		    req->work.identity->files == files)
-			return true;
-	}
-	return false;
+	return !task || req->task == task;
 }
 
 static void io_sq_thread_drop_mm_files(void)
@@ -1591,15 +1576,14 @@ static void io_kill_timeout(struct io_kiocb *req)
 /*
  * Returns true if we found and killed one or more timeouts
  */
-static bool io_kill_timeouts(struct io_ring_ctx *ctx, struct task_struct *tsk,
-			     struct files_struct *files)
+static bool io_kill_timeouts(struct io_ring_ctx *ctx, struct task_struct *tsk)
 {
 	struct io_kiocb *req, *tmp;
 	int canceled = 0;
 
 	spin_lock_irq(&ctx->completion_lock);
 	list_for_each_entry_safe(req, tmp, &ctx->timeout_list, timeout.list) {
-		if (io_match_task(req, tsk, files)) {
+		if (io_match_task(req, tsk)) {
 			io_kill_timeout(req);
 			canceled++;
 		}
@@ -1752,8 +1736,7 @@ static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
 
 /* Returns true if there are no backlogged entries after the flush */
 static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force,
-				       struct task_struct *tsk,
-				       struct files_struct *files)
+				       struct task_struct *tsk)
 {
 	struct io_rings *rings = ctx->rings;
 	struct io_kiocb *req, *tmp;
@@ -1767,7 +1750,7 @@ static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force,
 
 	spin_lock_irqsave(&ctx->completion_lock, flags);
 	list_for_each_entry_safe(req, tmp, &ctx->cq_overflow_list, compl.list) {
-		if (!io_match_task(req, tsk, files))
+		if (!io_match_task(req, tsk))
 			continue;
 
 		cqe = io_get_cqring(ctx);
@@ -1807,14 +1790,13 @@ static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force,
 }
 
 static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force,
-				     struct task_struct *tsk,
-				     struct files_struct *files)
+				     struct task_struct *tsk)
 {
 	if (test_bit(0, &ctx->cq_check_overflow)) {
 		/* iopoll syncs against uring_lock, not completion_lock */
 		if (ctx->flags & IORING_SETUP_IOPOLL)
 			mutex_lock(&ctx->uring_lock);
-		__io_cqring_overflow_flush(ctx, force, tsk, files);
+		__io_cqring_overflow_flush(ctx, force, tsk);
 		if (ctx->flags & IORING_SETUP_IOPOLL)
 			mutex_unlock(&ctx->uring_lock);
 	}
@@ -2597,7 +2579,7 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 		 * already triggered a CQE (eg in error).
 		 */
 		if (test_bit(0, &ctx->cq_check_overflow))
-			__io_cqring_overflow_flush(ctx, false, NULL, NULL);
+			__io_cqring_overflow_flush(ctx, false, NULL);
 		if (io_cqring_events(ctx))
 			break;
 
@@ -5518,8 +5500,7 @@ static bool io_poll_remove_one(struct io_kiocb *req)
 /*
  * Returns true if we found and killed one or more poll requests
  */
-static bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
-			       struct files_struct *files)
+static bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk)
 {
 	struct hlist_node *tmp;
 	struct io_kiocb *req;
@@ -5531,7 +5512,7 @@ static bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
 
 		list = &ctx->cancel_hash[i];
 		hlist_for_each_entry_safe(req, tmp, list, hash_node) {
-			if (io_match_task(req, tsk, files))
+			if (io_match_task(req, tsk))
 				posted += io_poll_remove_one(req);
 		}
 	}
@@ -6872,7 +6853,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
 
 	/* if we have a backlog and couldn't flush it all, return BUSY */
 	if (test_bit(0, &ctx->sq_check_overflow)) {
-		if (!__io_cqring_overflow_flush(ctx, false, NULL, NULL))
+		if (!__io_cqring_overflow_flush(ctx, false, NULL))
 			return -EBUSY;
 	}
 
@@ -7200,7 +7181,7 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 	int ret = 0;
 
 	do {
-		io_cqring_overflow_flush(ctx, false, NULL, NULL);
+		io_cqring_overflow_flush(ctx, false, NULL);
 		if (io_cqring_events(ctx) >= min_events)
 			return 0;
 		if (!io_run_task_work())
@@ -7229,7 +7210,7 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 	iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
 	trace_io_uring_cqring_wait(ctx, min_events);
 	do {
-		io_cqring_overflow_flush(ctx, false, NULL, NULL);
+		io_cqring_overflow_flush(ctx, false, NULL);
 		prepare_to_wait_exclusive(&ctx->wait, &iowq.wq,
 						TASK_INTERRUPTIBLE);
 		/* make sure we run task_work before checking for signals */
@@ -8747,7 +8728,7 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
 	smp_rmb();
 	if (!io_sqring_full(ctx))
 		mask |= EPOLLOUT | EPOLLWRNORM;
-	io_cqring_overflow_flush(ctx, false, NULL, NULL);
+	io_cqring_overflow_flush(ctx, false, NULL);
 	if (io_cqring_events(ctx))
 		mask |= EPOLLIN | EPOLLRDNORM;
 
@@ -8810,12 +8791,12 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)
 	/* if force is set, the ring is going away. always drop after that */
 	ctx->cq_overflow_flushed = 1;
 	if (ctx->rings)
-		__io_cqring_overflow_flush(ctx, true, NULL, NULL);
+		__io_cqring_overflow_flush(ctx, true, NULL);
 	idr_for_each(&ctx->personality_idr, io_remove_personalities, ctx);
 	mutex_unlock(&ctx->uring_lock);
 
-	io_kill_timeouts(ctx, NULL, NULL);
-	io_poll_remove_all(ctx, NULL, NULL);
+	io_kill_timeouts(ctx, NULL);
+	io_poll_remove_all(ctx, NULL);
 
 	if (ctx->io_wq)
 		io_wq_cancel_cb(ctx->io_wq, io_cancel_ctx_cb, ctx, true);
@@ -8855,19 +8836,18 @@ static bool io_cancel_task_cb(struct io_wq_work *work, void *data)
 	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
 	struct task_struct *tsk = data;
 
-	return io_match_task(req, tsk, NULL);
+	return io_match_task(req, tsk);
 }
 
 static void io_cancel_defer_files(struct io_ring_ctx *ctx,
-				  struct task_struct *task,
-				  struct files_struct *files)
+				  struct task_struct *task)
 {
 	struct io_defer_entry *de = NULL;
 	LIST_HEAD(list);
 
 	spin_lock_irq(&ctx->completion_lock);
 	list_for_each_entry_reverse(de, &ctx->defer_list, list) {
-		if (io_match_task(de->req, task, files)) {
+		if (io_match_task(de->req, task)) {
 			list_cut_position(&list, &ctx->defer_list, &de->list);
 			break;
 		}
@@ -8905,8 +8885,8 @@ static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 			}
 		}
 
-		ret |= io_poll_remove_all(ctx, task, NULL);
-		ret |= io_kill_timeouts(ctx, task, NULL);
+		ret |= io_poll_remove_all(ctx, task);
+		ret |= io_kill_timeouts(ctx, task);
 		ret |= io_run_task_work();
 		if (!ret)
 			break;
@@ -8930,8 +8910,7 @@ static void io_disable_sqo_submit(struct io_ring_ctx *ctx)
  * hard links. These persist even for failure of cancelations, hence keep
  * looping until none are found.
  */
-static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
-					  struct files_struct *files)
+static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx)
 {
 	struct task_struct *task = current;
 
@@ -8944,18 +8923,12 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
 		io_sq_thread_park(ctx->sq_data);
 	}
 
-	io_cancel_defer_files(ctx, task, files);
-	io_cqring_overflow_flush(ctx, true, task, files);
+	io_cancel_defer_files(ctx, task);
+	io_cqring_overflow_flush(ctx, true, task);
 	__io_uring_cancel_task_requests(ctx, task);
 
 	if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) {
 		atomic_dec(&task->io_uring->in_idle);
-		/*
-		 * If the files that are going away are the ones in the thread
-		 * identity, clear them out.
-		 */
-		if (task->io_uring->identity->files == files)
-			task->io_uring->identity->files = NULL;
 		io_sq_thread_unpark(ctx->sq_data);
 	}
 }
@@ -9032,7 +9005,7 @@ static void __io_uring_files_cancel(void)
 	/* make sure overflow events are dropped */
 	atomic_inc(&tctx->in_idle);
 	xa_for_each(&tctx->xa, index, file)
-		io_uring_cancel_task_requests(file->private_data, NULL);
+		io_uring_cancel_task_requests(file->private_data);
 	atomic_dec(&tctx->in_idle);
 }
 
@@ -9306,7 +9279,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 	 */
 	ret = 0;
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
-		io_cqring_overflow_flush(ctx, false, NULL, NULL);
+		io_cqring_overflow_flush(ctx, false, NULL);
 
 		ret = -EOWNERDEAD;
 		if (unlikely(ctx->sqo_dead))
-- 
2.24.0

Re: [PATCH 0/3] files cancellation cleanup

[Jens Axboe]

On 1/19/21 7:32 PM, Pavel Begunkov wrote:
> Carefully remove leftovers from removing cancellations by files
> 
> Pavel Begunkov (3):
>   io_uring: remove cancel_files and inflight tracking
>   io_uring: cleanup iowq cancellation files matching
>   io_uring: don't pass files for cancellation

This is exactly what I was thinking as the 5.12 cleanup on top of the
5.11 changes! Looks good to me, thanks.

-- 
Jens Axboe

Re: [PATCH 0/3] files cancellation cleanup

[Joseph Qi]

Seems this series can also resolve a possible deadlock case I'm looking
into.

CPU0:
...
io_kill_linked_timeout  // &ctx->completion_lock
io_commit_cqring
__io_queue_deferred
__io_queue_async_work
io_wq_enqueue
io_wqe_enqueue  // &wqe->lock

CPU1:
...
__io_uring_files_cancel
io_wq_cancel_cb
io_wqe_cancel_pending_work  // &wqe->lock
io_cancel_task_cb  // &ctx->completion_lock

Thanks,
Joseph

On 1/20/21 10:32 AM, Pavel Begunkov wrote:
> Carefully remove leftovers from removing cancellations by files
> 
> Pavel Begunkov (3):
>   io_uring: remove cancel_files and inflight tracking
>   io_uring: cleanup iowq cancellation files matching
>   io_uring: don't pass files for cancellation
> 
>  fs/io_uring.c | 168 ++++++++++----------------------------------------
>  1 file changed, 32 insertions(+), 136 deletions(-)
> 

Re: [PATCH 0/3] files cancellation cleanup

[Pavel Begunkov]

On 22/01/2021 09:45, Joseph Qi wrote:
> Seems this series can also resolve a possible deadlock case I'm looking
> into.

It removes dead code, I believe your issue is solved by
("io_uring: get rid of intermediate IORING_OP_CLOSE stage")
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.12/io_uring&id=7be8ba3b656cb4e0158b2c859b949f34a96aa94f

Did you try this series in particular, or tested for-5.12/io_uring
and see that the issue is gone there?

> CPU0:
> ...
> io_kill_linked_timeout  // &ctx->completion_lock
> io_commit_cqring
> __io_queue_deferred
> __io_queue_async_work
> io_wq_enqueue
> io_wqe_enqueue  // &wqe->lock
> 
> CPU1:
> ...
> __io_uring_files_cancel
> io_wq_cancel_cb
> io_wqe_cancel_pending_work  // &wqe->lock
> io_cancel_task_cb  // &ctx->completion_lock
> 
> Thanks,
> Joseph
> 
> On 1/20/21 10:32 AM, Pavel Begunkov wrote:
>> Carefully remove leftovers from removing cancellations by files
>>
>> Pavel Begunkov (3):
>>   io_uring: remove cancel_files and inflight tracking
>>   io_uring: cleanup iowq cancellation files matching
>>   io_uring: don't pass files for cancellation
>>
>>  fs/io_uring.c | 168 ++++++++++----------------------------------------
>>  1 file changed, 32 insertions(+), 136 deletions(-)
>>

-- 
Pavel Begunkov

Re: [PATCH 0/3] files cancellation cleanup

[Joseph Qi]

On 1/23/21 5:49 AM, Pavel Begunkov wrote:
> On 22/01/2021 09:45, Joseph Qi wrote:
>> Seems this series can also resolve a possible deadlock case I'm looking
>> into.
> 
> It removes dead code, I believe your issue is solved by
> ("io_uring: get rid of intermediate IORING_OP_CLOSE stage")
> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.12/io_uring&id=7be8ba3b656cb4e0158b2c859b949f34a96aa94f
> 
I've tested the above commit and the mentioned possible deadlock still
exists.

> Did you try this series in particular, or tested for-5.12/io_uring
> and see that the issue is gone there?
> 
I don't have this tree locally and it takes too long to clone it down.
Will check once it is ready.

Thanks,
Joseph

>> CPU0:
>> ...
>> io_kill_linked_timeout  // &ctx->completion_lock
>> io_commit_cqring
>> __io_queue_deferred
>> __io_queue_async_work
>> io_wq_enqueue
>> io_wqe_enqueue  // &wqe->lock
>>
>> CPU1:
>> ...
>> __io_uring_files_cancel
>> io_wq_cancel_cb
>> io_wqe_cancel_pending_work  // &wqe->lock
>> io_cancel_task_cb  // &ctx->completion_lock
>>

Re: [PATCH 0/3] files cancellation cleanup

[Pavel Begunkov]

On 25/01/2021 02:14, Joseph Qi wrote:
> On 1/23/21 5:49 AM, Pavel Begunkov wrote:
>> On 22/01/2021 09:45, Joseph Qi wrote:
>>> Seems this series can also resolve a possible deadlock case I'm looking
>>> into.
>>
>> It removes dead code, I believe your issue is solved by
>> ("io_uring: get rid of intermediate IORING_OP_CLOSE stage")
>> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.12/io_uring&id=7be8ba3b656cb4e0158b2c859b949f34a96aa94f
>>
> I've tested the above commit and the mentioned possible deadlock still
> exists.

I pasted a wrong patch, apparently had something else on my mind...
The intention was to point to a Jens' patch that made mentioned in
your report __io_uring_files_cancel() unreachable, though it was
dropped for unrelated reasons. So... the bug is still here at the
moment.

Anyway, thanks a lot for testing! Your reports are very helpful

>> Did you try this series in particular, or tested for-5.12/io_uring
>> and see that the issue is gone there?
>>
> I don't have this tree locally and it takes too long to clone it down.
> Will check once it is ready.
> 
> Thanks,
> Joseph
> 
>>> CPU0:
>>> ...
>>> io_kill_linked_timeout  // &ctx->completion_lock
>>> io_commit_cqring
>>> __io_queue_deferred
>>> __io_queue_async_work
>>> io_wq_enqueue
>>> io_wqe_enqueue  // &wqe->lock
>>>
>>> CPU1:
>>> ...
>>> __io_uring_files_cancel
>>> io_wq_cancel_cb
>>> io_wqe_cancel_pending_work  // &wqe->lock
>>> io_cancel_task_cb  // &ctx->completion_lock
>>>

-- 
Pavel Begunkov