Potential deadlock in pgaio_io_wait()

From ec2e1e21054f00918d3b28ce01129bc06de37790 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sun, 3 Aug 2025 23:07:56 +1200
Subject: [PATCH v2 1/2] aio: Fix pgaio_io_wait() for staged IOs.

Previously, pgaio_io_wait()'s cases for PGAIO_HS_DEFINED and
PGAIO_HS_STAGED fell through to waiting for completion.  The owner only
promises to advance it to PGAIO_HS_SUBMITTED.  The waiter needs to be
prepared to call ->wait_one() itself once the IO is submitted in order
to guarantee progress and avoid deadlocks on IO methods that provide
->wait_one().

Introduce a new per-backend condition variable submit_cv, woken by by
pgaio_submit_stage(), and use it to wait for the state to advance.  The
new broadcast doesn't seem to cause any measurable slowdown, so ideas
for optimizing the common no-waiters case were abandoned for now.

It may not be possible to reach any real deadlock with existing AIO
users, but that situation could change.  There's also no reason the
waiter shouldn't begin to wait via the IO method as soon as possible
even without a deadlock.

Picked up by testing a proposed IO method that has ->wait_one(), like
io_method=io_uring, and code review.

Backpatch-through: 18
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA%2BhUKG%2BmZYrSdnhk-XrBYO18H829K77S9gMKUsykOiTJtqB43g%40mail.gmail.com
---
 src/backend/storage/aio/aio.c                 | 26 ++++++++++++++++---
 src/backend/storage/aio/aio_init.c            |  1 +
 .../utils/activity/wait_event_names.txt       |  1 +
 src/include/storage/aio_internal.h            |  7 +++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index 3643f27ad6e..87eb1f71705 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -607,6 +607,23 @@ pgaio_io_wait(PgAioHandle *ioh, uint64 ref_generation)
 				elog(ERROR, "IO in wrong state: %d", state);
 				break;
 
+			case PGAIO_HS_DEFINED:
+			case PGAIO_HS_STAGED:
+
+				/*
+				 * The owner hasn't submitted the IO yet. Wait for it to do
+				 * so. Only then can this backend wait via the IO method if
+				 * required, or otherwise ioh->cv.
+				 */
+				ConditionVariablePrepareToSleep(&pgaio_ctl->backend_state[ioh->owner_procno].submit_cv);
+				while (!pgaio_io_was_recycled(ioh, ref_generation, &state) &&
+					   (state == PGAIO_HS_DEFINED ||
+						state == PGAIO_HS_STAGED))
+					ConditionVariableSleep(&pgaio_ctl->backend_state[ioh->owner_procno].submit_cv,
+										   WAIT_EVENT_AIO_IO_SUBMIT);
+				ConditionVariableCancelSleep();
+				continue;
+
 			case PGAIO_HS_SUBMITTED:
 
 				/*
@@ -621,9 +638,6 @@ pgaio_io_wait(PgAioHandle *ioh, uint64 ref_generation)
 				}
 				/* fallthrough */
 
-				/* waiting for owner to submit */
-			case PGAIO_HS_DEFINED:
-			case PGAIO_HS_STAGED:
 				/* waiting for reaper to complete */
 				/* fallthrough */
 			case PGAIO_HS_COMPLETED_IO:
@@ -1139,6 +1153,12 @@ pgaio_submit_staged(void)
 
 	pgaio_my_backend->num_staged_ios = 0;
 
+	/*
+	 * Wake any backend that started waiting for any of these IOs before
+	 * submission, so that it can call ->wait_one() if necessary.
+	 */
+	ConditionVariableBroadcast(&pgaio_my_backend->submit_cv);
+
 	pgaio_debug(DEBUG4,
 				"aio: submitted %d IOs",
 				total_submitted);
diff --git a/src/backend/storage/aio/aio_init.c b/src/backend/storage/aio/aio_init.c
index 885c3940c66..a065ad1463f 100644
--- a/src/backend/storage/aio/aio_init.c
+++ b/src/backend/storage/aio/aio_init.c
@@ -188,6 +188,7 @@ AioShmemInit(void)
 
 		dclist_init(&bs->idle_ios);
 		memset(bs->staged_ios, 0, sizeof(PgAioHandle *) * PGAIO_SUBMIT_BATCH_SIZE);
