On 9/4/24 21:40, Jens Axboe wrote: > On 9/4/24 1:25 PM, Bernd Schubert wrote: >> >> >> On 9/4/24 18:16, Jens Axboe wrote: >>> On 9/4/24 10:08 AM, Bernd Schubert wrote: >>>> Hi Jens, >>>> >>>> thanks for your help. >>>> >>>> On 9/4/24 17:47, Jens Axboe wrote: >>>>> On 9/1/24 7:37 AM, Bernd Schubert wrote: >>>>>> This is to allow copying into the buffer from the application >>>>>> without the need to copy in ring context (and with that, >>>>>> the need that the ring task is active in kernel space). >>>>>> >>>>>> Also absolutely needed for now to avoid this teardown issue >>>>> >>>>> I'm fine using these helpers, but they are absolutely not needed to >>>>> avoid that teardown issue - well they may help because it's already >>>>> mapped, but it's really the fault of your handler from attempting to map >>>>> in user pages from when it's teardown/fallback task_work. If invoked and >>>>> the ring is dying or not in the right task (as per the patch from >>>>> Pavel), then just cleanup and return -ECANCELED. >>>> >>>> As I had posted on Friday/Saturday, it didn't work. I had added a >>>> debug pr_info into Pavels patch, somehow it didn't trigger on PF_EXITING >>>> and I didn't further debug it yet as I was working on the pin anyway. >>>> And since Monday occupied with other work... >>> >>> Then there's something wrong with that patch, as it definitely should >>> work. How did you reproduce the teardown crash? I'll take a look here. >> >> Thank you! In this specific case >> >> 1) Run passthrough_hp with --debug-fuse >> >> 2) dd if=/dev/zero of=/scratch/test/testfile bs=1M count=1 >> >> Then on the console that has passthrough_hp output and runs slow with my >> ASAN/etc kernel: ctrl-z and kill -9 % >> I guess a pkill -9 passthrough_hp should also work > > Eerily similar to what I tried, but I managed to get it to trigger. > Should work what's in there, but I think checking for task != current is > better and not race prone like PF_EXITING is. So maybe? Try with the > below incremental. > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 55bdcb4b63b3..fa5a0f724a84 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -121,7 +121,8 @@ 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) > + /* Different task should only happen if the original is going away */ > + if (req->task != current) > flags |= IO_URING_F_TASK_DEAD; > > /* task_work executor checks the deffered list completion */ > Thanks, just tested this version works fine! My user of that (patch 16/17) left the fuse ring entry in bad state - fixed in my v4 branch. Thanks, Bernd
