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

Hi,

On 2025-10-04 09:05:45 +0200, Matthias van de Meent wrote:
> On Tue, 23 Sept 2025 at 00:14, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2025-09-15 19:05:37 -0400, Andres Freund wrote:
> > > Here are the first few cleaned up patches implementing the above steps, as
> > > well as some cleanups. I included a commit from another thread, as it
> > > conflicts with these changes, and we really should apply it - and it's
> > > arguably required to make the changes viable, as it removes one more use of
> > > PinBuffer_Locked().
> > >
> > > Another change included is to not return the buffer with the spinlock held
> > > from StrategyGetBuffer(), and instead pin the buffer in freelist.c. The reason
> > > for that is to reduce the most common PinBuffer_locked() call. By definition
> > > PinBuffer_locked() will become a bit slower due to 0003. But even without 0003
> > > it 0002 is faster than master. And the previous approach also just seems
> > > pretty unclean. I don't love that it requires the new TrackNewBufferPin(),
> > > but I don't really have a better idea.
> > >
> > > I invite particular attention to the commit message for 0003 as well as the
> > > comment changes in buf_internals.h within.
> >
> > Robert looked at the patches while we were chatting, and I addressed his
> > feedback in this new version.
>
> I like these changes, and have some minor comments:

Thank for reviewing!

> 0001 ensures that ReadRecentBuffer increments the usage counter, which
> someone who uses an access strategy may want to prevent. I know this
> isn't exactly new behaviour, but something I noticed anyway. Apart
> from that observation, LGTM

Are you proposing to change behaviour? Right now ReadRecentBuffer doesn't even
accept a strategy, so I don't really see this as something that needs to be
tackled at this point.

I'm not sure I see any real use cases for ReadRecentBuffer() that would
benefit from a strategy, but I very well might just not be thinking wide
enough.

> 0002 has a FIXME in a comment in GetVictimBuffer. Assuming it's about
> the comment itself needing updates

Indeed.

> , how about:
>
> + * Ensure, before we pin a victim buffer, that there's a free refcount
> + * entry, and a resource owner slot for the pin.
>
> Again, LGTM.

WFM.

> 0003's UnlockBufHdrExt:
> This is implemented with CAS, even when we only want to change bits we
> know the state of (or could know, if we spent the effort).
> Given its inline nature, wouldn't it be better to use atomic_sub
> instructions? Or is this to handle cases where the bits we want to
> (un)set might be (un)set by a concurrent process?

Yes, it's to handle concurrent changes to the buffer state.

> If the latter, could we specialize this to do a single atomic_sub
> whenever we want to change state bits that we know can be only changed
> whilst holding the spinlock?

We probably could optimize some cases as an atomic-sub, some others as an
atomic-and and others again as an atomic-or. The latter to however are
implemented as a CAS on x86 anyway...

After 0004 I don't think any of the paths using this are actually particularly
hot, so I'm somewhat doubtful it's worth to try to optimize this too much. If
there are hot paths, we really should try to work towards not even needing the
buffer header spinlock, that has a bigger impact that improving the code for
unlocking the buffer header...

Greetings,

Andres Freund