Date: Mon, 16 Sep 2024 07:43:49 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Heikki Linnakangas <hlinnaka@iki.fi>,
	=?utf-8?B?6ZmI5a6X5b+X?= <baotiao@gmail.com>
Subject: Re: AIO v2.0
Message-ID: <20240916144349.74.nmisch@google.com>
References: <uvrtrknj4kdytuboidbhwclo4gxhswwcpgadptsjvjqcluzmah@brqs62irg4dt>
 <x3f32prdpgalmiieyialqtn53j5uvb2e4c47nvnjetkipq3zyk@xk7jy7fnua6w>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="EPKgKjQsJI3J50Dn"
Content-Disposition: inline
In-Reply-To: <x3f32prdpgalmiieyialqtn53j5uvb2e4c47nvnjetkipq3zyk@xk7jy7fnua6w>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240916144349.74.nmisch%40google.com>
Precedence: bulk


--EPKgKjQsJI3J50Dn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Sep 06, 2024 at 03:38:16PM -0400, Andres Freund wrote:
> There's plenty more to do, but I thought this would be a useful checkpoint.

I find patches 1-5 are Ready for Committer.

> +typedef enum PgAioHandleState

This enum clarified a lot for me, so I wish I had read it before anything
else.  I recommend referring to it in README.md.  Would you also cover the
valid state transitions and which of them any backend can do vs. which are
specific to the defining backend?

> +{
> +	/* not in use */
> +	AHS_IDLE = 0,
> +
> +	/* returned by pgaio_io_get() */
> +	AHS_HANDED_OUT,
> +
> +	/* pgaio_io_start_*() has been called, but IO hasn't been submitted yet */
> +	AHS_DEFINED,
> +
> +	/* subjects prepare() callback has been called */
> +	AHS_PREPARED,
> +
> +	/* IO is being executed */
> +	AHS_IN_FLIGHT,

Let's align terms between functions and states those functions reach.  For
example, I recommend calling this state AHS_SUBMITTED, because
pgaio_io_prepare_submit() is the function reaching this state.
(Alternatively, use in_flight in the function name.)

> +
> +	/* IO finished, but result has not yet been processed */
> +	AHS_REAPED,
> +
> +	/* IO completed, shared completion has been called */
> +	AHS_COMPLETED_SHARED,
> +
> +	/* IO completed, local completion has been called */
> +	AHS_COMPLETED_LOCAL,
> +} PgAioHandleState;

