public inbox for [email protected]
 help / color / mirror / Atom feed
* [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
@ 2022-05-26 12:38 Xiaoguang Wang
  2022-05-27  5:54 ` Hao Xu
  2022-05-28 12:27 ` Jens Axboe
  0 siblings, 2 replies; 7+ messages in thread
From: Xiaoguang Wang @ 2022-05-26 12:38 UTC (permalink / raw)
  To: io-uring; +Cc: axboe, asml.silence

One big issue with file registration feature is that it needs user
space apps to maintain free slot info about io_uring's fixed file
table, which really is a burden for development. Now since io_uring
starts to choose free file slot for user space apps by using
IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
need app to uses direct accept or direct open, which as far as I know,
some apps are not prepared to use direct accept or open yet.

To support apps, who still need real fds, use registration feature
easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
which will return free file slot in cqe->res.

TODO list:
    Need to prepare liburing corresponding helpers.

Signed-off-by: Xiaoguang Wang <[email protected]>
---
 fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9f1c682d7caf..d77e6bbec81c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -680,6 +680,7 @@ struct io_rsrc_update {
 	u64				arg;
 	u32				nr_args;
 	u32				offset;
+	u32				flags;
 };
 
 struct io_fadvise {
@@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
 	return 0;
 }
 
+#define IORING_FILES_UPDATE_INDEX_ALLOC 1
+
 static int io_rsrc_update_prep(struct io_kiocb *req,
 				const struct io_uring_sqe *sqe)
 {
+	u32 flags = READ_ONCE(sqe->files_update_flags);
+
 	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
 		return -EINVAL;
-	if (sqe->rw_flags || sqe->splice_fd_in)
+	if (sqe->splice_fd_in)
+		return -EINVAL;
+	if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
+		return -EINVAL;
+	if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)
 		return -EINVAL;
 
+	req->rsrc_update.flags = flags;
 	req->rsrc_update.offset = READ_ONCE(sqe->off);
 	req->rsrc_update.nr_args = READ_ONCE(sqe->len);
 	if (!req->rsrc_update.nr_args)
@@ -7990,18 +8000,38 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_uring_rsrc_update2 up;
+	struct file *file;
 	int ret;
 
-	up.offset = req->rsrc_update.offset;
-	up.data = req->rsrc_update.arg;
-	up.nr = 0;
-	up.tags = 0;
-	up.resv = 0;
-	up.resv2 = 0;
+	if (req->rsrc_update.flags & IORING_FILES_UPDATE_INDEX_ALLOC) {
+		int fd;
 
-	io_ring_submit_lock(ctx, issue_flags);
-	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
-					&up, req->rsrc_update.nr_args);
+		if (copy_from_user(&fd, (int *)req->rsrc_update.arg, sizeof(fd))) {
+			ret = -EFAULT;
+			goto out;
+		}
+		file = fget(fd);
+		if (!file) {
+			ret = -EBADF;
+			goto out;
+		}
+		ret = io_fixed_fd_install(req, issue_flags, file,
+					  IORING_FILE_INDEX_ALLOC);
+	} else {
+		up.offset = req->rsrc_update.offset;
+		up.data = req->rsrc_update.arg;
+		up.nr = 0;
+		up.tags = 0;
+		up.resv = 0;
+		up.resv2 = 0;
+
+		io_ring_submit_lock(ctx, issue_flags);
+		ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
+				&up, req->rsrc_update.nr_args);
+		io_ring_submit_unlock(ctx, issue_flags);
+	}
+
+out:
 	io_ring_submit_unlock(ctx, issue_flags);
 
 	if (ret < 0)
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 53e7dae92e42..b4af01d56d7c 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -47,6 +47,7 @@ struct io_uring_sqe {
 		__u32		unlink_flags;
 		__u32		hardlink_flags;
 		__u32		xattr_flags;
+		__u32		files_update_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
-- 
2.14.4.44.g2045bb6


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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-26 12:38 [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot Xiaoguang Wang
@ 2022-05-27  5:54 ` Hao Xu
  2022-05-28  9:45   ` Xiaoguang Wang
  2022-05-28 12:27 ` Jens Axboe
  1 sibling, 1 reply; 7+ messages in thread
From: Hao Xu @ 2022-05-27  5:54 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: axboe, asml.silence

Hi Xiaoguang,

On 5/26/22 20:38, Xiaoguang Wang wrote:
> One big issue with file registration feature is that it needs user
> space apps to maintain free slot info about io_uring's fixed file
> table, which really is a burden for development. Now since io_uring
> starts to choose free file slot for user space apps by using
> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
> need app to uses direct accept or direct open, which as far as I know,
> some apps are not prepared to use direct accept or open yet.
> 
> To support apps, who still need real fds, use registration feature
> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
> which will return free file slot in cqe->res.
> 
> TODO list:
>      Need to prepare liburing corresponding helpers.
> 
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>   include/uapi/linux/io_uring.h |  1 +
>   2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 9f1c682d7caf..d77e6bbec81c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>   	u64				arg;
>   	u32				nr_args;
>   	u32				offset;
> +	u32				flags;
>   };
>   
>   struct io_fadvise {
> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   	return 0;
>   }
>   
> +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
> +
>   static int io_rsrc_update_prep(struct io_kiocb *req,
>   				const struct io_uring_sqe *sqe)
>   {
> +	u32 flags = READ_ONCE(sqe->files_update_flags);
> +
>   	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>   		return -EINVAL;
> -	if (sqe->rw_flags || sqe->splice_fd_in)
> +	if (sqe->splice_fd_in)
> +		return -EINVAL;
> +	if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
> +		return -EINVAL;
> +	if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)

How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
case? For example, using the sqe->addr(the fd array) to store the slots 
we allocated, and let cqe return the number of slots allocated.
By the way, another way, we can levarage up->offset == 
IORING_FILE_INDEX_ALLOC
to do the mode check since seems it is not used in this mode. Though
I'm not sure that is better..

>   		return -EINVAL;
>   
> +	req->rsrc_update.flags = flags;
>   	req->rsrc_update.offset = READ_ONCE(sqe->off);
>   	req->rsrc_update.nr_args = READ_ONCE(sqe->len);
>   	if (!req->rsrc_update.nr_args)
> @@ -7990,18 +8000,38 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_uring_rsrc_update2 up;
> +	struct file *file;
>   	int ret;
>   
> -	up.offset = req->rsrc_update.offset;
> -	up.data = req->rsrc_update.arg;
> -	up.nr = 0;
> -	up.tags = 0;
> -	up.resv = 0;
> -	up.resv2 = 0;
> +	if (req->rsrc_update.flags & IORING_FILES_UPDATE_INDEX_ALLOC) {
> +		int fd;
>   
> -	io_ring_submit_lock(ctx, issue_flags);
> -	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
> -					&up, req->rsrc_update.nr_args);
> +		if (copy_from_user(&fd, (int *)req->rsrc_update.arg, sizeof(fd))) {

                                           ^ (void __user *) ?

Regards,
Hao

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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-27  5:54 ` Hao Xu
@ 2022-05-28  9:45   ` Xiaoguang Wang
  2022-05-28 12:28     ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Xiaoguang Wang @ 2022-05-28  9:45 UTC (permalink / raw)
  To: Hao Xu, io-uring; +Cc: axboe, asml.silence

hi Hao,

> Hi Xiaoguang,
>
> On 5/26/22 20:38, Xiaoguang Wang wrote:
>> One big issue with file registration feature is that it needs user
>> space apps to maintain free slot info about io_uring's fixed file
>> table, which really is a burden for development. Now since io_uring
>> starts to choose free file slot for user space apps by using
>> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
>> need app to uses direct accept or direct open, which as far as I know,
>> some apps are not prepared to use direct accept or open yet.
>>
>> To support apps, who still need real fds, use registration feature
>> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
>> which will return free file slot in cqe->res.
>>
>> TODO list:
>>      Need to prepare liburing corresponding helpers.
>>
>> Signed-off-by: Xiaoguang Wang <[email protected]>
>> ---
>>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>>   include/uapi/linux/io_uring.h |  1 +
>>   2 files changed, 41 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 9f1c682d7caf..d77e6bbec81c 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>>       u64                arg;
>>       u32                nr_args;
>>       u32                offset;
>> +    u32                flags;
>>   };
>>     struct io_fadvise {
>> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>>       return 0;
>>   }
>>   +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
>> +
>>   static int io_rsrc_update_prep(struct io_kiocb *req,
>>                   const struct io_uring_sqe *sqe)
>>   {
>> +    u32 flags = READ_ONCE(sqe->files_update_flags);
>> +
>>       if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>>           return -EINVAL;
>> -    if (sqe->rw_flags || sqe->splice_fd_in)
>> +    if (sqe->splice_fd_in)
>> +        return -EINVAL;
>> +    if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
>> +        return -EINVAL;
>> +    if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)
>
> How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
> case? For example, using the sqe->addr(the fd array) to store the slots we allocated, and let cqe return the number of slots allocated.
Good idea, I'll try in patch v2, thanks.
Jens, any comments about this patch? At least It's really helpful to our
internal apps based on io_uring :)

Regards,
Xiaoguang Wang

> By the way, another way, we can levarage up->offset == IORING_FILE_INDEX_ALLOC
> to do the mode check since seems it is not used in this mode. Though
> I'm not sure that is better..
>
>>           return -EINVAL;
>>   +    req->rsrc_update.flags = flags;
>>       req->rsrc_update.offset = READ_ONCE(sqe->off);
>>       req->rsrc_update.nr_args = READ_ONCE(sqe->len);
>>       if (!req->rsrc_update.nr_args)
>> @@ -7990,18 +8000,38 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>   {
>>       struct io_ring_ctx *ctx = req->ctx;
>>       struct io_uring_rsrc_update2 up;
>> +    struct file *file;
>>       int ret;
>>   -    up.offset = req->rsrc_update.offset;
>> -    up.data = req->rsrc_update.arg;
>> -    up.nr = 0;
>> -    up.tags = 0;
>> -    up.resv = 0;
>> -    up.resv2 = 0;
>> +    if (req->rsrc_update.flags & IORING_FILES_UPDATE_INDEX_ALLOC) {
>> +        int fd;
>>   -    io_ring_submit_lock(ctx, issue_flags);
>> -    ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>> -                    &up, req->rsrc_update.nr_args);
>> +        if (copy_from_user(&fd, (int *)req->rsrc_update.arg, sizeof(fd))) {
>
>                                           ^ (void __user *) ?
>
> Regards,
> Hao


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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-26 12:38 [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot Xiaoguang Wang
  2022-05-27  5:54 ` Hao Xu
@ 2022-05-28 12:27 ` Jens Axboe
  1 sibling, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2022-05-28 12:27 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: asml.silence

On 5/26/22 6:38 AM, Xiaoguang Wang wrote:
> One big issue with file registration feature is that it needs user
> space apps to maintain free slot info about io_uring's fixed file
> table, which really is a burden for development. Now since io_uring
> starts to choose free file slot for user space apps by using
> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
> need app to uses direct accept or direct open, which as far as I know,
> some apps are not prepared to use direct accept or open yet.
> 
> To support apps, who still need real fds, use registration feature
> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
> which will return free file slot in cqe->res.

This looks good. In retrospect, the direct open/accept/etc really
should've just returned the direct descriptor in cqe->res, so we would
not have this odd "0 for success when you pick a slot, <slot> in
cqe->res if io_uring picks it". But this is consistent with the alloc
case.

Do you have a liburing test case too? I think we should just get this
done for 5.19 rather than spread it over multiple releases, since 5.19
introduced the alloc as well.

-- 
Jens Axboe


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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-28  9:45   ` Xiaoguang Wang
@ 2022-05-28 12:28     ` Jens Axboe
  2022-05-29 16:57       ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2022-05-28 12:28 UTC (permalink / raw)
  To: Xiaoguang Wang, Hao Xu, io-uring; +Cc: asml.silence

On 5/28/22 3:45 AM, Xiaoguang Wang wrote:
> hi Hao,
> 
>> Hi Xiaoguang,
>>
>> On 5/26/22 20:38, Xiaoguang Wang wrote:
>>> One big issue with file registration feature is that it needs user
>>> space apps to maintain free slot info about io_uring's fixed file
>>> table, which really is a burden for development. Now since io_uring
>>> starts to choose free file slot for user space apps by using
>>> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
>>> need app to uses direct accept or direct open, which as far as I know,
>>> some apps are not prepared to use direct accept or open yet.
>>>
>>> To support apps, who still need real fds, use registration feature
>>> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
>>> which will return free file slot in cqe->res.
>>>
>>> TODO list:
>>>      Need to prepare liburing corresponding helpers.
>>>
>>> Signed-off-by: Xiaoguang Wang <[email protected]>
>>> ---
>>>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>>>   include/uapi/linux/io_uring.h |  1 +
>>>   2 files changed, 41 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 9f1c682d7caf..d77e6bbec81c 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>>>       u64                arg;
>>>       u32                nr_args;
>>>       u32                offset;
>>> +    u32                flags;
>>>   };
>>>     struct io_fadvise {
>>> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>>>       return 0;
>>>   }
>>>   +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
>>> +
>>>   static int io_rsrc_update_prep(struct io_kiocb *req,
>>>                   const struct io_uring_sqe *sqe)
>>>   {
>>> +    u32 flags = READ_ONCE(sqe->files_update_flags);
>>> +
>>>       if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>>>           return -EINVAL;
>>> -    if (sqe->rw_flags || sqe->splice_fd_in)
>>> +    if (sqe->splice_fd_in)
>>> +        return -EINVAL;
>>> +    if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
>>> +        return -EINVAL;
>>> +    if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)
>>
>> How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
>> case? For example, using the sqe->addr(the fd array) to store the slots we allocated, and let cqe return the number of slots allocated.
> Good idea, I'll try in patch v2, thanks.
> Jens, any comments about this patch? At least It's really helpful to our
> internal apps based on io_uring :)

I like this suggestion too, other thoughts in reply to the original.

-- 
Jens Axboe


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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-28 12:28     ` Jens Axboe
@ 2022-05-29 16:57       ` Jens Axboe
  2022-05-30  2:47         ` Xiaoguang Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2022-05-29 16:57 UTC (permalink / raw)
  To: Xiaoguang Wang, Hao Xu, io-uring; +Cc: asml.silence

On 5/28/22 6:28 AM, Jens Axboe wrote:
> On 5/28/22 3:45 AM, Xiaoguang Wang wrote:
>> hi Hao,
>>
>>> Hi Xiaoguang,
>>>
>>> On 5/26/22 20:38, Xiaoguang Wang wrote:
>>>> One big issue with file registration feature is that it needs user
>>>> space apps to maintain free slot info about io_uring's fixed file
>>>> table, which really is a burden for development. Now since io_uring
>>>> starts to choose free file slot for user space apps by using
>>>> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
>>>> need app to uses direct accept or direct open, which as far as I know,
>>>> some apps are not prepared to use direct accept or open yet.
>>>>
>>>> To support apps, who still need real fds, use registration feature
>>>> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
>>>> which will return free file slot in cqe->res.
>>>>
>>>> TODO list:
>>>>      Need to prepare liburing corresponding helpers.
>>>>
>>>> Signed-off-by: Xiaoguang Wang <[email protected]>
>>>> ---
>>>>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>>>>   include/uapi/linux/io_uring.h |  1 +
>>>>   2 files changed, 41 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 9f1c682d7caf..d77e6bbec81c 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>>>>       u64                arg;
>>>>       u32                nr_args;
>>>>       u32                offset;
>>>> +    u32                flags;
>>>>   };
>>>>     struct io_fadvise {
>>>> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>>>>       return 0;
>>>>   }
>>>>   +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
>>>> +
>>>>   static int io_rsrc_update_prep(struct io_kiocb *req,
>>>>                   const struct io_uring_sqe *sqe)
>>>>   {
>>>> +    u32 flags = READ_ONCE(sqe->files_update_flags);
>>>> +
>>>>       if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>>>>           return -EINVAL;
>>>> -    if (sqe->rw_flags || sqe->splice_fd_in)
>>>> +    if (sqe->splice_fd_in)
>>>> +        return -EINVAL;
>>>> +    if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
>>>> +        return -EINVAL;
>>>> +    if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)
>>>
>>> How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
>>> case? For example, using the sqe->addr(the fd array) to store the slots we allocated, and let cqe return the number of slots allocated.
>> Good idea, I'll try in patch v2, thanks.
>> Jens, any comments about this patch? At least It's really helpful to our
>> internal apps based on io_uring :)
> 
> I like this suggestion too, other thoughts in reply to the original.

