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. -- Jens Axboe