+		ConditionVariableInit(&bs->submit_cv);
 		dclist_init(&bs->in_flight_ios);
 
 		/* initialize per-backend IOs */
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 0be307d2ca0..7d1dff2d6da 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -194,6 +194,7 @@ ABI_compatibility:
 Section: ClassName - WaitEventIO
 
 AIO_IO_COMPLETION	"Waiting for another process to complete IO."
+AIO_IO_SUBMIT	"Waiting for another process to submit IO."
 AIO_IO_URING_SUBMIT	"Waiting for IO submission via io_uring."
 AIO_IO_URING_EXECUTION	"Waiting for IO execution via io_uring."
 BASEBACKUP_READ	"Waiting for base backup to read from a file."
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
index 2d37a243abe..3e438b587d9 100644
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -208,6 +208,13 @@ typedef struct PgAioBackend
 	uint16		num_staged_ios;
 	PgAioHandle *staged_ios[PGAIO_SUBMIT_BATCH_SIZE];
 
+	/*
+	 * Other backends sometimes need to wait for the owning backend to submit.
+	 * The per-IO CV would work for that purpose, but a per-backend CV allows
+	 * for just one broadcast per submitted batch.
+	 */
+	ConditionVariable submit_cv;
+
 	/*
 	 * List of in-flight IOs. Also contains IOs that aren't strictly speaking
 	 * in-flight anymore, but have been waited-for and completed by another
-- 
2.39.5 (Apple Git-154)

From a804e6044514078db7c7917a885b19e05eac962b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sun, 3 Aug 2025 23:07:56 +1200
Subject: [PATCH v2] aio: Fix pgaio_io_wait() for staged IOs (B).

Previously, pgaio_io_wait()'s cases for PGAIO_HS_DEFINED and
PGAIO_HS_STAGED fell through to waiting for completion.  The owner only
promises to advance it to PGAIO_HS_SUBMITTED.  The waiter needs to be
prepared to call ->wait_one() itself once the IO is submitted in order
to guarantee progress and avoid deadlocks on IO methods that provide
->wait_one().

Introduce a new per-backend condition variable submit_cv, woken by by
pgaio_submit_stage(), and use it to wait for the state to advance.  The
new broadcast doesn't seem to cause any measurable slowdown, so ideas
for optimizing the common no-waiters case were abandoned for now.

It may not be possible to reach any real deadlock with existing AIO
users, but that situation could change.  There's also no reason the
waiter shouldn't begin to wait via the IO method as soon as possible
even without a deadlock.

Picked up by testing a proposed IO method that has ->wait_one(), like
io_method=io_uring, and code review.

Backpatch-through: 18
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA%2BhUKG%2BmZYrSdnhk-XrBYO18H829K77S9gMKUsykOiTJtqB43g%40mail.gmail.com
---
 src/backend/storage/aio/aio.c                 | 50 ++++++++++++++++---
 src/backend/storage/aio/aio_init.c            |  1 +
 .../utils/activity/wait_event_names.txt       |  1 +
 src/include/storage/aio_internal.h            |  7 +++
 4 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index 3643f27ad6e..dea1721254b 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -568,6 +568,16 @@ pgaio_io_was_recycled(PgAioHandle *ioh, uint64 ref_generation, PgAioHandleState
 	return ioh->generation != ref_generation;
 }
 
+/*
+ * Whether we need to wait via the IO method. Don't check via the IO method if
+ * the issuing backend is executing the IO synchronously.
+ */
+static bool
+pgaio_io_needs_wait_one(PgAioHandle *ioh)
+{
+	return pgaio_method_ops->wait_one && !(ioh->flags & PGAIO_HF_SYNCHRONOUS);
+}
+
 /*
  * Wait for IO to complete. External code should never use this, outside of
  * the AIO subsystem waits are only allowed via pgaio_wref_wait().
@@ -607,23 +617,38 @@ pgaio_io_wait(PgAioHandle *ioh, uint64 ref_generation)
 				elog(ERROR, "IO in wrong state: %d", state);
 				break;
 
-			case PGAIO_HS_SUBMITTED:
+			case PGAIO_HS_DEFINED:
+			case PGAIO_HS_STAGED:
 
 				/*
-				 * If we need to wait via the IO method, do so now. Don't
-				 * check via the IO method if the issuing backend is executing
-				 * the IO synchronously.
+				 * The owner hasn't submitted the IO yet. If we need to wait
+				 * via the IO method, wait for submission, giving this backend
+				 * the chance to call ->wait_one().
 				 */
