Re: Buffer locking is special (hints, checksums, AIO writes)

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Andres Freund <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: [email protected]
Cc: Thomas Munro <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Wed, 14 Jan 2026 10:26:07 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>
References: <lneuyxqxamqoayd2ntau3lqjblzdckw6tjgeu4574ezwh4tzlg@noioxkquezdw>
	<[email protected]>
	<q3ybyaiownp4ivt6xfy55s5llvtq6vqsd4zbt5kn6hacxdcpp4@2ngd4h4bev5d>
	<dsbacri4xldlobl3jbqqlksefsyrwj7lzljbohxsbqjus3z4o7@otb6rge4w7az>
	<[email protected]>
	<5dwlfu2jyzkyf3nrlzxxblxctb6xio5es73ptgsahjnmfu5miu@772rc764hfhi>
	<ossv2eistssmubfsir6xjll76tynvxv5lup4zkrfzjkud7fycw@rf5vii6l6cha>
	<4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
	<CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
	<kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
	<jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>



> On Jan 13, 2026, at 08:33, Andres Freund <[email protected]> wrote:
> 
> Hi,
> 
> On 2026-01-12 12:45:03 -0500, Andres Freund wrote:
>> I'm doing another pass through 0003 and will push that if I don't find
>> anything significant.
> 
> Done, after adjust two comments in minor ways.
> 
> 
>> Also working on doing comment polishing of the later patches, found a few
>> things, but not quite enough to be worth reposting yet.
> 
> Here are the remaining commits, with a bit of polish:
> 
> - fixed references to old names in some places (lwlocks, release_ok)
> 
> - Aded an assert that we don't already hold a lock in BufferLockConditional()
> 
> - typo and grammar fixes
> 
> - updated the commit message of the LW_FLAG_RELEASE_OK, as "requested" by
>  Melanie. I hope this explains the situation better.
> 
> - added a commit that renames ResOwnerReleaseBufferPin to
>  ResOwnerReleaseBuffer (et al), as it now also releases content locks if held
> 
>  I kept this separate as I'm not yet sure about the new name, partially due
>  to there also being a "buffer io" resowner.  I tried "buffer ownership" for
>  the resowner that tracks pins and locks, but that was long and not clearly
>  better.
> 
> Greetings,
> 
> Andres Freund
> <v10-0001-lwlock-Invert-meaning-of-LW_FLAG_RELEASE_OK.patch><v10-0002-bufmgr-Make-definitions-related-to-buffer-descri.patch><v10-0003-bufmgr-Change-BufferDesc.state-to-be-a-64-bit-at.patch><v10-0004-bufmgr-Implement-buffer-content-locks-independen.patch><v10-0005-Require-share-exclusive-lock-to-set-hint-bits-an.patch><v10-0006-WIP-Make-UnlockReleaseBuffer-more-efficient.patch><v10-0007-WIP-bufmgr-Don-t-copy-pages-while-writing-out.patch><v10-0008-WIP-bufmgr-Rename-ResOwnerReleaseBufferPin.patch>

Hi Andres,

So far I’ve only reviewed 0001 and 0002. I’m not very familiar with this area, so the review has been a bit slow.

Overall, 0001 looks good to me. It renames LW_FLAG_RELEASE_OK to LW_FLAG_WAKE_IN_PROGRESS and inverts the meaning, which makes sense. I only have a small nit on naming: the local variable “new_release_in_progress". I see that it’s inherited from the old name and was updated from “_ok" to “_in_progress", but now that the flag itself is renamed, would it make sense to rename the variable as well? Something like “wake_in_progress" or “new_wake_in_progress" might better reflect the new flag name.

In 0002, a bunch of new macros are introduced. My initial impression wasn’t great, mostly due to the amount of line wrapping. Looking a bit closer, I also noticed some duplication, for example, "BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS" appears more than once; and a small inconsistency between BUF_STATE_GET_REFCOUNT and BUF_STATE_GET_USAGECOUNT (even though the former doesn’t actually need a shift).

I tried a small refactor of the macro definitions in the attached diff to see if things could be made a bit more regular. It introduces a helper macro MASK() and a BUF_REFCOUNT_SHIFT constant, and removes a bit of duplication. If you like it, feel free to take it; otherwise, please just ignore it. Note that, the diff is based on 0002.

(I actually hesitated to attach a diff, because if you’ve already created a CF entry, the attached diff could break the CI tests. If that happens, sorry about that.)

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/






Attachments:

  [application/octet-stream] buf_internals_h.diff (2.2K, 2-buf_internals_h.diff)
  download | inline diff:
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 2f607ea2ac5..34e6c6cd54f 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -49,28 +49,26 @@
 StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
 				 "parts of buffer state space need to equal 32");
 
-/* refcount related definitions */
-#define BUF_REFCOUNT_ONE 1
-#define BUF_REFCOUNT_MASK \
-	((1U << BUF_REFCOUNT_BITS) - 1)
+#define BUF_REFCOUNT_SHIFT       0
+#define BUF_USAGECOUNT_SHIFT     (BUF_REFCOUNT_SHIFT + BUF_REFCOUNT_BITS)
+#define BUF_FLAG_SHIFT           (BUF_USAGECOUNT_SHIFT + BUF_USAGECOUNT_BITS)
+
+/* mask generator */
+#define MASK(bits) ((1U << (bits)) - 1)
 
+/* refcount related definitions */
+#define BUF_REFCOUNT_ONE         1U
+#define BUF_REFCOUNT_MASK        (MASK(BUF_REFCOUNT_BITS) << BUF_REFCOUNT_SHIFT)
 /* usage count related definitions */
-#define BUF_USAGECOUNT_SHIFT \
-	BUF_REFCOUNT_BITS
-#define BUF_USAGECOUNT_MASK \
-	(((1U << BUF_USAGECOUNT_BITS) - 1) << (BUF_USAGECOUNT_SHIFT))
-#define BUF_USAGECOUNT_ONE \
-	(1U << BUF_REFCOUNT_BITS)
+#define BUF_USAGECOUNT_ONE       (1U << BUF_USAGECOUNT_SHIFT)
+#define BUF_USAGECOUNT_MASK      (MASK(BUF_USAGECOUNT_BITS) << BUF_USAGECOUNT_SHIFT)
 
 /* flags related definitions */
-#define BUF_FLAG_SHIFT \
-	(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS)
-#define BUF_FLAG_MASK \
-	(((1U << BUF_FLAG_BITS) - 1) << BUF_FLAG_SHIFT)
+#define BUF_FLAG_MASK            (MASK(BUF_FLAG_BITS) << BUF_FLAG_SHIFT)
 
 /* Get refcount and usagecount from buffer state */
 #define BUF_STATE_GET_REFCOUNT(state) \
-	((state) & BUF_REFCOUNT_MASK)
+	(((state) & BUF_REFCOUNT_MASK) >> BUF_REFCOUNT_SHIFT)
 #define BUF_STATE_GET_USAGECOUNT(state) \
 	(((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT)
 
@@ -81,8 +79,7 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
  * entry associated with the buffer's tag.
  */
 
-#define BUF_DEFINE_FLAG(flagno)	\
-	(1U << (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + (flagno)))
+#define BUF_DEFINE_FLAG(flagno)	(1U << (BUF_FLAG_SHIFT + (flagno)))
 
 /* buffer header is locked */
 #define BM_LOCKED					BUF_DEFINE_FLAG( 0)

view thread (35+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox