aio, bufmgr: Comment fixes/improvements

anarazel · anarazel · commit 08ccd56ac765 · 2025-03-29T14:45:42.000-04:00
Some of these comments have been wrong for a while (12f3867), some I recently introduced (da72269, 55b454d). This includes an update to a comment in FlushBuffer(), which will be copied in a future commit. These changes seem big enough to be worth doing in separate commits. Suggested-by: Noah Misch <noah@leadboat.com> Discussion: https://postgr.es/m/20250319212530.80.nmisch@google.com
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
@@ -4401,7 +4401,7 @@ maybe_adjust_io_workers(void)
 			++io_worker_count;
 		}
 		else
-			break;				/* XXX try again soon? */
+			break;				/* try again next time */
 	}
 
 	/* Too many running? */
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
@@ -3929,9 +3929,10 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
 		XLogFlush(recptr);
 
 	/*
-	 * Now it's safe to write buffer to disk. Note that no one else should
-	 * have been able to write it while we were busy with log flushing because
-	 * only one process at a time can set the BM_IO_IN_PROGRESS bit.
+	 * Now it's safe to write the buffer to disk. Note that no one else should
+	 * have been able to write it, while we were busy with log flushing,
+	 * because we got the exclusive right to perform I/O by setting the
+	 * BM_IO_IN_PROGRESS bit.
 	 */
 	bufBlock = BufHdrGetBlock(buf);
 
@@ -5499,9 +5500,6 @@ IsBufferCleanupOK(Buffer buffer)
 /*
  *	Functions for buffer I/O handling
  *
- *	Note: We assume that nested buffer I/O never occurs.
- *	i.e at most one BM_IO_IN_PROGRESS bit is set per proc.
- *
  *	Also note that these are used only for shared buffers, not local ones.
  */
 
diff --git a/src/include/storage/aio.h b/src/include/storage/aio.h
@@ -80,7 +80,7 @@ typedef enum PgAioHandleFlags
 /*
  * The IO operations supported by the AIO subsystem.
  *
- * This could be in aio_internal.h, as it is not pubicly referenced, but
+ * This could be in aio_internal.h, as it is not publicly referenced, but
  * PgAioOpData currently *does* need to be public, therefore keeping this
  * public seems to make sense.
  */
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
@@ -34,6 +34,11 @@
  * linearly through all states.
  *
  * State changes should all go through pgaio_io_update_state().
+ *
+ * Note that the externally visible functions to start IO
+ * (e.g. FileStartReadV(), via pgaio_io_start_readv()) move an IO from
+ * PGAIO_HS_HANDED_OUT to at least PGAIO_HS_STAGED and at most
+ * PGAIO_HS_COMPLETED_LOCAL (at which point the handle will be reused).
  */
 typedef enum PgAioHandleState
 {
@@ -285,6 +290,9 @@ typedef struct IoMethodOps
 	/*
 	 * Start executing passed in IOs.
 	 *
+	 * Shall advance state to at least PGAIO_HS_SUBMITTED.  (By the time this
+	 * returns, other backends might have advanced the state further.)
+	 *
 	 * Will not be called if ->needs_synchronous_execution() returned true.
 	 *
 	 * num_staged_ios is <= PGAIO_SUBMIT_BATCH_SIZE.
@@ -293,12 +301,24 @@ typedef struct IoMethodOps
 	 */
 	int			(*submit) (uint16 num_staged_ios, PgAioHandle **staged_ios);
 
-	/*
+	/* ---
 	 * Wait for the IO to complete. Optional.
 	 *
+	 * On return, state shall be on of
+	 * - PGAIO_HS_COMPLETED_IO
+	 * - PGAIO_HS_COMPLETED_SHARED
+	 * - PGAIO_HS_COMPLETED_LOCAL
+	 *
+	 * The callback must not block if the handle is already in one of those
+	 * states, or has been reused (see pgaio_io_was_recycled()).  If, on
+	 * return, the state is PGAIO_HS_COMPLETED_IO, state will reach
+	 * PGAIO_HS_COMPLETED_SHARED without further intervention by the IO
+	 * method.
+	 *
 	 * If not provided, it needs to be guaranteed that the IO method calls
 	 * pgaio_io_process_completion() without further interaction by the
 	 * issuing backend.
+	 * ---
 	 */
 	void		(*wait_one) (PgAioHandle *ioh,
 							 uint64 ref_generation);

Original file line number	Diff line number	Diff line change
`@@ -4401,7 +4401,7 @@ maybe_adjust_io_workers(void)`
`4401`	`4401`	`++io_worker_count;`
`4402`	`4402`	`}`
`4403`	`4403`	`else`
`4404`		`- break; /* XXX try again soon? */`
	`4404`	`+ break; /* try again next time */`
`4405`	`4405`	`}`
`4406`	`4406`
`4407`	`4407`	`/* Too many running? */`
Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,7 @@ typedef enum PgAioHandleFlags`
`80`	`80`	`/*`
`81`	`81`	`* The IO operations supported by the AIO subsystem.`
`82`	`82`	`*`
`83`		`- * This could be in aio_internal.h, as it is not pubicly referenced, but`
	`83`	`+ * This could be in aio_internal.h, as it is not publicly referenced, but`
`84`	`84`	`* PgAioOpData currently does need to be public, therefore keeping this`
`85`	`85`	`* public seems to make sense.`
`86`	`86`	`*/`