public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
@ 2021-09-14 15:12 Pavel Begunkov
  2021-09-14 15:14 ` Jens Axboe
  2021-09-14 15:36 ` Jens Axboe
  0 siblings, 2 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-09-14 15:12 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Franz-B . Tuneke

It might be inconvenient that direct open/accept deviates from the
update semantics and fails if the slot is taken instead of removing a
file sitting there. Implement this auto-removal.

Note that removal might need to allocate and so may fail. However, if an
empty slot is specified, it's guaraneed to not fail on the fd
installation side for valid userspace programs. It's needed for users
who can't tolerate such failures, e.g. accept where the other end
never retries.

Suggested-by: Franz-B. Tuneke <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---

v2: simplify io_rsrc_node_switch_start() handling

 fs/io_uring.c | 52 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a864a94364c6..58c0cbfdd128 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8287,11 +8287,27 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
 #endif
 }
 
+static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
+				 struct io_rsrc_node *node, void *rsrc)
+{
+	struct io_rsrc_put *prsrc;
+
+	prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
+	if (!prsrc)
+		return -ENOMEM;
+
+	prsrc->tag = *io_get_tag_slot(data, idx);
+	prsrc->rsrc = rsrc;
+	list_add(&prsrc->list, &node->rsrc_list);
+	return 0;
+}
+
 static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool needs_switch = false;
 	struct io_fixed_file *file_slot;
 	int ret = -EBADF;
 
@@ -8307,9 +8323,22 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 
 	slot_index = array_index_nospec(slot_index, ctx->nr_user_files);
 	file_slot = io_fixed_file_slot(&ctx->file_table, slot_index);
-	ret = -EBADF;
-	if (file_slot->file_ptr)
-		goto err;
+
+	if (file_slot->file_ptr) {
+		struct file *old_file;
+
+		ret = io_rsrc_node_switch_start(ctx);
+		if (ret)
+			goto err;
+
+		old_file = (struct file *)(file_slot->file_ptr & FFS_MASK);
+		ret = io_queue_rsrc_removal(ctx->file_data, slot_index,
+					    ctx->rsrc_node, old_file);
+		if (ret)
+			goto err;
+		file_slot->file_ptr = 0;
+		needs_switch = true;
+	}
 
 	*io_get_tag_slot(ctx->file_data, slot_index) = 0;
 	io_fixed_file_set(file_slot, file);
@@ -8321,27 +8350,14 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 
 	ret = 0;
 err:
+	if (needs_switch)
+		io_rsrc_node_switch(ctx, ctx->file_data);
 	io_ring_submit_unlock(ctx, !force_nonblock);
 	if (ret)
 		fput(file);
 	return ret;
 }
 
-static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
-				 struct io_rsrc_node *node, void *rsrc)
-{
-	struct io_rsrc_put *prsrc;
-
-	prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
-	if (!prsrc)
-		return -ENOMEM;
-
-	prsrc->tag = *io_get_tag_slot(data, idx);
-	prsrc->rsrc = rsrc;
-	list_add(&prsrc->list, &node->rsrc_list);
-	return 0;
-}
-
 static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				 struct io_uring_rsrc_update2 *up,
 				 unsigned nr_args)
-- 
2.33.0


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

