Exhausting file descriptors when direct variants are used

[cmazakas]

I'm using the direct variant of multishot accept, limiting the ring to only 10 file descriptors by only registering a sparse set of 10 files.

However, I've noticed that in my unit test, if I want to accept more than 10 connections, io_uring_prep_close_direct() doesn't seem to mark those previously used entries as open for multishot accept()!

I'm also using io_uring_prep_socket_direct() as well and that seems fine to reuse the descriptors. I'm also kind of intermixing the allocation strategy used by multishot accept() and my own for manually selecting a socket fd.

So basically, I have a hash set of entries 0 through nr_files-1. When a multishot accept() CQE comes in, I remove the corresponding fd from the hash set. This ensure that when I do a socket creation request, I don't accidentally reuse a in-use fd. This part seems to be working fine.

But the problem seems to be that accept() walks the sparse table up to its length and then starts returning -23 in its cqe->res field. I've examined the hash set and there should be available descriptor entries.

Am I missing a call to io_uring_register_files_update? Or is this because I'm not partitioning the descriptor table using alloc range? In my mind, I thought this would all Just Work and io_uring would magically know which entries were being used by the io_uring_prep_socket_direct() calls.

I'll try partitioning and seeing if that helps!

[axboe]

I write a small reproducer, but seems to work fine for me. See below paste. It sets up NFILES sparse descriptors, and uses multishot accept to get connections. When we hit NFILES of connections, we randomly close one of the previous ones. Run this for 4 * NFILES, works for me.

You neglected to mention what kernel you are running, just in case this is a kernel issue.

Code:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <liburing.h>

enum {
	T_EXIT_PASS = 0,
	T_EXIT_SKIP = 77,
	T_EXIT_FAIL = 1,
};

static int use_port;

#define NFILES	20

int t_bind_ephemeral_port(int fd, struct sockaddr_in *addr)
{
	socklen_t addrlen;
	int ret;

	addr->sin_port = 0;
	if (bind(fd, (struct sockaddr *)addr, sizeof(*addr)))
		return -errno;

	addrlen = sizeof(*addr);
	ret = getsockname(fd, (struct sockaddr *)addr, &addrlen);
	assert(!ret);
	assert(addr->sin_port != 0);
	return 0;
}

static int start_accept_listen(void)
{
	int fd, ret;

	fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);

	int32_t val = 1;
	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &val, sizeof(val));
	assert(ret != -1);
	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
	assert(ret != -1);

	struct sockaddr_in laddr;

	laddr.sin_family = AF_INET;
	laddr.sin_addr.s_addr = inet_addr("127.0.0.1");
	ret = t_bind_ephemeral_port(fd, &laddr);
	use_port = laddr.sin_port;
	assert(!ret);
	ret = listen(fd, 128);
	assert(ret != -1);

	return fd;
}

static int do_connect(void)
{
	struct sockaddr_in addr;
	int fd, ret;
	int32_t val;

	fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);

	val = 1;
	ret = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &val, sizeof(val));
	assert(ret != -1);

	int32_t flags = fcntl(fd, F_GETFL, 0);
	assert(flags != -1);

	flags |= O_NONBLOCK;
	ret = fcntl(fd, F_SETFL, flags);
	assert(ret != -1);

	addr.sin_family = AF_INET;
	addr.sin_addr.s_addr = inet_addr("127.0.0.1");
	addr.sin_port = use_port;
	
	ret = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
	assert(ret == -1);

	flags = fcntl(fd, F_GETFL, 0);
	assert(flags != -1);

	flags &= ~O_NONBLOCK;
	ret = fcntl(fd, F_SETFL, flags);
	assert(ret != -1);

	return fd;
}

static int close_rand_direct_file(struct io_uring *ring, int *expect_fd)
{
	struct io_uring_sqe *sqe;
	struct io_uring_cqe *cqe;
	int to_close, ret;

	to_close = rand() % NFILES;
	sqe = io_uring_get_sqe(ring);
	io_uring_prep_close_direct(sqe, to_close);
	sqe->user_data = 2;

	ret = io_uring_submit(ring);
	if (ret != 1) {
		printf("submit %d\n", ret);
		return 1;
	}

	ret = io_uring_wait_cqe(ring, &cqe);
	if (ret) {
		printf("wait res close %d\n", ret);
		return 1;
	}

	printf("closed direct %d, res %d\n", to_close, cqe->res);
	io_uring_cqe_seen(ring, cqe);
	*expect_fd = to_close;
	return 0;
}

