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

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

On 8/30/24 22:08, Jens Axboe wrote:
> On 8/30/24 8:55 AM, 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
> 
> Right, I did actually consider that yesterday and why I replied with the
> fuse callback needing to do it, but then forgot... Since we can't do a
> generic cleanup callback, it'll have to be done in the handler.
> 
> I do like making this generic and not needing individual task_work
> handlers like this checking for some magic, so I like the flag addition.
> 

Found another issue in (error handling in my code) while working on page 
pinning of the user buffer and fixed that first. Ways to late now (or early)
to continue with the page pinning, but I gave Pavels patch a try with the
additional patch below - same issue. 
I added a warn message to see if triggers - doesn't come up

	if (unlikely(issue_flags & IO_URING_F_TASK_DEAD)) {
		pr_warn("IO_URING_F_TASK_DEAD");
		goto terminating;
	}


I could digg further, but I'm actually not sure if we need to. With early page pinning
the entire function should go away, as I hope that the application can write into the
buffer again. Although I'm not sure yet if Miklos will like that pinning.


bschubert2@imesrv6 linux.git>stg show handle-IO_URING_F_TASK_DEAD
commit 42b4dae795bd37918455bad0ce3eea64b28be03c (HEAD -> fuse-uring-for-6.10-rfc3-without-mmap)
Author: Bernd Schubert <bschubert@ddn.com>
Date:   Sat Aug 31 01:26:26 2024 +0200

    fuse: {uring} Handle IO_URING_F_TASK_DEAD
    
    The ring task is terminating, it not safe to still access
    its resources. Also no need for further actions.

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index e557f595133b..1d5dfa9c0965 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -1003,6 +1003,9 @@ fuse_uring_send_req_in_task(struct io_uring_cmd *cmd,
        BUILD_BUG_ON(sizeof(pdu) > sizeof(cmd->pdu));
        int err;
 
+       if (unlikely(issue_flags & IO_URING_F_TASK_DEAD))
+               goto terminating;
+
        err = fuse_uring_prepare_send(ring_ent);
        if (err)
                goto err;
@@ -1017,6 +1020,10 @@ fuse_uring_send_req_in_task(struct io_uring_cmd *cmd,
        return;
 err:
        fuse_uring_next_fuse_req(ring_ent, queue);
+
+terminating:
+       /* Avoid all actions as the task that issues the ring is terminating */
+       io_uring_cmd_done(cmd, -ECANCELED, 0, issue_flags);
 }
 
 /* queue a fuse request and send it if a ring entry is available */




Thanks,
Bernd