Re: [PATCH RFC v2 00/19] fuse: fuse-over-io-uring

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

On 8/30/24 14:33, Jens Axboe wrote:
> On 8/30/24 7:28 AM, Bernd Schubert wrote:
>> On 8/30/24 15:12, Jens Axboe wrote:
>>> On 8/29/24 4:32 PM, Bernd Schubert wrote:
>>>> We probably need to call iov_iter_get_pages2() immediately
>>>> on submitting the buffer from fuse server and not only when needed.
>>>> I had planned to do that as optimization later on, I think
>>>> it is also needed to avoid io_uring_cmd_complete_in_task().
>>>
>>> I think you do, but it's not really what's wrong here - fallback work is
>>> being invoked as the ring is being torn down, either directly or because
>>> the task is exiting. Your task_work should check if this is the case,
>>> and just do -ECANCELED for this case rather than attempt to execute the
>>> work. Most task_work doesn't do much outside of post a completion, but
>>> yours seems complex in that attempts to map pages as well, for example.
>>> In any case, regardless of whether you move the gup to the actual issue
>>> side of things (which I think you should), then you'd want something
>>> ala:
>>>
>>> if (req->task != current)
>>> 	don't issue, -ECANCELED
>>>
>>> in your task_work.nvme_uring_task_cb
>>
>> Thanks a lot for your help Jens! I'm a bit confused, doesn't this belong
>> into __io_uring_cmd_do_in_task then? Because my task_work_cb function
>> (passed to io_uring_cmd_complete_in_task) doesn't even have the request.
> 
> Yeah it probably does, the uring_cmd case is a bit special is that it's
> a set of helpers around task_work that can be consumed by eg fuse and
> ublk. The existing users don't really do anything complicated on that
> side, hence there's no real need to check. But since the ring/task is
> going away, we should be able to generically do it in the helpers like
> you did below.

That won't work, we should give commands an opportunity to clean up
after themselves. I'm pretty sure it will break existing users.
For now we can pass a flag to the callback, fuse would need to
check it and fail. Compile tested only

commit a5b382f150b44476ccfa84cefdb22ce2ceeb12f1
Author: Pavel Begunkov <asml.silence@gmail.com>
Date:   Fri Aug 30 15:43:32 2024 +0100

     io_uring/cmd: let cmds tw know about dying task
     
     When the taks that submitted a request is dying, a task work for that
     request might get run by a kernel thread or even worse by a half
     dismantled task. We can't just cancel the task work without running the
     callback as the cmd might need to do some clean up, so pass a flag
     instead. If set, it's not safe to access any task resources and the
     callback is expected to cancel the cmd ASAP.
     
     Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index ace7ac056d51..a89abec98832 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -37,6 +37,7 @@ enum io_uring_cmd_flags {
  	/* set when uring wants to cancel a previously issued command */
  	IO_URING_F_CANCEL		= (1 << 11),
  	IO_URING_F_COMPAT		= (1 << 12),
+	IO_URING_F_TASK_DEAD		= (1 << 13),
  };
  
  struct io_zcrx_ifq;
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 8391c7c7c1ec..55bdcb4b63b3 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -119,9 +119,13 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);
  static void io_uring_cmd_work(struct io_kiocb *req, struct io_tw_state *ts)
  {
  	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
+	unsigned flags = IO_URING_F_COMPLETE_DEFER;
+
+	if (req->task->flags & PF_EXITING)
+		flags |= IO_URING_F_TASK_DEAD;
  
  	/* task_work executor checks the deffered list completion */
-	ioucmd->task_work_cb(ioucmd, IO_URING_F_COMPLETE_DEFER);
+	ioucmd->task_work_cb(ioucmd, flags);
  }
  
  void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,

-- 
Pavel Begunkov