> +void
> +pgaio_io_release_resowner(dlist_node *ioh_node, bool on_error)
> +{
> +	PgAioHandle *ioh = dlist_container(PgAioHandle, resowner_node, ioh_node);
> +
> +	Assert(ioh->resowner);
> +
> +	ResourceOwnerForgetAioHandle(ioh->resowner, &ioh->resowner_node);
> +	ioh->resowner = NULL;
> +
> +	switch (ioh->state)
> +	{
> +		case AHS_IDLE:
> +			elog(ERROR, "unexpected");
> +			break;
> +		case AHS_HANDED_OUT:
> +			Assert(ioh == my_aio->handed_out_io || my_aio->handed_out_io == NULL);
> +
> +			if (ioh == my_aio->handed_out_io)
> +			{
> +				my_aio->handed_out_io = NULL;
> +				if (!on_error)
> +					elog(WARNING, "leaked AIO handle");
> +			}
> +
> +			pgaio_io_reclaim(ioh);
> +			break;
> +		case AHS_DEFINED:
> +		case AHS_PREPARED:
> +			/* XXX: Should we warn about this when is_commit? */

Yes.

> +			pgaio_submit_staged();
> +			break;
> +		case AHS_IN_FLIGHT:
> +		case AHS_REAPED:
> +		case AHS_COMPLETED_SHARED:
> +			/* this is expected to happen */
> +			break;
> +		case AHS_COMPLETED_LOCAL:
> +			/* XXX: unclear if this ought to be possible? */
> +			pgaio_io_reclaim(ioh);
> +			break;
> +	}

> +void
> +pgaio_io_ref_wait(PgAioHandleRef *ior)
> +{
> +	uint64		ref_generation;
> +	PgAioHandleState state;
> +	bool		am_owner;
> +	PgAioHandle *ioh;
> +
> +	ioh = pgaio_io_from_ref(ior, &ref_generation);
> +
> +	am_owner = ioh->owner_procno == MyProcNumber;
> +
> +
> +	if (pgaio_io_was_recycled(ioh, ref_generation, &state))
> +		return;
> +
> +	if (am_owner)
> +	{
> +		if (state == AHS_DEFINED || state == AHS_PREPARED)
> +		{
> +			/* XXX: Arguably this should be prevented by callers? */
> +			pgaio_submit_staged();

Agreed for AHS_DEFINED, if not both.  AHS_DEFINED here would suggest a past
longjmp out of pgaio_io_prepare() w/o a subxact rollback to cleanup.  Even so,
the next point might remove the need here:

> +void
> +pgaio_io_prepare(PgAioHandle *ioh, PgAioOp op)
> +{
> +	Assert(ioh->state == AHS_HANDED_OUT);
> +	Assert(pgaio_io_has_subject(ioh));
> +
> +	ioh->op = op;
> +	ioh->state = AHS_DEFINED;
> +	ioh->result = 0;
> +
> +	/* allow a new IO to be staged */
> +	my_aio->handed_out_io = NULL;
> +
> +	pgaio_io_prepare_subject(ioh);
> +
> +	ioh->state = AHS_PREPARED;

As defense in depth, let's add a critical section from before assigning
AHS_DEFINED to here.  This code already needs to be safe for that (per
README.md).  When running outside a critical section, an ERROR in a subject
callback could leak the lwlock disowned in shared_buffer_prepare_common().  I
doubt there's a plausible way to reach that leak today, but future subject
callbacks could add risk over time.

> +if test "$with_liburing" = yes; then
> +  PKG_CHECK_MODULES(LIBURING, liburing)
> +fi

I used the attached makefile patch to build w/ liburing.

> +pgaio_uring_shmem_init(bool first_time)
> +{
> +	uint32		TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS - MAX_IO_WORKERS;
> +	bool		found;
> +
> +	aio_uring_contexts = (PgAioUringContext *)
> +		ShmemInitStruct("AioUring", pgaio_uring_shmem_size(), &found);
> +
> +	if (found)
> +		return;
> +
> +	for (int contextno = 0; contextno < TotalProcs; contextno++)
> +	{
> +		PgAioUringContext *context = &aio_uring_contexts[contextno];
> +		int			ret;
> +
> +		/*
> +		 * XXX: Probably worth sharing the WQ between the different rings,
> +		 * when supported by the kernel. Could also cause additional
> +		 * contention, I guess?
> +		 */
> +#if 0
> +		if (!AcquireExternalFD())
> +			elog(ERROR, "No external FD available");
> +#endif
> +		ret = io_uring_queue_init(io_max_concurrency, &context->io_uring_ring, 0);

With EXEC_BACKEND, "make check PG_TEST_INITDB_EXTRA_OPTS=-cio_method=io_uring"
fails early:

2024-09-15 12:46:08.168 PDT postmaster[2069397] LOG:  starting PostgreSQL 18devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 13.2.0-13) 13.2.0, 64-bit
2024-09-15 12:46:08.168 PDT postmaster[2069397] LOG:  listening on Unix socket "/tmp/pg_regress-xgQOPH/.s.PGSQL.65312"
2024-09-15 12:46:08.203 PDT startup[2069423] LOG:  database system was shut down at 2024-09-15 12:46:07 PDT
2024-09-15 12:46:08.209 PDT client backend[2069425] [unknown] FATAL:  the database system is starting up
2024-09-15 12:46:08.222 PDT postmaster[2069397] LOG:  database system is ready to accept connections
2024-09-15 12:46:08.254 PDT autovacuum launcher[2069435] PANIC:  failed: -9/Bad file descriptor
2024-09-15 12:46:08.286 PDT client backend[2069444] [unknown] PANIC:  failed: -95/Operation not supported
2024-09-15 12:46:08.355 PDT client backend[2069455] [unknown] PANIC:  unexpected: -95/Operation not supported: No such file or directory
2024-09-15 12:46:08.370 PDT postmaster[2069397] LOG:  received fast shutdown request

I expect that's from io_uring_queue_init() stashing in shared memory a file
descriptor and mmap address, which aren't valid in EXEC_BACKEND children.
Reattaching descriptors and memory in each child may work, or one could just
block io_method=io_uring under EXEC_BACKEND.

> +pgaio_uring_submit(uint16 num_staged_ios, PgAioHandle **staged_ios)
> +{
> +	struct io_uring *uring_instance = &my_shared_uring_context->io_uring_ring;
> +
> +	Assert(num_staged_ios <= PGAIO_SUBMIT_BATCH_SIZE);
> +
> +	for (int i = 0; i < num_staged_ios; i++)
> +	{
> +		PgAioHandle *ioh = staged_ios[i];
> +		struct io_uring_sqe *sqe;
> +
> +		sqe = io_uring_get_sqe(uring_instance);
> +
> +		pgaio_io_prepare_submit(ioh);
> +		pgaio_uring_sq_from_io(ioh, sqe);
> +	}
> +
> +	while (true)
> +	{
> +		int			ret;
> +
> +		pgstat_report_wait_start(WAIT_EVENT_AIO_SUBMIT);
> +		ret = io_uring_submit(uring_instance);
> +		pgstat_report_wait_end();
> +
> +		if (ret == -EINTR)
> +		{
> +			elog(DEBUG3, "submit EINTR, nios: %d", num_staged_ios);
> +			continue;
> +		}

Since io_uring_submit() is a wrapper around io_uring_enter(), this should also
retry on EAGAIN.  "man io_uring_enter" has:

    EAGAIN The kernel was unable to allocate memory for the request, or
    otherwise ran out of resources to handle it. The application should wait
    for some completions and try again.

> +FileStartWriteV(struct PgAioHandle *ioh, File file,
> +				int iovcnt, off_t offset,
> +				uint32 wait_event_info)
> +{
> +	int			returnCode;
> +	Vfd		   *vfdP;
> +
> +	Assert(FileIsValid(file));
> +
> +	DO_DB(elog(LOG, "FileStartWriteV: %d (%s) " INT64_FORMAT " %d",
> +			   file, VfdCache[file].fileName,
> +			   (int64) offset,
> +			   iovcnt));
> +
> +	returnCode = FileAccess(file);
> +	if (returnCode < 0)
> +		return returnCode;
> +
> +	vfdP = &VfdCache[file];
> +
> +	/* FIXME: think about / reimplement  temp_file_limit */
> +
> +	pgaio_io_prep_writev(ioh, vfdP->fd, iovcnt, offset);
> +
> +	return 0;
> +}

FileStartWriteV() gets to state AHS_PREPARED, so let's align with the state
name by calling it FilePrepareWriteV (or FileWriteVPrepare or whatever).


For non-sync IO methods, I gather it's essential that a process other than the
IO definer be scanning for incomplete IOs and completing them.  Otherwise,
deadlocks like this would happen:

backend1 locks blk1 for non-IO reasons
backend2 locks blk2, starts AIO write
backend1 waits for lock on blk2 for non-IO reasons
backend2 waits for lock on blk1 for non-IO reasons

If that's right, in worker mode, the IO worker resolves that deadlock.  What
resolves it under io_uring?  Another process that happens to do
pgaio_io_ref_wait() would dislodge things, but I didn't locate the code to
make that happen systematically.  Could you add a mention of "deadlock" in the
comment at whichever code achieves that?


I could share more-tactical observations about patches 6-20, but they're
probably things you'd change without those observations.  Is there any
specific decision you'd like to settle before patch 6 exits WIP?

Thanks,
nm

--EPKgKjQsJI3J50Dn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="uring-makefile-v1.patch"

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 84302cc..b123fdc 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -43,9 +43,10 @@ OBJS = \
 	$(top_builddir)/src/common/libpgcommon_srv.a \
 	$(top_builddir)/src/port/libpgport_srv.a
 
-# We put libpgport and libpgcommon into OBJS, so remove it from LIBS; also add
-# libldap and ICU
-LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS)) $(LDAP_LIBS_BE) $(ICU_LIBS)
+# We put libpgport and libpgcommon into OBJS, so remove it from LIBS.
+LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS))
+# The backend conditionally needs libraries that most executables don't need.
+LIBS += $(LDAP_LIBS_BE) $(ICU_LIBS) $(LIBURING_LIBS)
 
 # The backend doesn't need everything that's in LIBS, however
 LIBS := $(filter-out -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS))

--EPKgKjQsJI3J50Dn--