Hi,
On 2025-05-08 19:22:27 -0700, Noah Misch wrote:
> 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.
I pushed it this way just now.
Thanks again for Alexander for reporting & investigating the issues and for
Noah's review.
Greetings,
Andres Freund