* Re: [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
  2021-09-14 15:12 [PATCH v2 5.15] io_uring: auto-removal for direct open/accept Pavel Begunkov
@ 2021-09-14 15:14 ` Jens Axboe
  2021-09-14 15:21   ` Pavel Begunkov
  2021-09-14 15:36 ` Jens Axboe
  1 sibling, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2021-09-14 15:14 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Franz-B . Tuneke

On 9/14/21 9:12 AM, Pavel Begunkov wrote:
> It might be inconvenient that direct open/accept deviates from the
> update semantics and fails if the slot is taken instead of removing a
> file sitting there. Implement this auto-removal.
> 
> Note that removal might need to allocate and so may fail. However, if an
> empty slot is specified, it's guaraneed to not fail on the fd
> installation side for valid userspace programs. It's needed for users
> who can't tolerate such failures, e.g. accept where the other end
> never retries.

Can you submit a liburing test case for this too?

-- 
Jens Axboe


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

* Re: [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
  2021-09-14 15:14 ` Jens Axboe
@ 2021-09-14 15:21   ` Pavel Begunkov
  0 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-09-14 15:21 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Franz-B . Tuneke

On 9/14/21 4:14 PM, Jens Axboe wrote:
> On 9/14/21 9:12 AM, Pavel Begunkov wrote:
>> It might be inconvenient that direct open/accept deviates from the
>> update semantics and fails if the slot is taken instead of removing a
>> file sitting there. Implement this auto-removal.
>>
>> Note that removal might need to allocate and so may fail. However, if an
>> empty slot is specified, it's guaraneed to not fail on the fd
>> installation side for valid userspace programs. It's needed for users
>> who can't tolerate such failures, e.g. accept where the other end
>> never retries.
> 
> Can you submit a liburing test case for this too?

Made some for v1, I just sent out them.

-- 
Pavel Begunkov

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

* Re: [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
  2021-09-14 15:12 [PATCH v2 5.15] io_uring: auto-removal for direct open/accept Pavel Begunkov
  2021-09-14 15:14 ` Jens Axboe
@ 2021-09-14 15:36 ` Jens Axboe
  2021-09-14 15:48   ` Pavel Begunkov
  1 sibling, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2021-09-14 15:36 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Franz-B . Tuneke

On 9/14/21 9:12 AM, Pavel Begunkov wrote:
> It might be inconvenient that direct open/accept deviates from the
> update semantics and fails if the slot is taken instead of removing a
> file sitting there. Implement this auto-removal.
> 
> Note that removal might need to allocate and so may fail. However, if an
> empty slot is specified, it's guaraneed to not fail on the fd
> installation side for valid userspace programs. It's needed for users
> who can't tolerate such failures, e.g. accept where the other end
> never retries.
> 
> Suggested-by: Franz-B. Tuneke <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
> 
> v2: simplify io_rsrc_node_switch_start() handling
> 
>  fs/io_uring.c | 52 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index a864a94364c6..58c0cbfdd128 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8287,11 +8287,27 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
>  #endif
>  }
>  
> +static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
> +				 struct io_rsrc_node *node, void *rsrc)
> +{
> +	struct io_rsrc_put *prsrc;
> +
> +	prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
> +	if (!prsrc)
> +		return -ENOMEM;
> +
> +	prsrc->tag = *io_get_tag_slot(data, idx);
> +	prsrc->rsrc = rsrc;
> +	list_add(&prsrc->list, &node->rsrc_list);
> +	return 0;
> +}

I know this code is just being moved, but I tend to like making the
expected/fast path inline:

prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
if (prsrc) {
	prsrc->tag = *io_get_tag_slot(data, idx);
	prsrc->rsrc = rsrc;
	list_add(&prsrc->list, &node->rsrc_list);
	return 0;
}
return -ENOMEM;

-- 
Jens Axboe


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

