bufmgr: Fix order of operations in UnlockBufHdrExt

In c75ebc657ffc I (Andres) introduced UnlockBufHdrExt() which can set and
clear bits in the buffer state using CAS. Unfortunately I added bits before
subtracting them, which means that a bit that was both removed and set would
remain unset.  Fix the order of operations.

The only known case where that is a problem is that BM_IO_ERROR would not
actually remain set.

It's unfortunately not trivial to add a decent, race-free, test to verify that
BM_IO_ERROR remains set. That's therefore left for the 20 cycle.

Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru>
Discussion: https://postgr.es/m/ab0dcc9e-aba0-44e3-ac23-8d74c48888e6@postgrespro.ru
Backpatch-through: 19, where c75ebc657ffc went in

diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 2a1ffcc6c5c390cd11b0507f401c9316fc15c26e..e4ff5619b79ca5f1800a2c36285b6961b01dfc69 100644 (file)
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -465,12 +465,13 @@ UnlockBufHdr(BufferDesc *desc)
 }
 
 /*
- * Unlock the buffer header, while atomically adding the flags in set_bits,
- * unsetting the ones in unset_bits and changing the refcount by
- * refcount_change.
+ * Unlock the buffer header, while atomically unsetting the ones in
+ * unset_bits, adding the flags in set_bits, and changing the refcount by
+ * refcount_change. If a flag is both cleared and added, it will end up being
+ * set.
  *
- * Note that this approach would not work for usagecount, since we need to cap
- * the usagecount at BM_MAX_USAGE_COUNT.
+ * Note that this approach would not trivially work for usagecount, since we
+ * need to cap the usagecount at BM_MAX_USAGE_COUNT.
  */
 static inline uint64
 UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state,
@@ -483,8 +484,12 @@ UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state,
 
                Assert(buf_state & BM_LOCKED);
 
-               buf_state |= set_bits;
+               /*
+                * Set bits after clearing bits, so that a cleared and re-added flag
+                * survives.
+                */
                buf_state &= ~unset_bits;
+               buf_state |= set_bits;
                buf_state &= ~BM_LOCKED;
 
                if (refcount_change != 0)