Re: AIO v2.5

On Mon, Mar 24, 2025 at 10:30:27PM -0400, Andres Freund wrote:
> On 2025-03-24 17:45:37 -0700, Noah Misch wrote:
> > (We may be due for a test mode that does smgrreleaseall() at every
> > CHECK_FOR_INTERRUPTS()?)
>
> I suspect we are. I'm a bit afraid of even trying...
>
> ...
>
> It's extremely slow - but at least the main regression as well as the aio tests pass!

One less thing!

> > > I however don't particularly like the *start* or *prep* names, I've gone back
> > > and forth on those a couple times. I could see "begin" work uniformly across
> > > those.
> >
> > For ease of new readers understanding things, I think it helps for the
> > functions that advance PgAioHandleState to have names that use words from
> > PgAioHandleState. It's one less mapping to get into the reader's head.
>
> Unfortunately for me it's kind of the opposite in this case, see below.
>
>
> > "Begin", "Start" and "prep" are all outside that taxonomy, making the reader
> > learn how to map them to the taxonomy. What reward does the reader get at the
> > end of that exercise? I'm not seeing one, but please do tell me what I'm
> > missing here.
>
> Because the end state varies, depending on the number of previously staged
> IOs, the IO method and whether batchmode is enabled, I think it's better if
> the "function naming pattern" (i.e. FileStartReadv, smgrstartreadv etc) is
> *not* aligned with an internal state name. It will just mislead readers to
> think that there's a deterministic mapping when that does not exist.

That's fair. Could we provide the mapping in a comment, something like the
following?

--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -34,5 +34,10 @@
* linearly through all states.
*
- * State changes should all go through pgaio_io_update_state().
+ * State changes should all go through pgaio_io_update_state(). Its callers
+ * use these naming conventions:
+ *
+ * - A "start" function (e.g. FileStartReadV()) moves an IO from
+ * PGAIO_HS_HANDED_OUT to at least PGAIO_HS_STAGED and at most
+ * PGAIO_HS_COMPLETED_LOCAL.
*/
typedef enum PgAioHandleState

> That's not an excuse for pgaio_io_prep* though, that's a pointlessly different
> naming that I just stopped seeing.
>
> I'll try to think more about this, perhaps I can make myself see your POV
> more.

> > As the patch stands, LockBufferForCleanup() can succeed when
> > ConditionalLockBufferForCleanup() would have returned false.
>
> That's already true today, right? In master ConditionalLockBufferForCleanup()
> for temp buffers checks LocalRefCount, whereas LockBufferForCleanup() doesn't.

I'm finding a LocalRefCount check under LockBufferForCleanup:

LockBufferForCleanup(Buffer buffer)
{
...
CheckBufferIsPinnedOnce(buffer);

CheckBufferIsPinnedOnce(Buffer buffer)
{
if (BufferIsLocal(buffer))
{
if (LocalRefCount[-buffer - 1] != 1)
elog(ERROR, "incorrect local pin count: %d",
LocalRefCount[-buffer - 1]);
}
else
{
if (GetPrivateRefCount(buffer) != 1)
elog(ERROR, "incorrect local pin count: %d",
GetPrivateRefCount(buffer));
}
}

> > Like the comment, I expect it's academic today. I expect it will stay
> > academic. Anything that does a cleanup will start by reading the buffer,
> > which will resolve any refcnt the AIO subsystems holds for a read. If there's
> > an AIO write, the LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE) will block on
> > that. How about just removing the ConditionalLockBufferForCleanup() changes
> > or replacing them with a comment (like the present paragraph)?
>
> I think we'll need an expanded version of what I suggest once we have writes -
> but as you say, it shouldn't matter as long as we only have reads. So I think
> moving the relevant changes, with adjusted caveats, to the bufmgr: write
> change makes sense.

Moving those changes works for me. I'm not currently seeing the need under
writes, but that may get clearer upon reaching those patches.

> > > > /* ---
> > > > * Opt-in to using AIO batchmode.
> > > > *
> > > > * Submitting IO in larger batches can be more efficient than doing so
> > > > * one-by-one, particularly for many small reads. It does, however, require
> > > > * the ReadStreamBlockNumberCB callback to abide by the restrictions of AIO
> > > > * batching (c.f. pgaio_enter_batchmode()). Basically, the callback may not:
> > > > * a) block without first calling pgaio_submit_staged(), unless a
> > > > * to-be-waited-on lock cannot be part of a deadlock, e.g. because it is
> > > > * never acquired in a nested fashion
> > > > * b) directly or indirectly start another batch pgaio_enter_batchmode()
> >
> > I think a callback could still do:
> >
> > pgaio_exit_batchmode()
> > ... arbitrary code that might reach pgaio_enter_batchmode() ...
> > pgaio_enter_batchmode()
>
> Yea - but I somehow doubt there are many cases where it makes sense to
> deep-queue IOs within the callback. The cases I can think of are things like
> ensuring the right VM buffer is in s_b. But if it turns out to be necessary,
> what you seuggest would be an out.

I don't foresee a callback specifically wanting to batch, but callbacks might
call into other infrastructure that can elect to batch. The exit+reenter
pattern would be better than adding no-batch options to other infrastructure.

> Do you think it's worth mentioning the above workaround? I'm mildly inclined
> that not.

Perhaps not in that detail, but perhaps we can rephrase (b) to not imply
exit+reenter is banned. Maybe "(b) start another batch (without first exiting
one)". It's also fine as-is, though.

> If it turns out to be actually useful to do nested batching, we can change it
> so that nested batching *is* allowed, that'd not be hard.

Good point.

> I'm ok with all of these. In order of preference:
>
> 1) READ_STREAM_USE_BATCHING or READ_STREAM_BATCH_OK
> 2) READ_STREAM_BATCHMODE_AWARE
> 3) READ_STREAM_CALLBACK_BATCHMODE_AWARE

Same for me.