Re: [LIBURING PATCH] Let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Jens Axboe <axboe@kernel.dk>]

On 5/30/22 6:48 AM, Xiaoguang Wang wrote:
> Allocate available direct descriptors instead of having the
> application pass free fixed file slots. To use it, pass
> IORING_FILE_INDEX_ALLOC to io_uring_prep_files_update(), then
> io_uring in kernel will store picked fixed file slots in fd
> array and let cqe return the number of slots allocated.

Ah thanks, didn't see this before replying. A few minor comments:

> diff --git a/src/include/liburing.h b/src/include/liburing.h
> index 6429dff..9b95ad5 100644
> --- a/src/include/liburing.h
> +++ b/src/include/liburing.h
> @@ -614,6 +614,14 @@ static inline void io_uring_prep_close_direct(struct io_uring_sqe *sqe,
>  	__io_uring_set_target_fixed_file(sqe, file_index);
>  }
>  
> +static inline void io_uring_prep_close_all(struct io_uring_sqe *sqe,
> +					   int fd, unsigned file_index)
> +{
> +	io_uring_prep_close(sqe, fd);
> +	__io_uring_set_target_fixed_file(sqe, file_index);
> +	sqe->close_flags = 1;
> +}

This needs a man page addition as well to io_uring_prep_close.3.

> diff --git a/test/file-update-index-alloc.c b/test/file-update-index-alloc.c
> new file mode 100644
> index 0000000..774cbb5
> --- /dev/null
> +++ b/test/file-update-index-alloc.c
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Description: test IORING_OP_FILES_UPDATE can support io_uring
> + * allocates an available direct descriptor instead of having the
> + * application pass one.
> + */
> +
> +#include <errno.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <sys/uio.h>
> +
> +#include "helpers.h"
> +#include "liburing.h"
> +
> +int main(int argc, char *argv[])
> +{
> +	struct io_uring_cqe *cqe;
> +	struct io_uring_sqe *sqe;
> +	char wbuf[1] = { 0xef }, rbuf[1] = {0x0};
> +	struct io_uring ring;
> +	int i, ret, pipe_fds[2], fds[2] = { -1, -1};
> +
> +	ret = io_uring_queue_init(8, &ring, 0);
> +	if (ret) {
> +		fprintf(stderr, "ring setup failed\n");
> +		return -1;
> +	}
> +
> +	ret = io_uring_register_files(&ring, fds, 2);
> +	if (ret) {
> +		fprintf(stderr, "%s: register ret=%d\n", __func__, ret);
> +		return -1;
> +	}
> +
> +	if (pipe2(pipe_fds, O_NONBLOCK)) {
> +		fprintf(stderr, "pipe() failed\n");
> +		return -1;
> +	}
> +
> +	/*
> +	 * Pass IORING_FILE_INDEX_ALLOC, so io_uring in kernel will allocate
> +	 * available direct descriptors.
> +	 */
> +	fds[0] = pipe_fds[0];
> +	fds[1] = pipe_fds[1];
> +	sqe = io_uring_get_sqe(&ring);
> +	io_uring_prep_files_update(sqe, fds, 2, IORING_FILE_INDEX_ALLOC);
> +	ret = io_uring_submit(&ring);
> +	if (ret != 1) {
> +		fprintf(stderr, "sqe submit failed: %d\n", ret);
> +		return -1;
> +	}
> +	ret = io_uring_wait_cqe(&ring, &cqe);
> +	if (ret < 0 || cqe->res < 0) {
> +		fprintf(stderr, "io_uring_prep_files_update failed: %d\n", ret);
> +		return ret;
> +	}

If cqe->res == -EINVAL, then the feature isn't supported. We should not
fail the test for that, we should just skip it and return 0. Otherwise
this test case will fail on older kernels, which is annoying noise.

Apart from that, test case looks good, and it's nice that it also uses
the fd post updating to ensure everything is sane.

-- 
Jens Axboe