On Sat, Oct 26, 2019 at 12:22 AM Jens Axboe <axboe@xxxxxxxxx> 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?
> +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.
> - if (!ctx->user_files[fd])
> + file = io_file_from_index(ctx, fd);
> + if (!file)
> return -EBADF;
> - req->file = ctx->user_files[fd];
> + req->file = file;
> req->flags |= REQ_F_FIXED_FILE;
> } else {
> if (s->needs_fixed_file)
[...]
> +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;
> +
> + table->files = kcalloc(this_files, sizeof(struct file *),
> + GFP_KERNEL);
> + if (!table->files)
> + break;
> + }
> +
> + if (i == nr_tables)
> + return 0;
> +
> + for (i = 0; i < nr_tables; i++) {
> + struct fixed_file_table *table = &ctx->file_table[i];
> + kfree(table->files);
> + }
> + return 1;
> +}
