Date: Thu, 8 May 2025 19:22:27 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alexander Lakhin <exclusion@gmail.com>, pgsql-hackers@postgresql.org,
	Thomas Munro <thomas.munro@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Robert Haas <robertmhaas@gmail.com>,
	Jakub Wartak <jakub.wartak@enterprisedb.com>,
	Jelte Fennema-Nio <postgres@jeltef.nl>,
	Antonin Houska <ah@cybertec.at>
Subject: Re: AIO v2.5
Message-ID: <20250509022227.5b.nmisch@google.com>
References: <3fhulsvzks4khqahl6tngcnuda7pyrrl7223tmjh2x2spralm3@iisldh6pcowy>
 <8160688e-db8e-4d7d-a7e7-ff366adf9717@gmail.com>
 <4nervqmqplfr23jrjvkp5tsumi6qgouhgjqlubf7ujrudw2epb@6mszddainl4u>
 <6a6756a0-bad5-4ee5-a591-c763af289940@gmail.com>
 <96abefe8-fa72-41f5-8840-0517125c24e3@gmail.com>
 <4qk3ehe6w7x7hfrldei2hefjcb7v7nfmj2owl2ir64craqcapz@kbrao22ljxeb>
 <062daca9-dfad-4750-9da8-b13388301ad9@gmail.com>
 <xg2s43enrr7lwvghh55kofxdj7o7zd6s6ocgmowel76oxyb2gm@eh6guzumnido>
 <20250503030511.9b.nmisch@google.com>
 <mvpm7ga3dfgz7bvum22hmuz26cariylmcppb3irayftc7bwk3r@l7gb6gr7azhc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <mvpm7ga3dfgz7bvum22hmuz26cariylmcppb3irayftc7bwk3r@l7gb6gr7azhc>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250509022227.5b.nmisch%40google.com>
Precedence: bulk

On Thu, May 08, 2025 at 09:06:18PM -0400, Andres Freund wrote:
> On 2025-05-02 20:05:11 -0700, Noah Misch wrote:
> > On Wed, Apr 30, 2025 at 04:00:35PM -0400, Andres Freund wrote:

> We do need to hold interrupts in a few other places, I think - with some debug
> infrastructure (things like calling ProcessBarrierSmgrRelease() whenever
> interrupts could be processed and calling CFI() in errstart() in its return
> false case) it's possible to find state confusions which trigger
> assertions. The issue is that pgaio_io_update_state() contains a
> pgaio_debug_io() and executing pgaio_closing_fd() in places that call
> pgaio_io_update_state() doesn't end well.  There's a similar danger with the
> debug message in pgaio_io_reclaim().
> 
> In the attached patch I added an assertion to pgaio_io_update_state()
> verifying that interrupts are held and added code to hold interrupts in the
> relevant places.

Works for me.

> > For the "no free IOs despite no in-flight IOs" case, I'd replace the
> > ereport(ERROR) with "return;" since we now know interrupt processing reclaimed
> > an IO.
> 
> Hm - it seems better to me to check if there are now free handles and return
> if that's the case, but to keep the error check in case there actually is no
> free IO? That seems like a not implausible bug...

Works for me.

> > Then decide what protection if any, we need against bugs causing an
> > infinite loop in caller pgaio_io_acquire().  What's the case motivating the
> > unbounded loop in pgaio_io_acquire(), as opposed to capping at two
> > pgaio_io_acquire_nb() calls?  If the theory is that pgaio_io_acquire() could
> > be reentrant, what scenario would reach that reentrancy?
> 
> I do not remember why I wrote this as an endless loop.  If you prefer I could
> change that as part of this patch.

I asked because it would be sad to remove the ereport(ERROR) like I proposed
and then have a bug cause a real infinite loop.  Removing the loop was one way
to prove that can't happen.  As you say, another way would be keeping the
ereport(ERROR) and guarding it with a free-handles check, like in your patch
today.  I don't have a strong preference between those.

> It does seem rather dangerous that errstart() processes interrupts for debug
> messages, but only if the debug message is actually logged. That's really a
> recipe for hard to find bugs.  I wonder if we should, at least in assertion
> mode, process interrupts even if not emitting the message.

Yes, that sounds excellent to have.