From 1766e29c3bbe0e0659d3bf1d5d1e6d7aa88c1a05 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 20 Mar 2025 11:32:10 -0400 Subject: [PATCH v2.14 06/29] aio, bufmgr: Comment fixes Some of these comments have been wrong for a while (12f3867f5534), some I recently introduced (da7226993fd, 55b454d0e14). This also updates 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 Discussion: https://postgr.es/m/20250319212530.80.nmisch@google.com --- src/include/storage/aio.h | 2 +- src/include/storage/aio_internal.h | 22 +++++++++++++++++++++- src/backend/postmaster/postmaster.c | 2 +- src/backend/storage/buffer/bufmgr.c | 10 ++++------ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/include/storage/aio.h b/src/include/storage/aio.h index c386fc96d90..5c886e478a5 100644 --- 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 index fb0425ccbfc..7f18da2c856 100644 --- 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); diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index a0c37532d2f..c966c2e83af 100644 --- 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 index 5cac8cd7389..03317b49025 100644 --- 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. */ -- 2.48.1.76.g4e746b1a31.dirty