* [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel
@ 2020-10-09 12:49 Matthew Wilcox (Oracle)
2020-10-09 12:49 ` [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file Matthew Wilcox (Oracle)
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-10-09 12:49 UTC (permalink / raw)
To: Jens Axboe
Cc: Matthew Wilcox (Oracle), linux-fsdevel, io-uring, linux-kernel,
Pavel Begunkov
We have to drop the lock during each iteration, so there's no advantage
to using the advanced API. Convert this to a standard xa_for_each() loop.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/io_uring.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 299c530c66f9..2978cc78538a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
void __io_uring_files_cancel(struct files_struct *files)
{
struct io_uring_task *tctx = current->io_uring;
- XA_STATE(xas, &tctx->xa, 0);
+ struct file *file;
+ unsigned long index;
/* make sure overflow events are dropped */
tctx->in_idle = true;
- do {
- struct io_ring_ctx *ctx;
- struct file *file;
-
- xas_lock(&xas);
- file = xas_next_entry(&xas, ULONG_MAX);
- xas_unlock(&xas);
-
- if (!file)
- break;
-
- ctx = file->private_data;
+ xa_for_each(&tctx->xa, index, file) {
+ struct io_ring_ctx *ctx = file->private_data;
io_uring_cancel_task_requests(ctx, files);
if (files)
io_uring_del_task_file(file);
- } while (1);
+ }
}
static inline bool io_uring_task_idle(struct io_uring_task *tctx)
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file
2020-10-09 12:49 [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Matthew Wilcox (Oracle)
@ 2020-10-09 12:49 ` Matthew Wilcox (Oracle)
2020-10-09 14:57 ` Jens Axboe
2020-10-09 12:49 ` [PATCH 3/3] io_uring: Convert advanced XArray uses to the normal API Matthew Wilcox (Oracle)
2020-10-09 12:57 ` [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Pavel Begunkov
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-10-09 12:49 UTC (permalink / raw)
To: Jens Axboe
Cc: Matthew Wilcox (Oracle), linux-fsdevel, io-uring, linux-kernel,
Pavel Begunkov
The xas_store() wasn't paired with an xas_nomem() loop, so if it couldn't
allocate memory using GFP_NOWAIT, it would leak the reference to the file
descriptor. Also the node pointed to by the xas could be freed between
the call to xas_load() under the rcu_read_lock() and the acquisition of
the xa_lock.
It's easier to just use the normal xa_load/xa_store interface here.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/io_uring.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2978cc78538a..bcef6210bf67 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8586,27 +8586,24 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
*/
static int io_uring_add_task_file(struct file *file)
{
- if (unlikely(!current->io_uring)) {
+ struct io_uring_task *cur_uring = current->io_uring;
+
+ if (unlikely(!cur_uring)) {
int ret;
ret = io_uring_alloc_task_context(current);
if (unlikely(ret))
return ret;
}
- if (current->io_uring->last != file) {
- XA_STATE(xas, ¤t->io_uring->xa, (unsigned long) file);
- void *old;
+ if (cur_uring->last != file) {
+ void *old = xa_load(&cur_uring->xa, (unsigned long)file);
- rcu_read_lock();
- old = xas_load(&xas);
- if (old != file) {
+ if (!old) {
get_file(file);
- xas_lock(&xas);
- xas_store(&xas, file);
- xas_unlock(&xas);
+ xa_store(&cur_uring->xa, (unsigned long)file, file,
+ GFP_KERNEL);
}
- rcu_read_unlock();
- current->io_uring->last = file;
+ cur_uring->last = file;
}
return 0;
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] io_uring: Convert advanced XArray uses to the normal API
2020-10-09 12:49 [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Matthew Wilcox (Oracle)
2020-10-09 12:49 ` [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file Matthew Wilcox (Oracle)
@ 2020-10-09 12:49 ` Matthew Wilcox (Oracle)
2020-10-09 12:57 ` [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Pavel Begunkov
2 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-10-09 12:49 UTC (permalink / raw)
To: Jens Axboe
Cc: Matthew Wilcox (Oracle), linux-fsdevel, io-uring, linux-kernel,
Pavel Begunkov
There are no bugs here that I've spotted, it's just easier to use the
normal API and there are no performance advantages to using the more
verbose advanced API.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/io_uring.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index bcef6210bf67..1a894df526da 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8615,27 +8615,17 @@ static int io_uring_add_task_file(struct file *file)
static void io_uring_del_task_file(struct file *file)
{
struct io_uring_task *tctx = current->io_uring;
- XA_STATE(xas, &tctx->xa, (unsigned long) file);
if (tctx->last == file)
tctx->last = NULL;
-
- xas_lock(&xas);
- file = xas_store(&xas, NULL);
- xas_unlock(&xas);
-
+ file = xa_erase(&tctx->xa, (unsigned long)file);
if (file)
fput(file);
}
static void __io_uring_attempt_task_drop(struct file *file)
{
- XA_STATE(xas, ¤t->io_uring->xa, (unsigned long) file);
- struct file *old;
-
- rcu_read_lock();
- old = xas_load(&xas);
- rcu_read_unlock();
+ struct file *old = xa_load(¤t->io_uring->xa, (unsigned long)file);
if (old == file)
io_uring_del_task_file(file);
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel
2020-10-09 12:49 [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Matthew Wilcox (Oracle)
2020-10-09 12:49 ` [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file Matthew Wilcox (Oracle)
2020-10-09 12:49 ` [PATCH 3/3] io_uring: Convert advanced XArray uses to the normal API Matthew Wilcox (Oracle)
@ 2020-10-09 12:57 ` Pavel Begunkov
2 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-10-09 12:57 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Jens Axboe; +Cc: linux-fsdevel, io-uring, linux-kernel
On 09/10/2020 15:49, Matthew Wilcox (Oracle) wrote:
> We have to drop the lock during each iteration, so there's no advantage
> to using the advanced API. Convert this to a standard xa_for_each() loop.
LGTM, but would be better to add
Reported-by: [email protected]
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> fs/io_uring.c | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 299c530c66f9..2978cc78538a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
> void __io_uring_files_cancel(struct files_struct *files)
> {
> struct io_uring_task *tctx = current->io_uring;
> - XA_STATE(xas, &tctx->xa, 0);
> + struct file *file;
> + unsigned long index;
>
> /* make sure overflow events are dropped */
> tctx->in_idle = true;
>
> - do {
> - struct io_ring_ctx *ctx;
> - struct file *file;
> -
> - xas_lock(&xas);
> - file = xas_next_entry(&xas, ULONG_MAX);
> - xas_unlock(&xas);
> -
> - if (!file)
> - break;
> -
> - ctx = file->private_data;
> + xa_for_each(&tctx->xa, index, file) {
> + struct io_ring_ctx *ctx = file->private_data;
>
> io_uring_cancel_task_requests(ctx, files);
> if (files)
> io_uring_del_task_file(file);
> - } while (1);
> + }
> }
>
> static inline bool io_uring_task_idle(struct io_uring_task *tctx)
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file
2020-10-09 12:49 ` [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file Matthew Wilcox (Oracle)
@ 2020-10-09 14:57 ` Jens Axboe
2020-10-09 17:36 ` Matthew Wilcox
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-10-09 14:57 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: linux-fsdevel, io-uring, linux-kernel, Pavel Begunkov
On 10/9/20 6:49 AM, Matthew Wilcox (Oracle) wrote:
> The xas_store() wasn't paired with an xas_nomem() loop, so if it couldn't
> allocate memory using GFP_NOWAIT, it would leak the reference to the file
> descriptor. Also the node pointed to by the xas could be freed between
> the call to xas_load() under the rcu_read_lock() and the acquisition of
> the xa_lock.
>
> It's easier to just use the normal xa_load/xa_store interface here.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> fs/io_uring.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 2978cc78538a..bcef6210bf67 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8586,27 +8586,24 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
> */
> static int io_uring_add_task_file(struct file *file)
> {
> - if (unlikely(!current->io_uring)) {
> + struct io_uring_task *cur_uring = current->io_uring;
> +
> + if (unlikely(!cur_uring)) {
> int ret;
>
> ret = io_uring_alloc_task_context(current);
> if (unlikely(ret))
> return ret;
> }
I think this is missing a:
cur_uring = current->io_uring;
after the successful io_uring_alloc_task(). I'll also rename it to tctx
like what is used in other spots.
Apart from that, series looks good to me, thanks Matthew!
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file
2020-10-09 14:57 ` Jens Axboe
@ 2020-10-09 17:36 ` Matthew Wilcox
0 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2020-10-09 17:36 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-fsdevel, io-uring, linux-kernel, Pavel Begunkov
On Fri, Oct 09, 2020 at 08:57:55AM -0600, Jens Axboe wrote:
> > + if (unlikely(!cur_uring)) {
> > int ret;
> >
> > ret = io_uring_alloc_task_context(current);
> > if (unlikely(ret))
> > return ret;
> > }
>
> I think this is missing a:
>
> cur_uring = current->io_uring;
>
> after the successful io_uring_alloc_task(). I'll also rename it to tctx
> like what is used in other spots.
Quite right! I should have woken up a little bit more before writing code.
> Apart from that, series looks good to me, thanks Matthew!
NP. At some point, I'd like to understand a bit better how you came
to write the code the way you did, so I can improve the documentation.
Maybe I just need to strengthen the warnings to stay away from the
advanced API unless you absolutely need it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-09 17:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-09 12:49 [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Matthew Wilcox (Oracle)
2020-10-09 12:49 ` [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file Matthew Wilcox (Oracle)
2020-10-09 14:57 ` Jens Axboe
2020-10-09 17:36 ` Matthew Wilcox
2020-10-09 12:49 ` [PATCH 3/3] io_uring: Convert advanced XArray uses to the normal API Matthew Wilcox (Oracle)
2020-10-09 12:57 ` [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox