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

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 8/30/24 16:55, Pavel Begunkov wrote:
> 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,
> 


Thanks and yeah you are right, the previous patch would have missed an
io_uring_cmd_done for fuse-uring as well.


Thanks,
Bernd