-				if (pgaio_method_ops->wait_one && !(ioh->flags & PGAIO_HF_SYNCHRONOUS))
+				if (pgaio_io_needs_wait_one(ioh))
+				{
+					PgAioBackend *backend = &pgaio_ctl->backend_state[ioh->owner_procno];
+
+					ConditionVariablePrepareToSleep(&backend->submit_cv);
+					while (!pgaio_io_was_recycled(ioh, ref_generation, &state) &&
+						   (state == PGAIO_HS_DEFINED ||
+							state == PGAIO_HS_STAGED))
+						ConditionVariableSleep(&backend->submit_cv, WAIT_EVENT_AIO_IO_SUBMIT);
+					ConditionVariableCancelSleep();
+					continue;
+				}
+				/* fallthrough */
+
+			case PGAIO_HS_SUBMITTED:
+
+				/* If we need to wait via the IO method, do so now. */
+				if (pgaio_io_needs_wait_one(ioh))
 				{
 					pgaio_method_ops->wait_one(ioh, ref_generation);
 					continue;
 				}
 				/* fallthrough */
 
-				/* waiting for owner to submit */
-			case PGAIO_HS_DEFINED:
-			case PGAIO_HS_STAGED:
 				/* waiting for reaper to complete */
 				/* fallthrough */
 			case PGAIO_HS_COMPLETED_IO:
@@ -1139,6 +1164,15 @@ pgaio_submit_staged(void)
 
 	pgaio_my_backend->num_staged_ios = 0;
 
+	/*
+	 * Wake any backend that started waiting for any of these IOs before
+	 * submission, if it is necessary to call ->wait_one() to guarantee
+	 * progress with the configured IO method.  On its side, pgaio_io_wait()
+	 * only waits for submit_cv on IO methods needing that.
+	 */
+	if (pgaio_method_ops->wait_one)
+		ConditionVariableBroadcast(&pgaio_my_backend->submit_cv);
+
 	pgaio_debug(DEBUG4,
 				"aio: submitted %d IOs",
 				total_submitted);
diff --git a/src/backend/storage/aio/aio_init.c b/src/backend/storage/aio/aio_init.c
index 885c3940c66..a065ad1463f 100644
--- a/src/backend/storage/aio/aio_init.c
+++ b/src/backend/storage/aio/aio_init.c
@@ -188,6 +188,7 @@ AioShmemInit(void)
 
 		dclist_init(&bs->idle_ios);
 		memset(bs->staged_ios, 0, sizeof(PgAioHandle *) * PGAIO_SUBMIT_BATCH_SIZE);
+		ConditionVariableInit(&bs->submit_cv);
 		dclist_init(&bs->in_flight_ios);
 
 		/* initialize per-backend IOs */
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 0be307d2ca0..7d1dff2d6da 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -194,6 +194,7 @@ ABI_compatibility:
 Section: ClassName - WaitEventIO
 
 AIO_IO_COMPLETION	"Waiting for another process to complete IO."
+AIO_IO_SUBMIT	"Waiting for another process to submit IO."
 AIO_IO_URING_SUBMIT	"Waiting for IO submission via io_uring."
 AIO_IO_URING_EXECUTION	"Waiting for IO execution via io_uring."
 BASEBACKUP_READ	"Waiting for base backup to read from a file."
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
index 2d37a243abe..3e438b587d9 100644
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -208,6 +208,13 @@ typedef struct PgAioBackend
 	uint16		num_staged_ios;
 	PgAioHandle *staged_ios[PGAIO_SUBMIT_BATCH_SIZE];
 
