From e546b1554b346c0c90d28c69ebdfabbc57373755 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sun, 9 Mar 2025 19:55:05 -0400 Subject: [PATCH v2.7 18/35] localbuf: Introduce TerminateLocalBufferIO() Previously TerminateLocalBufferIO() was open-coded in multiple places, which doesn't seem like a great idea. While TerminateLocalBufferIO() currently is rather simple, an upcoming patch requires additional code to be added to TerminateLocalBufferIO(), making this modification particularly worthwhile. For some reason FlushRelationBuffers() previously cleared BM_JUST_DIRTIED, even though that's never set for temporary buffers. This is not carried over as part of this change. Reviewed-by: Melanie Plageman Discussion: https://postgr.es/m/CAAKRu_b9anbWzEs5AAF9WCvcEVmgz-1AkHSQ-CLLy-p7WHzvFw@mail.gmail.com --- src/include/storage/buf_internals.h | 2 ++ src/backend/storage/buffer/bufmgr.c | 32 ++++++++------------------- src/backend/storage/buffer/localbuf.c | 29 +++++++++++++++++++++--- 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 8b32fb108b0..4611a60d3e0 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -471,6 +471,8 @@ extern BlockNumber ExtendBufferedRelLocal(BufferManagerRelation bmr, Buffer *buffers, uint32 *extended_by); extern void MarkLocalBufferDirty(Buffer buffer); +extern void TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty, + uint32 set_flag_bits); extern void DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber firstDelBlock); diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 75a928e802a..ead562249d3 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1072,19 +1072,11 @@ ZeroAndLockBuffer(Buffer buffer, ReadBufferMode mode, bool already_valid) if (!isLocalBuf) LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_EXCLUSIVE); + /* Set BM_VALID, terminate IO, and wake up any waiters */ if (isLocalBuf) - { - /* Only need to adjust flags */ - uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); - - buf_state |= BM_VALID; - pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); - } + TerminateLocalBufferIO(bufHdr, false, BM_VALID); else - { - /* Set BM_VALID, terminate IO, and wake up any waiters */ TerminateBufferIO(bufHdr, false, BM_VALID, true); - } } else if (!isLocalBuf) { @@ -1608,19 +1600,11 @@ WaitReadBuffers(ReadBuffersOperation *operation) relpath(operation->smgr->smgr_rlocator, forknum).str))); } - /* Terminate I/O and set BM_VALID. */ + /* Set BM_VALID, terminate IO, and wake up any waiters */ if (persistence == RELPERSISTENCE_TEMP) - { - uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); - - buf_state |= BM_VALID; - pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); - } + TerminateLocalBufferIO(bufHdr, false, BM_VALID); else - { - /* Set BM_VALID, terminate IO, and wake up any waiters */ TerminateBufferIO(bufHdr, false, BM_VALID, true); - } /* Report I/Os as completing individually. */ TRACE_POSTGRESQL_BUFFER_READ_DONE(forknum, io_first_block + j, @@ -4564,8 +4548,7 @@ FlushRelationBuffers(Relation rel) IOCONTEXT_NORMAL, IOOP_WRITE, io_start, 1, BLCKSZ); - buf_state &= ~(BM_DIRTY | BM_JUST_DIRTIED); - pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); + TerminateLocalBufferIO(bufHdr, true, 0); pgBufferUsage.local_blks_written++; @@ -5652,8 +5635,11 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits, buf_state = LockBufHdr(buf); Assert(buf_state & BM_IO_IN_PROGRESS); + buf_state &= ~BM_IO_IN_PROGRESS; + + /* Clear earlier errors, if this IO failed, it'll be marked again */ + buf_state &= ~BM_IO_ERROR; - buf_state &= ~(BM_IO_IN_PROGRESS | BM_IO_ERROR); if (clear_dirty && !(buf_state & BM_JUST_DIRTIED)) buf_state &= ~(BM_DIRTY | BM_CHECKPOINT_NEEDED); diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index f46ba0c0558..6df51a3a754 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -235,7 +235,6 @@ GetLocalVictimBuffer(void) */ if (pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY) { - uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); instr_time io_start; SMgrRelation oreln; Page localpage = (char *) LocalBufHdrGetBlock(bufHdr); @@ -259,8 +258,7 @@ GetLocalVictimBuffer(void) IOOP_WRITE, io_start, 1, BLCKSZ); /* Mark not-dirty now in case we error out below */ - buf_state &= ~BM_DIRTY; - pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); + TerminateLocalBufferIO(bufHdr, true, 0); pgBufferUsage.local_blks_written++; } @@ -483,6 +481,31 @@ MarkLocalBufferDirty(Buffer buffer) pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); } +/* + * Like TerminateBufferIO, but for local buffers + */ +void +TerminateLocalBufferIO(BufferDesc *bufHdr, bool clear_dirty, uint32 set_flag_bits) +{ + /* Only need to adjust flags */ + uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); + + /* BM_IO_IN_PROGRESS isn't currently used for local buffers */ + + /* Clear earlier errors, if this IO failed, it'll be marked again */ + buf_state &= ~BM_IO_ERROR; + + if (clear_dirty) + buf_state &= ~BM_DIRTY; + + buf_state |= set_flag_bits; + pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); + + /* local buffers don't track IO using resowners */ + + /* local buffers don't use the IO CV, as no other process can see buffer */ +} + /* * InvalidateLocalBuffer -- mark a local buffer invalid. * -- 2.48.1.76.g4e746b1a31.dirty