From 44e2d272418ec3ac4521ed332675718680cbc17a Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 17 Aug 2026 16:37:14 -0400 Subject: [PATCH v16 12/21] Don't retry the strategy ring after a failed clock-sweep claim Move the flush/reject/invalidate logic for ring buffers out of GetVictimBuffer() entirely so that GetBufferFromClocksweep() and GetBufferFromRing() each return a fully cleaned buffer with its former contents invalidated. This changes one behavior: previously, if we selected a victim from the clock sweep and then failed to invalidate it (e.g. a concurrent pinner or dirtier), we would loop back and search the strategy ring again. Now, once we move from the ring to the clock sweep, we continue searching the clock sweep until we find a victim. This is clearer and avoids re-checking the ring for a buffer we already decided to evict from shared buffers. It also prevents occupying a spot in the strategy ring with an abandoned shared buffer we failed to invalidate. After this commit, we do not add any shared buffers to the strategy ring until we have fully claimed them. With this, GetVictimBuffer() no longer needs a retry loop: a ring buffer that cannot be claimed is handled inside GetBufferFromRing() by advancing to the next ring slot, and the clock sweep is entered at most once, only after the ring search has given up. One other small change to the control flow: the retry loop over the strategy ring is now explicitly bounded to one full pass. Previously termination was implicit and relied on concurrent pin races being transient. As a side effect of these control flow changes, numBufferAllocs and the bgwriter latch check happen at most once per victim buffer acquisition, rather than once per claim attempt. Author: Melanie Plageman Earlier version Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/D0E7C430-76A9-4030-B8AB-109E7D7ED8C1%40yandex-team.ru --- src/backend/storage/buffer/bufmgr.c | 79 ++------- src/backend/storage/buffer/freelist.c | 221 +++++++++++++++++--------- src/include/storage/buf_internals.h | 15 +- 3 files changed, 172 insertions(+), 143 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 09040bbb3c1..784af0fe3ea 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -679,10 +679,6 @@ static pg_always_inline void TrackBufferHit(IOObject io_object, ForkNumber forknum, BlockNumber blocknum); static uint32 MaxWriteBuffers(void); static Buffer GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context); -static bool ClaimVictimBuffer(BufferAccessStrategy strategy, - BufferDesc *buf_hdr, Buffer bufnum, - uint64 buf_state, - bool from_ring, IOContext io_context); static void FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, @@ -2590,7 +2586,7 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr) * failed to do so. Buffer must already be pinned, but if we fail to claim it * we will unpin it. */ -static bool +bool ClaimVictimBuffer(BufferAccessStrategy strategy, BufferDesc *buf_hdr, Buffer bufnum, uint64 buf_state, bool from_ring, IOContext io_context) @@ -2692,11 +2688,11 @@ MaxWriteBuffers(void) static Buffer GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) { + Buffer bufnum = InvalidBuffer; +#ifdef USE_ASSERT_CHECKING BufferDesc *buf_hdr; - Buffer buf; uint64 buf_state; - bool from_ring; - bool buf_valid; +#endif /* * Ensure, before we pin a victim buffer, that there's a free refcount @@ -2705,77 +2701,33 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) ReservePrivateRefCountEntry(); ResourceOwnerEnlarge(CurrentResourceOwner); - /* we return here if a prospective victim buffer gets used concurrently */ -again: - /* * Select a victim buffer. The buffer is returned pinned and owned by this - * backend. If given a strategy object, see whether it can select a - * buffer; otherwise, or if the ring cannot provide one, get a buffer from - * shared buffers using the clock sweep and record it in the ring. + * backend and is already cleaned and invalidated. */ - buf_hdr = NULL; - from_ring = false; if (strategy) - { - buf_hdr = GetBufferFromRing(strategy, &buf_state); - if (buf_hdr) - from_ring = true; - } - if (!buf_hdr) - { - buf_hdr = GetBufferFromClocksweep(&buf_state); - if (strategy) - AddBufferToRing(strategy, buf_hdr); - } - buf = BufferDescriptorGetBuffer(buf_hdr); + bufnum = GetBufferFromRing(strategy, io_context); - /* Don't use a stale buf_state value after InvalidateVictimBuffer */ - buf_valid = buf_state & BM_VALID; - - /* - * Try to claim the buffer -- flushing it first if dirty, and possibly - * letting the strategy reject it. If someone else takes or dirties the - * buffer before we can claim it, loop back and get another one. - */ - if (!ClaimVictimBuffer(strategy, buf_hdr, buf, buf_state, from_ring, - io_context)) - goto again; - - if (buf_valid) + /* If no strategy or didn't find a strategy buffer, get one from SB */ + if (!BufferIsValid(bufnum)) { - /* - * When a BufferAccessStrategy is in use, blocks evicted from shared - * buffers are counted as IOOP_EVICT in the corresponding context - * (e.g. IOCONTEXT_BULKWRITE). Shared buffers are evicted by a - * strategy in two cases: 1) while initially claiming buffers for the - * strategy ring 2) to replace an existing strategy ring buffer - * because it is pinned or in use and cannot be reused. - * - * Blocks evicted from buffers already in the strategy ring are - * counted as IOOP_REUSE in the corresponding strategy context. - * - * At this point, we can accurately count evictions and reuses, - * because we have successfully claimed the valid buffer and - * invalidated its previous tenant. Previously, we may have been - * forced to release the buffer due to concurrent pinners or erroring - * out. - */ - pgstat_count_io_op(IOOBJECT_RELATION, io_context, - from_ring ? IOOP_REUSE : IOOP_EVICT, 1, 0); + bufnum = GetBufferFromClocksweep(io_context); + if (strategy) + AddBufferToRing(strategy, bufnum); } /* a final set of sanity checks */ #ifdef USE_ASSERT_CHECKING + buf_hdr = GetBufferDescriptor(bufnum - 1); buf_state = pg_atomic_read_u64(&buf_hdr->state); Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 1); Assert(!(buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY))); - CheckBufferIsPinnedOnce(buf); + CheckBufferIsPinnedOnce(bufnum); #endif - return buf; + return bufnum; } /* @@ -4838,8 +4790,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, * If a shared buffer which was added to the ring later because the * current strategy buffer is pinned or in use or because all strategy * buffers were dirty and rejected (for BAS_BULKREAD operations only) - * requires flushing, this is counted as an IOCONTEXT_NORMAL IOOP_WRITE - * (from_ring will be false). + * requires flushing, this is counted as an IOCONTEXT_NORMAL IOOP_WRITE. * * When a strategy is not in use, the write can only be a "regular" write * of a dirty shared buffer (IOCONTEXT_NORMAL IOOP_WRITE). diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 7a8b20c6929..c56addaabc4 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -167,10 +167,9 @@ ClockSweepTick(void) * TrackNewBufferPin(). It is the caller's responsibility to make sure the * buffer ownership can be tracked. */ -BufferDesc * -GetBufferFromClocksweep(uint64 *buf_state) +Buffer +GetBufferFromClocksweep(IOContext io_context) { - BufferDesc *buf; int bgwprocno; int trycounter; @@ -211,8 +210,11 @@ GetBufferFromClocksweep(uint64 *buf_state) trycounter = NBuffers; for (;;) { + BufferDesc *buf; + Buffer bufnum = InvalidBuffer; uint64 old_buf_state; - uint64 local_buf_state; + uint64 buf_state; + bool buf_valid; buf = GetBufferDescriptor(ClockSweepTick()); @@ -223,7 +225,7 @@ GetBufferFromClocksweep(uint64 *buf_state) old_buf_state = pg_atomic_read_u64(&buf->state); for (;;) { - local_buf_state = old_buf_state; + buf_state = old_buf_state; /* * If the buffer is pinned or has a nonzero usage_count, we cannot @@ -231,7 +233,7 @@ GetBufferFromClocksweep(uint64 *buf_state) * scanning. */ - if (BUF_STATE_GET_REFCOUNT(local_buf_state) != 0) + if (BUF_STATE_GET_REFCOUNT(buf_state) != 0) { if (--trycounter == 0) { @@ -248,18 +250,18 @@ GetBufferFromClocksweep(uint64 *buf_state) } /* See equivalent code in PinBuffer() */ - if (unlikely(local_buf_state & BM_LOCKED)) + if (unlikely(buf_state & BM_LOCKED)) { old_buf_state = WaitBufHdrUnlocked(buf); continue; } - if (BUF_STATE_GET_USAGECOUNT(local_buf_state) != 0) + if (BUF_STATE_GET_USAGECOUNT(buf_state) != 0) { - local_buf_state -= BUF_USAGECOUNT_ONE; + buf_state -= BUF_USAGECOUNT_ONE; if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, - local_buf_state)) + buf_state)) { trycounter = NBuffers; break; @@ -268,17 +270,52 @@ GetBufferFromClocksweep(uint64 *buf_state) else { /* pin the buffer if the CAS succeeds */ - local_buf_state += BUF_REFCOUNT_ONE; + buf_state += BUF_REFCOUNT_ONE; if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, - local_buf_state)) + buf_state)) { /* Found a usable buffer */ - *buf_state = local_buf_state; + bufnum = BufferDescriptorGetBuffer(buf); + TrackNewBufferPin(bufnum); - TrackNewBufferPin(BufferDescriptorGetBuffer(buf)); + /* + * Don't use a stale buf_state value after + * InvalidateVictimBuffer + */ + buf_valid = buf_state & BM_VALID; + + if (ClaimVictimBuffer(NULL, buf, bufnum, buf_state, + false, io_context)) + { + /* + * Blocks evicted from shared buffers are counted as + * IOOP_EVICT in the corresponding context, even when + * a strategy is in use. Shared buffers are evicted by + * a strategy in two cases: 1) while initially + * claiming buffers for the strategy ring 2) to + * replace an existing strategy ring buffer because it + * is pinned or in use and cannot be reused. + * + * We can only count this now that we've successfully + * claimed the buffer and invalidated its previous + * tenant. Previously we may have been forced to + * release the buffer due to concurrent pinners or + * erroring out. + */ + if (buf_valid) + pgstat_count_io_op(IOOBJECT_RELATION, io_context, + IOOP_EVICT, 1, 0); + return bufnum; + } - return buf; + /* + * The claim failed because the buffer was concurrently + * pinned or dirtied. Resume the sweep with a fresh + * budget, as if we had restarted the search. + */ + trycounter = NBuffers; + break; } } } @@ -582,95 +619,129 @@ FreeAccessStrategy(BufferAccessStrategy strategy) } /* - * GetBufferFromRing -- returns a buffer from the ring, or NULL if the - * ring is empty / not usable. + * Returns a clean, invalidated buffer from the ring, or InvalidBuffer if the + * ring is empty or contains no usable buffers. In that case, the caller will + * select a buffer from the clocksweep and add it to the ring. * * The buffer is pinned and marked as owned, using TrackNewBufferPin(), before - * returning. + * returning. It is the caller's responsibility to make sure the buffer + * ownership can be tracked. */ -BufferDesc * -GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) +Buffer +GetBufferFromRing(BufferAccessStrategy strategy, IOContext io_context) { - BufferDesc *buf; - Buffer bufnum; - uint64 old_buf_state; - uint64 local_buf_state; /* to avoid repeated (de-)referencing */ - - - /* Advance to next ring slot */ - if (++strategy->current >= strategy->nbuffers) - strategy->current = 0; - - /* - * If the slot hasn't been filled yet, tell the caller to allocate a new - * buffer with the normal allocation strategy. He will then fill this - * slot by calling AddBufferToRing with the new buffer. - */ - bufnum = strategy->buffers[strategy->current]; - if (bufnum == InvalidBuffer) - return NULL; - - buf = GetBufferDescriptor(bufnum - 1); + Assert(strategy); /* - * Check whether the buffer can be used and pin it if so. Do this using a - * CAS loop, to avoid having to lock the buffer header. + * Bound the retries so this loop terminates no matter why claims fail. A + * buffer that fails the reuse checks (pinned or recently used) ends the + * ring search immediately, but a claim failure -- a concurrent pin or + * dirty, or a strategy rejection -- continues it. Today every such + * failure is either transient or removes the buffer from the ring, but + * rather than depend on that, give up after one full pass and let the + * caller fall back to the clock sweep. */ - old_buf_state = pg_atomic_read_u64(&buf->state); - for (;;) + for (int i = 0; i < strategy->nbuffers; i++) { - local_buf_state = old_buf_state; + BufferDesc *buf; + Buffer bufnum; + uint64 old_buf_state; + uint64 buf_state; + bool buf_valid; + + /* Advance to next ring slot */ + if (++strategy->current >= strategy->nbuffers) + strategy->current = 0; /* - * If the buffer is pinned we cannot use it under any circumstances. - * - * If usage_count is 0 or 1 then the buffer is fair game (we expect 1, - * since our own previous usage of the ring element would have left it - * there, but it might've been decremented by clock-sweep since then). - * A higher usage_count indicates someone else has touched the buffer, - * so we shouldn't re-use it. + * If the slot hasn't been filled yet, tell the caller to allocate a + * new buffer with the normal allocation strategy. He will then fill + * this slot by calling AddBufferToRing with the new buffer. */ - if (BUF_STATE_GET_REFCOUNT(local_buf_state) != 0 - || BUF_STATE_GET_USAGECOUNT(local_buf_state) > 1) - break; + bufnum = strategy->buffers[strategy->current]; + if (bufnum == InvalidBuffer) + return InvalidBuffer; + + buf = GetBufferDescriptor(bufnum - 1); - /* See equivalent code in PinBuffer() */ - if (unlikely(local_buf_state & BM_LOCKED)) + /* + * Check whether the buffer can be used and pin it if so. Do this + * using a CAS loop, to avoid having to lock the buffer header. + */ + old_buf_state = pg_atomic_read_u64(&buf->state); + for (;;) { - old_buf_state = WaitBufHdrUnlocked(buf); - continue; + buf_state = old_buf_state; + + /* + * If the buffer is pinned we cannot use it under any + * circumstances. + * + * If usage_count is 0 or 1 then the buffer is fair game (we + * expect 1, since our own previous usage of the ring element + * would have left it there, but it might've been decremented by + * clock-sweep since then). A higher usage_count indicates someone + * else has touched the buffer, so we shouldn't re-use it. + */ + if (BUF_STATE_GET_REFCOUNT(buf_state) != 0 + || BUF_STATE_GET_USAGECOUNT(buf_state) > 1) + return InvalidBuffer; + + /* See equivalent code in PinBuffer() */ + if (unlikely(buf_state & BM_LOCKED)) + { + old_buf_state = WaitBufHdrUnlocked(buf); + continue; + } + + /* pin the buffer if the CAS succeeds */ + buf_state += BUF_REFCOUNT_ONE; + + /* if we can't change state, keep trying */ + if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, + buf_state)) + { + /* got a pin */ + TrackNewBufferPin(BufferDescriptorGetBuffer(buf)); + break; + } } - /* pin the buffer if the CAS succeeds */ - local_buf_state += BUF_REFCOUNT_ONE; + /* Don't use a stale buf_state value after InvalidateVictimBuffer */ + buf_valid = buf_state & BM_VALID; - if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, - local_buf_state)) + if (ClaimVictimBuffer(strategy, buf, bufnum, buf_state, true, io_context)) { - *buf_state = local_buf_state; - - TrackNewBufferPin(BufferDescriptorGetBuffer(buf)); - return buf; + /* + * Blocks evicted from buffers already in the strategy ring are + * counted as IOOP_REUSE in the corresponding strategy context. + * + * We can only count this now that we've successfully claimed the + * buffer and invalidated its previous tenant. Previously we may + * have been forced to release the buffer due to concurrent + * pinners or erroring out. + */ + if (buf_valid) + pgstat_count_io_op(IOOBJECT_RELATION, io_context, IOOP_REUSE, 1, 0); + return bufnum; } } /* * Tell caller to allocate a new buffer with the normal allocation - * strategy. He'll then replace this ring element via AddBufferToRing. + * strategy. He'll then replace this ring element via AddBufferToRing. */ - return NULL; + return InvalidBuffer; } /* - * AddBufferToRing -- add a buffer to the buffer ring - * - * Caller must hold the buffer header spinlock on the buffer. Since this - * is called with the spinlock held, it had better be quite cheap. + * Records the buffer in the current ring slot. The caller must have already + * pinned the buffer and invalidated its previous contents. */ void -AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf) +AddBufferToRing(BufferAccessStrategy strategy, Buffer bufnum) { - strategy->buffers[strategy->current] = BufferDescriptorGetBuffer(buf); + strategy->buffers[strategy->current] = bufnum; } /* diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index c4c155b7e0c..541308a678c 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -558,6 +558,13 @@ ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer) ResourceOwnerForget(owner, Int32GetDatum(buffer), &buffer_io_resowner_desc); } +/* For use in freelist.c but defined in bufmgr.c */ +extern bool ClaimVictimBuffer(BufferAccessStrategy strategy, + BufferDesc *buf_hdr, Buffer bufnum, + uint64 buf_state, + bool from_ring, + IOContext io_context); + /* * Internal buffer management routines */ @@ -594,10 +601,10 @@ extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag /* freelist.c */ extern IOContext IOContextForStrategy(BufferAccessStrategy strategy); -extern BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy, - uint64 *buf_state); -extern void AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf); -extern BufferDesc *GetBufferFromClocksweep(uint64 *buf_state); +extern Buffer GetBufferFromRing(BufferAccessStrategy strategy, + IOContext io_context); +extern void AddBufferToRing(BufferAccessStrategy strategy, Buffer bufnum); +extern Buffer GetBufferFromClocksweep(IOContext io_context); extern bool StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, uint64 buf_state); -- 2.47.3