public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: cancel all requests on task exit
@ 2021-01-17  4:04 Jens Axboe
  2021-01-17 10:52 ` Pavel Begunkov
  0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2021-01-17  4:04 UTC (permalink / raw)
  To: io-uring

We used to have task exit tied to canceling files_struct ownership, but we
really should just simplify this and cancel any request that the task has
pending when it exits. Instead of handling files ownership specially, we
do the same regardless of request type.

This can be further simplified in the next major kernel release, unifying
how we cancel across exec and exit.

Cc: [email protected] # 5.9+
Signed-off-by: Jens Axboe <[email protected]>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 383ff6ed3734..1190296fc95f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
 		io_uring_del_task_file(file);
 }
 
-void __io_uring_files_cancel(struct files_struct *files)
+static void __io_uring_files_cancel(void)
 {
 	struct io_uring_task *tctx = current->io_uring;
 	struct file *file;
@@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
 	/* make sure overflow events are dropped */
 	atomic_inc(&tctx->in_idle);
 	xa_for_each(&tctx->xa, index, file)
-		io_uring_cancel_task_requests(file->private_data, files);
+		io_uring_cancel_task_requests(file->private_data, NULL);
 	atomic_dec(&tctx->in_idle);
 
-	if (files)
-		io_uring_remove_task_files(tctx);
+	io_uring_remove_task_files(tctx);
 }
 
 static s64 tctx_inflight(struct io_uring_task *tctx)
@@ -9087,14 +9086,14 @@ void __io_uring_task_cancel(void)
 
 	/* trigger io_disable_sqo_submit() */
 	if (tctx->sqpoll)
-		__io_uring_files_cancel(NULL);
+		__io_uring_files_cancel();
 
 	do {
 		/* read completions before cancelations */
 		inflight = tctx_inflight(tctx);
 		if (!inflight)
 			break;
-		__io_uring_files_cancel(NULL);
+		__io_uring_files_cancel();
 
 		prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
 
diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
index 35b2d845704d..378662dcfa02 100644
--- a/include/linux/io_uring.h
+++ b/include/linux/io_uring.h
@@ -37,7 +37,6 @@ struct io_uring_task {
 #if defined(CONFIG_IO_URING)
 struct sock *io_uring_get_socket(struct file *file);
 void __io_uring_task_cancel(void);
-void __io_uring_files_cancel(struct files_struct *files);
 void __io_uring_free(struct task_struct *tsk);
 
 static inline void io_uring_task_cancel(void)
@@ -45,11 +44,6 @@ static inline void io_uring_task_cancel(void)
 	if (current->io_uring && !xa_empty(&current->io_uring->xa))
 		__io_uring_task_cancel();
 }
-static inline void io_uring_files_cancel(struct files_struct *files)
-{
-	if (current->io_uring && !xa_empty(&current->io_uring->xa))
-		__io_uring_files_cancel(files);
-}
 static inline void io_uring_free(struct task_struct *tsk)
 {
 	if (tsk->io_uring)
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e35e69a..81523e8d8893 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -777,7 +777,7 @@ void __noreturn do_exit(long code)
 		schedule();
 	}
 
-	io_uring_files_cancel(tsk->files);
+	io_uring_task_cancel();
 	exit_signals(tsk);  /* sets PF_EXITING */
 
 	/* sync mm's RSS info before statistics gathering */
-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] io_uring: cancel all requests on task exit
  2021-01-17  4:04 [PATCH] io_uring: cancel all requests on task exit Jens Axboe
@ 2021-01-17 10:52 ` Pavel Begunkov
  2021-01-17 12:51   ` Josef
  2021-01-17 14:53   ` Jens Axboe
  0 siblings, 2 replies; 5+ messages in thread
From: Pavel Begunkov @ 2021-01-17 10:52 UTC (permalink / raw)
  To: Jens Axboe, io-uring

On 17/01/2021 04:04, Jens Axboe wrote:
> We used to have task exit tied to canceling files_struct ownership, but we
> really should just simplify this and cancel any request that the task has
> pending when it exits. Instead of handling files ownership specially, we
> do the same regardless of request type.
> 
> This can be further simplified in the next major kernel release, unifying
> how we cancel across exec and exit.

Looks good in general. See a comment below, but otherwise
Reviewed-by: Pavel Begunkov <[email protected]>

btw, I wonder if we can incite syzbot to try to break it.

> 
> Cc: [email protected] # 5.9+
> Signed-off-by: Jens Axboe <[email protected]>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 383ff6ed3734..1190296fc95f 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
>  		io_uring_del_task_file(file);
>  }
>  
> -void __io_uring_files_cancel(struct files_struct *files)
> +static void __io_uring_files_cancel(void)
>  {
>  	struct io_uring_task *tctx = current->io_uring;
>  	struct file *file;
> @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
>  	/* make sure overflow events are dropped */
>  	atomic_inc(&tctx->in_idle);
>  	xa_for_each(&tctx->xa, index, file)
> -		io_uring_cancel_task_requests(file->private_data, files);
> +		io_uring_cancel_task_requests(file->private_data, NULL);
>  	atomic_dec(&tctx->in_idle);
>  
> -	if (files)
> -		io_uring_remove_task_files(tctx);
> +	io_uring_remove_task_files(tctx);

This restricts cancellations to only one iteration. Just delete it,
__io_uring_task_cancel() calls it already.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] io_uring: cancel all requests on task exit
  2021-01-17 10:52 ` Pavel Begunkov
@ 2021-01-17 12:51   ` Josef
  2021-01-17 14:52     ` Jens Axboe
  2021-01-17 14:53   ` Jens Axboe
  1 sibling, 1 reply; 5+ messages in thread