static int test(void)
{
	struct io_uring_params p = { };
	struct io_uring ring;
	struct io_uring_sqe *sqe;
	struct io_uring_cqe *cqe;
	int ret, fd, i, nopen, expect_fd;

	io_uring_queue_init_params(8, &ring, &p);

	ret = io_uring_register_files_sparse(&ring, NFILES);
	if (ret) {
		fprintf(stderr, "register files: %d\n", ret);
		return T_EXIT_FAIL;
	}

	fd = start_accept_listen();

	sqe = io_uring_get_sqe(&ring);
	io_uring_prep_multishot_accept_direct(sqe, fd, NULL, NULL, 0);
	sqe->user_data = 1;

	io_uring_submit(&ring);

	usleep(1000);

	expect_fd = 0;
	nopen = 0;
	for (i = 0; i < 4 * NFILES; i++) {
		int sockfd = do_connect();

		if (sockfd < 0)
			break;

		ret = io_uring_wait_cqe(&ring, &cqe);
		if (ret) {
			printf("wait cqe %d\n", ret);
			break;
		}
		printf("accept res %d, flags %x\n", cqe->res, cqe->flags);
		if (!(cqe->flags & IORING_CQE_F_MORE)) {
			printf("MORE not set, done\n");
			break;
		}
		if (expect_fd != cqe->res) {
			printf("expected fd %d, got %d\n", expect_fd, cqe->res);
			break;
		}
		io_uring_cqe_seen(&ring, cqe);
		close(sockfd);
		++nopen;
		if (nopen == NFILES) {
			if (close_rand_direct_file(&ring, &expect_fd))
				break;
			nopen--;
		} else {
			expect_fd++;
		}
	}

	io_uring_queue_exit(&ring);
	return ret;
}

int main(int argc, char *argv[])
{
	int ret;

	if (argc > 1)
		return T_EXIT_SKIP;

	srand(getpid());

	ret = test();
	if (ret) {
		fprintf(stderr, "test sqpoll failed\n");
		return T_EXIT_FAIL;
	}

	return T_EXIT_PASS;
}

[axboe]

I suspect you just want the ranges split, though, as you mentioned. Leave one range for direct allocations, one for ranges you pick.

[cmazakas]

I realize now, and I'm replying long after the answer has been accepted, because I think it's important to note this for future readers.

You can't safely share a descriptor table with manually managed fds and ops using FILE_INDEX_ALLOC because the acquisition of available fds is happening asynchronously in the background which introduces a race between your application-level view of available fds and what's actually being used by the kernel.

So, you must always use io_uring_register_file_alloc_range to partition a subsection for the kernel to exclusively manage.

I'm happy I thought of this now instead of having to solve it via subtle corruptions later.

[cmazakas]

Huh, you know what I just realized?

I was closing the file descriptor incorrectly lol.

auto ring = detail::executor_access_policy::ring( ex_ );
auto sqe = io_uring_get_sqe( ring );
io_uring_prep_close_direct( sqe, fd_ );
io_uring_sqe_set_flags(
    sqe, IOSQE_CQE_SKIP_SUCCESS /*  | IOSQE_FIXED_FILE  */ );
io_uring_sqe_set_data( sqe, nullptr );
io_uring_submit( ring );

Namely, if you use IOSQE_FIXED_FILE there, it seems to silently break things.

Interestingly enough, if you do set that flag erroneously, you can then "fix" it by manually invoking io_uring_register_files_update lol.

Ha, now everything is working perfectly. Thank you for taking the time, Jens, I really appreciate it.

Doing this much io_uring inspires me to help flesh out the man pages so hopefully in the future other users aren't as confused as me.

[axboe]

Doing this much io_uring inspires me to help flesh out the man pages so hopefully in the future other users aren't as confused as me.

Please do!!