* Re: [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
  2021-09-14 15:36 ` Jens Axboe
@ 2021-09-14 15:48   ` Pavel Begunkov
  2021-09-14 15:50     ` Jens Axboe
  0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2021-09-14 15:48 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Franz-B . Tuneke

On 9/14/21 4:36 PM, Jens Axboe wrote:
> On 9/14/21 9:12 AM, Pavel Begunkov wrote:
>> It might be inconvenient that direct open/accept deviates from the
>> update semantics and fails if the slot is taken instead of removing a
>> file sitting there. Implement this auto-removal.
>>
>> Note that removal might need to allocate and so may fail. However, if an
>> empty slot is specified, it's guaraneed to not fail on the fd
>> installation side for valid userspace programs. It's needed for users
>> who can't tolerate such failures, e.g. accept where the other end
>> never retries.
>>
>> Suggested-by: Franz-B. Tuneke <[email protected]>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>> ---
>>
>> v2: simplify io_rsrc_node_switch_start() handling
>>
>>  fs/io_uring.c | 52 +++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index a864a94364c6..58c0cbfdd128 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -8287,11 +8287,27 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
>>  #endif
>>  }
>>  
>> +static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
>> +				 struct io_rsrc_node *node, void *rsrc)
>> +{
>> +	struct io_rsrc_put *prsrc;
>> +
>> +	prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
>> +	if (!prsrc)
>> +		return -ENOMEM;
>> +
>> +	prsrc->tag = *io_get_tag_slot(data, idx);
>> +	prsrc->rsrc = rsrc;
>> +	list_add(&prsrc->list, &node->rsrc_list);
>> +	return 0;
>> +}
> 
> I know this code is just being moved, but I tend to like making the
> expected/fast path inline:

I think it's more natural as now, as we always have

ret = do_something();
if (ret)
    return ret;
ret = do_something2();
if (ret)
    goto err;


And I remember you telling once "... I tend to like to do the error
path like that unless it's a hot path ...". So maybe just add
unlikely()?


> prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
> if (prsrc) {
> 	prsrc->tag = *io_get_tag_slot(data, idx);
> 	prsrc->rsrc = rsrc;
> 	list_add(&prsrc->list, &node->rsrc_list);
> 	return 0;
> }
> return -ENOMEM;
> 

-- 
Pavel Begunkov

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

* Re: [PATCH v2 5.15] io_uring: auto-removal for direct open/accept
  2021-09-14 15:48   ` Pavel Begunkov
@ 2021-09-14 15:50     ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2021-09-14 15:50 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Franz-B . Tuneke

On 9/14/21 9:48 AM, Pavel Begunkov wrote:
> On 9/14/21 4:36 PM, Jens Axboe wrote:
>> On 9/14/21 9:12 AM, Pavel Begunkov wrote:
>>> It might be inconvenient that direct open/accept deviates from the
>>> update semantics and fails if the slot is taken instead of removing a
>>> file sitting there. Implement this auto-removal.
>>>
>>> Note that removal might need to allocate and so may fail. However, if an
>>> empty slot is specified, it's guaraneed to not fail on the fd
>>> installation side for valid userspace programs. It's needed for users
>>> who can't tolerate such failures, e.g. accept where the other end
>>> never retries.
>>>
>>> Suggested-by: Franz-B. Tuneke <[email protected]>
>>> Signed-off-by: Pavel Begunkov <[email protected]>
>>> ---
>>>
>>> v2: simplify io_rsrc_node_switch_start() handling
>>>
>>>  fs/io_uring.c | 52 +++++++++++++++++++++++++++++++++------------------
>>>  1 file changed, 34 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index a864a94364c6..58c0cbfdd128 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -8287,11 +8287,27 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
>>>  #endif
>>>  }
>>>  
>>> +static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
>>> +				 struct io_rsrc_node *node, void *rsrc)
>>> +{
>>> +	struct io_rsrc_put *prsrc;
>>> +
>>> +	prsrc = kzalloc(sizeof(*prsrc), GFP_KERNEL);
>>> +	if (!prsrc)
>>> +		return -ENOMEM;
>>> +
>>> +	prsrc->tag = *io_get_tag_slot(data, idx);
>>> +	prsrc->rsrc = rsrc;
>>> +	list_add(&prsrc->list, &node->rsrc_list);
>>> +	return 0;
>>> +}
>>
>> I know this code is just being moved, but I tend to like making the
>> expected/fast path inline:
> 
> I think it's more natural as now, as we always have
> 
> ret = do_something();
> if (ret)
>     return ret;
> ret = do_something2();
> if (ret)
>     goto err;
> 
> 
> And I remember you telling once "... I tend to like to do the error
> path like that unless it's a hot path ...". So maybe just add
> unlikely()?

Yeah doesn't matter too much for a non-fast path, and unlikely() here
won't matter as well. Let's just use it as-is.

-- 
Jens Axboe


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

end of thread, other threads:[~2021-09-14 15:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-14 15:12 [PATCH v2 5.15] io_uring: auto-removal for direct open/accept Pavel Begunkov
2021-09-14 15:14 ` Jens Axboe
2021-09-14 15:21   ` Pavel Begunkov
2021-09-14 15:36 ` Jens Axboe
2021-09-14 15:48   ` Pavel Begunkov
2021-09-14 15:50     ` Jens Axboe

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