From: Josef @ 2021-01-17 12:51 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: Jens Axboe, io-uring

thanks :) I ran several tests, I can confirm this fixed the issue.
Tested-by: Josef Grieb <[email protected]>

On Sun, 17 Jan 2021 at 12:00, Pavel Begunkov <[email protected]> wrote:
>
> On 17/01/2021 04:04, Jens Axboe wrote:
> > We used to have task exit tied to canceling files_struct ownership, but we
> > really should just simplify this and cancel any request that the task has
> > pending when it exits. Instead of handling files ownership specially, we
> > do the same regardless of request type.
> >
> > This can be further simplified in the next major kernel release, unifying
> > how we cancel across exec and exit.
>
> Looks good in general. See a comment below, but otherwise
> Reviewed-by: Pavel Begunkov <[email protected]>
>
> btw, I wonder if we can incite syzbot to try to break it.
>
> >
> > Cc: [email protected] # 5.9+
> > Signed-off-by: Jens Axboe <[email protected]>
> >
> > ---
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 383ff6ed3734..1190296fc95f 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
> >               io_uring_del_task_file(file);
> >  }
> >
> > -void __io_uring_files_cancel(struct files_struct *files)
> > +static void __io_uring_files_cancel(void)
> >  {
> >       struct io_uring_task *tctx = current->io_uring;
> >       struct file *file;
> > @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
> >       /* make sure overflow events are dropped */
> >       atomic_inc(&tctx->in_idle);
> >       xa_for_each(&tctx->xa, index, file)
> > -             io_uring_cancel_task_requests(file->private_data, files);
> > +             io_uring_cancel_task_requests(file->private_data, NULL);
> >       atomic_dec(&tctx->in_idle);
> >
> > -     if (files)
> > -             io_uring_remove_task_files(tctx);
> > +     io_uring_remove_task_files(tctx);
>
> This restricts cancellations to only one iteration. Just delete it,
> __io_uring_task_cancel() calls it already.
>
> --
> Pavel Begunkov



-- 
Josef

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] io_uring: cancel all requests on task exit
  2021-01-17 12:51   ` Josef
@ 2021-01-17 14:52     ` Jens Axboe
  0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2021-01-17 14:52 UTC (permalink / raw)
  To: Josef, Pavel Begunkov; +Cc: io-uring

On 1/17/21 5:51 AM, Josef wrote:
> thanks :) I ran several tests, I can confirm this fixed the issue.
> Tested-by: Josef Grieb <[email protected]>

Awesome, thanks!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] io_uring: cancel all requests on task exit
  2021-01-17 10:52 ` Pavel Begunkov
  2021-01-17 12:51   ` Josef
@ 2021-01-17 14:53   ` Jens Axboe
  1 sibling, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2021-01-17 14:53 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 1/17/21 3:52 AM, Pavel Begunkov wrote:
> On 17/01/2021 04:04, Jens Axboe wrote:
>> We used to have task exit tied to canceling files_struct ownership, but we
>> really should just simplify this and cancel any request that the task has
>> pending when it exits. Instead of handling files ownership specially, we
>> do the same regardless of request type.
>>
>> This can be further simplified in the next major kernel release, unifying
>> how we cancel across exec and exit.
> 
> Looks good in general. See a comment below, but otherwise
> Reviewed-by: Pavel Begunkov <[email protected]>
> 
> btw, I wonder if we can incite syzbot to try to break it.
> 
>>
>> Cc: [email protected] # 5.9+
>> Signed-off-by: Jens Axboe <[email protected]>
>>
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 383ff6ed3734..1190296fc95f 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
>>  		io_uring_del_task_file(file);
>>  }
>>  
>> -void __io_uring_files_cancel(struct files_struct *files)
>> +static void __io_uring_files_cancel(void)
>>  {
>>  	struct io_uring_task *tctx = current->io_uring;
>>  	struct file *file;
>> @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
>>  	/* make sure overflow events are dropped */
>>  	atomic_inc(&tctx->in_idle);
>>  	xa_for_each(&tctx->xa, index, file)
>> -		io_uring_cancel_task_requests(file->private_data, files);
>> +		io_uring_cancel_task_requests(file->private_data, NULL);
>>  	atomic_dec(&tctx->in_idle);
>>  
>> -	if (files)
>> -		io_uring_remove_task_files(tctx);
>> +	io_uring_remove_task_files(tctx);
> 
> This restricts cancellations to only one iteration. Just delete it,
> __io_uring_task_cancel() calls it already.

Ah indeed, makes it even better. I removed it, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-01-17 14:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-17  4:04 [PATCH] io_uring: cancel all requests on task exit Jens Axboe
2021-01-17 10:52 ` Pavel Begunkov
2021-01-17 12:51   ` Josef
2021-01-17 14:52     ` Jens Axboe
2021-01-17 14:53   ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox