[PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[noah]

From: noah <goldstein.n@wustl.edu>

This patch makes it so that specify a file descriptor value of -2 will
skip updating the corresponding fixed file index.

This will allow for users to reduce the number of syscalls necessary
to update a sparse file range when using the fixed file option.

Signed-off-by: noah <goldstein.w.n@gmail.com>
---
 fs/io_uring.c | 72 ++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6f9392c35eef..43ab2b7a87d4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7876,42 +7876,44 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 			err = -EFAULT;
 			break;
 		}
-		i = array_index_nospec(up->offset, ctx->nr_user_files);
-		table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
-		index = i & IORING_FILE_TABLE_MASK;
-		if (table->files[index]) {
-			file = table->files[index];
-			err = io_queue_file_removal(data, file);
-			if (err)
-				break;
-			table->files[index] = NULL;
-			needs_switch = true;
-		}
-		if (fd != -1) {
-			file = fget(fd);
-			if (!file) {
-				err = -EBADF;
-				break;
-			}
-			/*
-			 * Don't allow io_uring instances to be registered. If
-			 * UNIX isn't enabled, then this causes a reference
-			 * cycle and this instance can never get freed. If UNIX
-			 * is enabled we'll handle it just fine, but there's
-			 * still no point in allowing a ring fd as it doesn't
-			 * support regular read/write anyway.
-			 */
-			if (file->f_op == &io_uring_fops) {
-				fput(file);
-				err = -EBADF;
-				break;
-			}
-			table->files[index] = file;
-			err = io_sqe_file_register(ctx, file, i);
-			if (err) {
+		if (fd != -2) {
+			i = array_index_nospec(up->offset, ctx->nr_user_files);
+			table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
+			index = i & IORING_FILE_TABLE_MASK;
+			if (table->files[index]) {
+				file = table->files[index];
+				err = io_queue_file_removal(data, file);
+				if (err)
+					break;
 				table->files[index] = NULL;
-				fput(file);
-				break;
+				needs_switch = true;
+			}
+			if (fd != -1) {
+				file = fget(fd);
+				if (!file) {
+					err = -EBADF;
+					break;
+				}
+				/*
+				 * Don't allow io_uring instances to be registered. If
+				 * UNIX isn't enabled, then this causes a reference
+				 * cycle and this instance can never get freed. If UNIX
+				 * is enabled we'll handle it just fine, but there's
+				 * still no point in allowing a ring fd as it doesn't
+				 * support regular read/write anyway.
+				 */
+				if (file->f_op == &io_uring_fops) {
+					fput(file);
+					err = -EBADF;
+					break;
+				}
+				table->files[index] = file;
+				err = io_sqe_file_register(ctx, file, i);
+				if (err) {
+					table->files[index] = NULL;
+					fput(file);
+					break;
+				}
 			}
 		}
 		nr_args--;
-- 
2.29.2

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Pavel Begunkov]

On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
> 
> This patch makes it so that specify a file descriptor value of -2 will
> skip updating the corresponding fixed file index.
> 
> This will allow for users to reduce the number of syscalls necessary
> to update a sparse file range when using the fixed file option.

Answering the github thread -- it's indeed a simple change, I had it the
same day you posted the issue. See below it's a bit cleaner. However, I
want to first review "io_uring: buffer registration enhancements", and
if it's good, for easier merging/etc I'd rather prefer to let it go
first (even if partially).

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 941fe9b64fd9..b3ae9d5da17e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7847,9 +7847,8 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 	if (IS_ERR(ref_node))
 		return PTR_ERR(ref_node);
 
-	done = 0;
 	fds = u64_to_user_ptr(up->fds);
-	while (nr_args) {
+	for (done = 0; done < nr_args; done++) {
 		struct fixed_file_table *table;
 		unsigned index;
 
@@ -7858,7 +7857,10 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 			err = -EFAULT;
 			break;
 		}
-		i = array_index_nospec(up->offset, ctx->nr_user_files);
+		if (fd == IORING_REGISTER_FILES_SKIP)
+			continue;
+
+		i = array_index_nospec(up->offset + done, ctx->nr_user_files);
 		table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
 		index = i & IORING_FILE_TABLE_MASK;
 		if (table->files[index]) {
@@ -7896,9 +7898,6 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				break;
 			}
 		}
-		nr_args--;
-		done++;
-		up->offset++;
 	}
 
 	if (needs_switch) {

-- 
Pavel Begunkov

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Noah Goldstein]