+	/*
+	 * Other backends sometimes need to wait for the owning backend to submit.
+	 * The per-IO CV would work for that purpose, but a per-backend CV allows
+	 * for just one broadcast per submitted batch.
+	 */
+	ConditionVariable submit_cv;
+
 	/*
 	 * List of in-flight IOs. Also contains IOs that aren't strictly speaking
 	 * in-flight anymore, but have been waited-for and completed by another
-- 
2.39.5 (Apple Git-154)

From 98704151ae72b3cfa4a5fb85706fe5bca6c7998c Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 22 Sep 2025 17:08:58 +0300
Subject: [PATCH 1/1] Repro backend stuck waiting on submitted IO with io_uring

---
 src/test/modules/test_aio/t/001_aio.pl | 58 ++++++++++++++++++++++++++
 src/test/modules/test_aio/test_aio.c   | 11 ++++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/src/test/modules/test_aio/t/001_aio.pl b/src/test/modules/test_aio/t/001_aio.pl
index 3f0453619e8..1fcf02aafdb 100644
--- a/src/test/modules/test_aio/t/001_aio.pl
+++ b/src/test/modules/test_aio/t/001_aio.pl
@@ -673,6 +673,63 @@ sub test_complete_foreign
 	$psql_b->quit();
 }
 
+# 1. Backend A stages an IO
+# 2. Backend B blocks waiting on it
+# 3. Backend A submits the IO, but does not wait for it to complete
+# 4. Backend B should complete the IO
+sub test_wait_staged
+{
+	my $io_method = shift;
+	my $node = shift;
+	my ($ret, $output);
+
+	my $psql_a = $node->background_psql('postgres', on_error_stop => 0);
+	# 10 s timeout to make the test faster if backend B gets stuck waiting for IO
+	my $psql_b = $node->background_psql('postgres', on_error_stop => 0, timeout => 10);
+
+	# To warm the catalog caches
+	$psql_a->query_safe(
+		qq(SELECT read_rel_block_ll('tbl_ok', 2, wait_complete=>true);));
+
+	# Start a new batch
+	$psql_a->query_safe(
+		qq(begin));
+	$psql_a->query_safe(
+		qq(SELECT WHERE batch_start() IS NULL));
+
+	# Stage IO without submitting it yet
+	$psql_a->query_safe(
+		qq(SELECT read_rel_block_ll('tbl_ok', 1, batchmode_enter=>false, wait_complete=>false);));
+
+	# Start reading the same block in another backend. It sees the IO as "staged" by backend
+	# A, and blocks waiting on it.
+	$psql_b->query_until(
+		qr/start/, q{
+	\echo start
+    BEGIN;
+    SELECT read_rel_block_ll('tbl_ok', 1, wait_complete=>true);
+    SELECT 'SUCCESS';
+    COMMIT;
+});
+
+	# make sure psql_b has blocked on the IO
+	sleep(1);
+
+	# Cause the IO we staged in backend A to be submitted, but don't wait for its completion
+	$psql_a->query_safe(
+		qq(rollback));
+
+	# Wait for the IO on backend B to complete.
+	pump_until(
+			$psql_b->{run}, $psql_b->{timeout},
+		\$psql_b->{stdout}, qr/SUCCESS/);
+#	$psql_b->{stderr} = '';
+    ok(1, "$io_method: backend B sees the io staged and later submitted by backend A as completed");
+
+	$psql_a->quit();
+	$psql_b->quit();
+}
+
 # Test that we deal correctly with FDs being closed while IO is in progress
 sub test_close_fd
 {
@@ -1525,6 +1582,7 @@ CHECKPOINT;
 	test_checksum($io_method, $node);
 	test_ignore_checksum($io_method, $node);
 	test_checksum_createdb($io_method, $node);
+	test_wait_staged($io_method, $node);
 
   SKIP:
 	{
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index c55cf6c0aac..63ca3499815 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -374,11 +374,18 @@ read_rel_block_ll(PG_FUNCTION_ARGS)
 	PgAioHandle *ioh;
 	PgAioWaitRef iow;
 	SMgrRelation smgr;
+	ResourceOwner save_resowner = CurrentResourceOwner;
+
+	/*
+	 * Associate the IO with top-transaction owner, so that if you call
+	 * batch_start() before this, the IO gets staged in the batch.
+	 */
+	CurrentResourceOwner = TopTransactionResourceOwner;
 
 	if (nblocks <= 0 || nblocks > PG_IOV_MAX)
 		elog(ERROR, "nblocks is out of range");
 
-	rel = relation_open(relid, AccessExclusiveLock);
+	rel = relation_open(relid, AccessShareLock);
 
 	for (int i = 0; i < nblocks; i++)
 	{
@@ -449,6 +456,8 @@ read_rel_block_ll(PG_FUNCTION_ARGS)
 
 	relation_close(rel, NoLock);
 
+	CurrentResourceOwner = save_resowner;
+
 	PG_RETURN_VOID();
 }
 
-- 
2.47.3