Re: [PATCH for-5.10] io_uring: remove req cancel in ->flush()

[Pavel Begunkov <asml.silence@gmail.com>]

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