On 19/01/2021 13:39, Pavel Begunkov wrote: > On 19/01/2021 13:12, Joseph Qi wrote: >> On 1/19/21 7:45 PM, Pavel Begunkov wrote: >>> On 19/01/2021 08:00, Joseph Qi wrote: >>>> On 1/19/21 10:38 AM, Pavel Begunkov wrote: >>>>> On 19/01/2021 01:58, Joseph Qi wrote: >>>>>>> Hmm, I hastened, for files we need IO_WQ_WORK_FILES, >>>>>>> +IO_WQ_WORK_BLKCG for same reasons. needs_file would make >>>>>>> it to grab a struct file, that is wrong. >>>>>>> Probably worked out because it just grabbed fd=0/stdin. >>>>>>> >>>>>> >>>>>> I think IO_WQ_WORK_FILES can work since it will acquire >>>>>> files when initialize async cancel request. >>>>> >>>>> That the one controlling files in the first place, need_file >>>>> just happened to grab them submission. >>>>> >>>>>> Don't quite understand why we should have IO_WQ_WORK_BLKCG. >>>>> >>>>> Because it's set for IORING_OP_CLOSE, and similar situation >>>>> may happen but with async_cancel from io-wq. >>>>> >>>> So how about do switch and restore in io_run_cancel(), seems it can >>>> take care of direct request, sqthread and io-wq cases. >>> >>> It will get ugly pretty quickly, + this nesting of io-wq handlers >>> async_handler() -> io_close() is not great... >>> >>> I'm more inclined to skip them in io_wqe_cancel_pending_work() >>> to not execute inline. That may need to do some waiting on the >>> async_cancel side though to not change the semantics. Can you >>> try out this direction? >>> >> Sure, I'll try this way and send v2. > > There may be a much better way, that's to remove IO_WQ_WORK_NO_CANCEL > and move -EAGAIN section of io_close() before close_fd_get_file(), > so not splitting it in 2 and not keeping it half-done. I believe it is the right way, but there are tricks to that. I hope you don't mind me and Jens hijacking taking care of it. Enough of non-technical hassle expected... Thanks for reporting it! > > IIRC, it was done this way because of historical reasons when we > didn't put more stuff around files, but may be wrong. > Jens, do you remember what it was? -- Pavel Begunkov
