On 19/10/2020 21:08, Jens Axboe wrote:
> On 10/19/20 9:45 AM, Pavel Begunkov wrote:
>> Every close(io_uring) causes cancellation of all inflight requests
>> carrying ->files. That's not nice but was neccessary up until recently.
>> Now task->files removal is handled in the core code, so that part of
>> flush can be removed.
>
> It does change the behavior, but I'd wager that's safe. One minor
> comment:
Right, but I would think that users are not happy that every close
kills requests without apparent reasons.
>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 95d2bb7069c6..6536e24eb44e 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -8748,16 +8748,12 @@ void __io_uring_task_cancel(void)
>>
>> static int io_uring_flush(struct file *file, void *data)
>> {
>> - struct io_ring_ctx *ctx = file->private_data;
>> + bool exiting = !data;
>>
>> - /*
>> - * If the task is going away, cancel work it may have pending
>> - */
>> if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
>> - data = NULL;
>> + exiting = true;
>>
>> - io_uring_cancel_task_requests(ctx, data);
>> - io_uring_attempt_task_drop(file, !data);
>> + io_uring_attempt_task_drop(file, exiting);
>> return 0;
>> }
>
> Why not just keep the !data for task_drop? Would make the diff take
> away just the hunk we're interested in. Even adding a comment would be
> better, imho.
That would look cleaner, but I just left what already was there. TBH,
I don't even entirely understand why exiting=!data. Looking up how
exit_files() works, it passes down non-NULL files to
put_files_struct() -> ... filp_close() -> f_op->flush().
I'm curious how does this filp_close(file, files=NULL) happens?
Moreover, if that's exit_files() which is interesting, then first
it calls io_uring_cancel_task_requests(), which should remove all
struct file from tctx->xa. I haven't tested it though.
--
Pavel Begunkov