On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
> > 
> > This patch makes it so that specify a file descriptor value of -2 will
> > skip updating the corresponding fixed file index.
> > 
> > This will allow for users to reduce the number of syscalls necessary
> > to update a sparse file range when using the fixed file option.
> 
> Answering the github thread -- it's indeed a simple change, I had it the
> same day you posted the issue. See below it's a bit cleaner. However, I
> want to first review "io_uring: buffer registration enhancements", and
> if it's good, for easier merging/etc I'd rather prefer to let it go
> first (even if partially).
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 941fe9b64fd9..b3ae9d5da17e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -7847,9 +7847,8 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>  	if (IS_ERR(ref_node))
>  		return PTR_ERR(ref_node);
>  
> -	done = 0;
>  	fds = u64_to_user_ptr(up->fds);
> -	while (nr_args) {
> +	for (done = 0; done < nr_args; done++) {
>  		struct fixed_file_table *table;
>  		unsigned index;
>  
> @@ -7858,7 +7857,10 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>  			err = -EFAULT;
>  			break;
>  		}
> -		i = array_index_nospec(up->offset, ctx->nr_user_files);
> +		if (fd == IORING_REGISTER_FILES_SKIP)
> +			continue;
> +
> +		i = array_index_nospec(up->offset + done, ctx->nr_user_files);
>  		table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
>  		index = i & IORING_FILE_TABLE_MASK;
>  		if (table->files[index]) {
> @@ -7896,9 +7898,6 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>  				break;
>  			}
>  		}
> -		nr_args--;
> -		done++;
> -		up->offset++;
>  	}
>  
>  	if (needs_switch) {
> 
> -- 
> Pavel Begunkov
Ah. Got it.

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Pavel Begunkov]

On 22/12/2020 02:10, Noah Goldstein wrote:
> On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
>> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
>>>
>>> This patch makes it so that specify a file descriptor value of -2 will
>>> skip updating the corresponding fixed file index.
>>>
>>> This will allow for users to reduce the number of syscalls necessary
>>> to update a sparse file range when using the fixed file option.
>>
>> Answering the github thread -- it's indeed a simple change, I had it the
>> same day you posted the issue. See below it's a bit cleaner. However, I
>> want to first review "io_uring: buffer registration enhancements", and
>> if it's good, for easier merging/etc I'd rather prefer to let it go
>> first (even if partially).

Noah, want to give it a try? I've just sent a prep patch, with it you
can implement it cleaner with one continue.

>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 941fe9b64fd9..b3ae9d5da17e 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -7847,9 +7847,8 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>>  	if (IS_ERR(ref_node))
>>  		return PTR_ERR(ref_node);
>>  
>> -	done = 0;
>>  	fds = u64_to_user_ptr(up->fds);
>> -	while (nr_args) {
>> +	for (done = 0; done < nr_args; done++) {
>>  		struct fixed_file_table *table;
>>  		unsigned index;
>>  
>> @@ -7858,7 +7857,10 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>>  			err = -EFAULT;
>>  			break;
>>  		}
>> -		i = array_index_nospec(up->offset, ctx->nr_user_files);
>> +		if (fd == IORING_REGISTER_FILES_SKIP)
>> +			continue;
>> +
>> +		i = array_index_nospec(up->offset + done, ctx->nr_user_files);
>>  		table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
>>  		index = i & IORING_FILE_TABLE_MASK;
>>  		if (table->files[index]) {
>> @@ -7896,9 +7898,6 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>>  				break;
>>  			}
>>  		}
>> -		nr_args--;
>> -		done++;
>> -		up->offset++;
>>  	}
>>  
>>  	if (needs_switch) {

-- 
Pavel Begunkov

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Noah Goldstein]

On Tue, Jan 26, 2021 at 7:29 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 22/12/2020 02:10, Noah Goldstein wrote:
> > On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
> >> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
> >>>
> >>> This patch makes it so that specify a file descriptor value of -2 will
> >>> skip updating the corresponding fixed file index.
> >>>
> >>> This will allow for users to reduce the number of syscalls necessary
> >>> to update a sparse file range when using the fixed file option.
> >>
> >> Answering the github thread -- it's indeed a simple change, I had it the
> >> same day you posted the issue. See below it's a bit cleaner. However, I
> >> want to first review "io_uring: buffer registration enhancements", and
> >> if it's good, for easier merging/etc I'd rather prefer to let it go
> >> first (even if partially).
>
> Noah, want to give it a try? I've just sent a prep patch, with it you
> can implement it cleaner with one continue.
>

 Absolutely. Will get on it ASAP.

> >>
> >> diff --git a/fs/io_uring.c b/fs/io_uring.c
> >> index 941fe9b64fd9..b3ae9d5da17e 100644
> >> --- a/fs/io_uring.c
> >> +++ b/fs/io_uring.c
> >> @@ -7847,9 +7847,8 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
> >>      if (IS_ERR(ref_node))
> >>              return PTR_ERR(ref_node);
> >>
> >> -    done = 0;
> >>      fds = u64_to_user_ptr(up->fds);
> >> -    while (nr_args) {
> >> +    for (done = 0; done < nr_args; done++) {
> >>              struct fixed_file_table *table;
> >>              unsigned index;
> >>
> >> @@ -7858,7 +7857,10 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
> >>                      err = -EFAULT;
> >>                      break;
> >>              }
> >> -            i = array_index_nospec(up->offset, ctx->nr_user_files);
> >> +            if (fd == IORING_REGISTER_FILES_SKIP)
> >> +                    continue;
> >> +
> >> +            i = array_index_nospec(up->offset + done, ctx->nr_user_files);
> >>              table = &ctx->file_data->table[i >> IORING_FILE_TABLE_SHIFT];
> >>              index = i & IORING_FILE_TABLE_MASK;
> >>              if (table->files[index]) {
> >> @@ -7896,9 +7898,6 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
> >>                              break;
> >>                      }
> >>              }
> >> -            nr_args--;
> >> -            done++;
> >> -            up->offset++;
> >>      }
> >>
> >>      if (needs_switch) {
>
> --
> Pavel Begunkov

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Pavel Begunkov]