BTW, if you have time, would be great to get this done for 5.19. It
makes the whole thing more consistent and makes it so that 5.19 has
(hopefully) all the alloc bits for direct descriptors.

-- 
Jens Axboe


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

* Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot
  2022-05-29 16:57       ` Jens Axboe
@ 2022-05-30  2:47         ` Xiaoguang Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Xiaoguang Wang @ 2022-05-30  2:47 UTC (permalink / raw)
  To: Jens Axboe, Hao Xu, io-uring; +Cc: asml.silence

hi,

> On 5/28/22 6:28 AM, Jens Axboe wrote:
>> On 5/28/22 3:45 AM, Xiaoguang Wang wrote:
>>> hi Hao,
>>>
>>>> Hi Xiaoguang,
>>>>
>>>> On 5/26/22 20:38, Xiaoguang Wang wrote:
>>>>> One big issue with file registration feature is that it needs user
>>>>> space apps to maintain free slot info about io_uring's fixed file
>>>>> table, which really is a burden for development. Now since io_uring
>>>>> starts to choose free file slot for user space apps by using
>>>>> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
>>>>> need app to uses direct accept or direct open, which as far as I know,
>>>>> some apps are not prepared to use direct accept or open yet.
>>>>>
>>>>> To support apps, who still need real fds, use registration feature
>>>>> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
>>>>> which will return free file slot in cqe->res.
>>>>>
>>>>> TODO list:
>>>>>      Need to prepare liburing corresponding helpers.
>>>>>
>>>>> Signed-off-by: Xiaoguang Wang <[email protected]>
>>>>> ---
>>>>>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>>>>>   include/uapi/linux/io_uring.h |  1 +
>>>>>   2 files changed, 41 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index 9f1c682d7caf..d77e6bbec81c 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>>>>>       u64                arg;
>>>>>       u32                nr_args;
>>>>>       u32                offset;
>>>>> +    u32                flags;
>>>>>   };
>>>>>     struct io_fadvise {
>>>>> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>>>>>       return 0;
>>>>>   }
>>>>>   +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
>>>>> +
>>>>>   static int io_rsrc_update_prep(struct io_kiocb *req,
>>>>>                   const struct io_uring_sqe *sqe)
>>>>>   {
>>>>> +    u32 flags = READ_ONCE(sqe->files_update_flags);
>>>>> +
>>>>>       if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>>>>>           return -EINVAL;
>>>>> -    if (sqe->rw_flags || sqe->splice_fd_in)
>>>>> +    if (sqe->splice_fd_in)
>>>>> +        return -EINVAL;
>>>>> +    if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
>>>>> +        return -EINVAL;
>>>>> +    if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)
>>>> How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
>>>> case? For example, using the sqe->addr(the fd array) to store the slots we allocated, and let cqe return the number of slots allocated.
>>> Good idea, I'll try in patch v2, thanks.
>>> Jens, any comments about this patch? At least It's really helpful to our
>>> internal apps based on io_uring :)
>> I like this suggestion too, other thoughts in reply to the original.
> BTW, if you have time, would be great to get this done for 5.19. It
> makes the whole thing more consistent and makes it so that 5.19 has
> (hopefully) all the alloc bits for direct descriptors.
Yeah, I have free time now, will prepare new version today.

Regards,
Xiaoguang Wang

>


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

end of thread, other threads:[~2022-05-30  2:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-26 12:38 [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot Xiaoguang Wang
2022-05-27  5:54 ` Hao Xu
2022-05-28  9:45   ` Xiaoguang Wang
2022-05-28 12:28     ` Jens Axboe
2022-05-29 16:57       ` Jens Axboe
2022-05-30  2:47         ` Xiaoguang Wang
2022-05-28 12:27 ` Jens Axboe

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