Re: [PATCH] io_uring: support for larger fixed file sets

[Jens Axboe <axboe@kernel.dk>]

On 10/25/19 5:54 PM, Jann Horn wrote:
> On Sat, Oct 26, 2019 at 12:22 AM Jens Axboe <axboe@kernel.dk> wrote:
>> There's been a few requests for supporting more fixed files than 1024.
>> This isn't really tricky to do, we just need to split up the file table
>> into multiple tables and index appropriately.
>>
>> This patch adds support for up to 64K files, which should be enough for
>> everyone.
> 
> What's the reason against vmalloc()? Performance of table reallocation?

Yeah, not that it's a super hot path, but single page allocs are always
going to be faster. And it's not a big deal to manage an index of tables.
I've changed the table shift to 9, which will actually improve the
reliability of the 1024 file case as well now, as every table will just
be a 4096b alloc.

>> +static inline struct file *io_file_from_index(struct io_ring_ctx *ctx,
>> +                                             int index)
>> +{
>> +       struct fixed_file_table *table;
>> +
>> +       table = &ctx->file_table[index >> IORING_FILE_TABLE_SHIFT];
>> +       return table->files[index & IORING_FILE_TABLE_MASK];
>> +}
> [...]
>> @@ -2317,12 +2334,15 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s,
>>                  return 0;
>>
>>          if (flags & IOSQE_FIXED_FILE) {
>> -               if (unlikely(!ctx->user_files ||
>> +               struct file *file;
>> +
>> +               if (unlikely(!ctx->file_table ||
>>                      (unsigned) fd >= ctx->nr_user_files))
>>                          return -EBADF;
> 
> Not a problem with this patch, but: By the way, the array lookup after
> this is a "array index using bounds-checked index" pattern, so
> array_index_nospec() might be appropriate here. See __fcheck_files()
> for comparison.

Yes, I think we should update that separately. I agree, should be
using the nospec indexer.

>> +static int io_sqe_alloc_file_tables(struct io_ring_ctx *ctx, unsigned nr_tables,
>> +                                   unsigned nr_files)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < nr_tables; i++) {
>> +               struct fixed_file_table *table = &ctx->file_table[i];
>> +               unsigned this_files;
>> +
>> +               this_files = nr_files;
>> +               if (this_files > IORING_MAX_FILES_TABLE)
>> +                       this_files = IORING_MAX_FILES_TABLE;
> 
> nr_files and this_files stay the same throughout the loop. I suspect
> that the intent here was something like:
> 
> this_files = min_t(unsigned, nr_files, IORING_MAX_FILES_TABLE);
> nr_files -= this_files;

Yes, I messed up the decrement, always make a mental note to do so,
often forget. I've fixed this up, thanks.

Made a few other cleanups, will send a v2.

-- 
Jens Axboe