On 26/01/2021 17:14, Noah Goldstein wrote:
> On Tue, Jan 26, 2021 at 7:29 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 22/12/2020 02:10, Noah Goldstein wrote:
>>> On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
>>>> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
>>>>>
>>>>> This patch makes it so that specify a file descriptor value of -2 will
>>>>> skip updating the corresponding fixed file index.
>>>>>
>>>>> This will allow for users to reduce the number of syscalls necessary
>>>>> to update a sparse file range when using the fixed file option.
>>>>
>>>> Answering the github thread -- it's indeed a simple change, I had it the
>>>> same day you posted the issue. See below it's a bit cleaner. However, I
>>>> want to first review "io_uring: buffer registration enhancements", and
>>>> if it's good, for easier merging/etc I'd rather prefer to let it go
>>>> first (even if partially).
>>
>> Noah, want to give it a try? I've just sent a prep patch, with it you
>> can implement it cleaner with one continue.
> 
>  Absolutely. Will get on it ASAP.

Perfect. Even better if you add a liburing test

-- 
Pavel Begunkov

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Noah Goldstein]

On Tue, Jan 26, 2021 at 12:24 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 26/01/2021 17:14, Noah Goldstein wrote:
> > On Tue, Jan 26, 2021 at 7:29 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >>
> >> On 22/12/2020 02:10, Noah Goldstein wrote:
> >>> On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
> >>>> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
> >>>>>
> >>>>> This patch makes it so that specify a file descriptor value of -2 will
> >>>>> skip updating the corresponding fixed file index.
> >>>>>
> >>>>> This will allow for users to reduce the number of syscalls necessary
> >>>>> to update a sparse file range when using the fixed file option.
> >>>>
> >>>> Answering the github thread -- it's indeed a simple change, I had it the
> >>>> same day you posted the issue. See below it's a bit cleaner. However, I
> >>>> want to first review "io_uring: buffer registration enhancements", and
> >>>> if it's good, for easier merging/etc I'd rather prefer to let it go
> >>>> first (even if partially).
> >>
> >> Noah, want to give it a try? I've just sent a prep patch, with it you
> >> can implement it cleaner with one continue.
> >
> >  Absolutely. Will get on it ASAP.
>
> Perfect. Even better if you add a liburing test
>
> --
> Pavel Begunkov

Do you think the return value should not include files skipped?

i.e register fds[1, 2, 3, -1] with no errors returns 4. should fds[1,
2, -2, -1] return 3 or 4 do you think?

Personally think the latter makes more sense. Thoughts?

Re: [PATCH] fs: io_uring.c: Add skip option for __io_sqe_files_update

[Pavel Begunkov]

On 26/01/2021 18:43, Noah Goldstein wrote:
> On Tue, Jan 26, 2021 at 12:24 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 26/01/2021 17:14, Noah Goldstein wrote:
>>> On Tue, Jan 26, 2021 at 7:29 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>>>
>>>> On 22/12/2020 02:10, Noah Goldstein wrote:
>>>>> On Sun, Dec 20, 2020 at 03:18:05PM +0000, Pavel Begunkov wrote:
>>>>>> On 20/12/2020 06:50, noah wrote:> From: noah <goldstein.n@wustl.edu>
>>>>>>>
>>>>>>> This patch makes it so that specify a file descriptor value of -2 will
>>>>>>> skip updating the corresponding fixed file index.
>>>>>>>
>>>>>>> This will allow for users to reduce the number of syscalls necessary
>>>>>>> to update a sparse file range when using the fixed file option.
>>>>>>
>>>>>> Answering the github thread -- it's indeed a simple change, I had it the
>>>>>> same day you posted the issue. See below it's a bit cleaner. However, I
>>>>>> want to first review "io_uring: buffer registration enhancements", and
>>>>>> if it's good, for easier merging/etc I'd rather prefer to let it go
>>>>>> first (even if partially).
>>>>
>>>> Noah, want to give it a try? I've just sent a prep patch, with it you
>>>> can implement it cleaner with one continue.
>>>
>>>  Absolutely. Will get on it ASAP.
>>
>> Perfect. Even better if you add a liburing test
> 
> Do you think the return value should not include files skipped?
> 
> i.e register fds[1, 2, 3, -1] with no errors returns 4. should fds[1,
> 2, -2, -1] return 3 or 4 do you think?
> 
> Personally think the latter makes more sense. Thoughts?

Let's just return @done, 4 in your case. Because otherwise locating which
index has failed would be hell. And it's consistent with delete (i.e. -1).

-- 
Pavel Begunkov