Re: AIO v2.5

On Sun, Mar 23, 2025 at 11:11:53AM -0400, Andres Freund wrote:
> On 2025-03-22 17:20:56 -0700, Noah Misch wrote:
> > On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> > > Not sure yet how to best disable testing io_uring in this case. We can't
> > > just query EXEC_BACKEND from pg_config.h unfortunately. I guess making the
> > > initdb not fail and checking the error log would work, but that doesn't work
> > > nicely with Cluster.pm.
> >
> > How about "postgres -c io_method=io_uring -C <anything>":
> >
> > --- a/src/test/modules/test_aio/t/001_aio.pl
> > +++ b/src/test/modules/test_aio/t/001_aio.pl
> > @@ -29,7 +29,13 @@ $node_worker->stop();
> > # Test io_method=io_uring
> > ###
> >
> > -if ($ENV{with_liburing} eq 'yes')
> > +sub have_io_uring
> > +{
> > + local %ENV = $node_worker->_get_env(); # any node works
> > + return run_log [qw(postgres -c io_method=io_uring -C io_method)];
> > +}
> > +
> > +if (have_io_uring())
> > {
> > my $node_uring = create_node('io_uring');
> > $node_uring->start();
>
> Yea, that's a good idea.
>
> One thing that doesn't seem great is that it requires a prior node - what if
> we do -c io_method=invalid' that would report the list of valid GUC options,
> so we could just grep for io_uring?

Works for me.

> > One idea so far is to comment on valid states after some IoMethodOps
> > callbacks:

> I think these are a good idea. I added those to the copy-edit patch, with a
> few more tweaks:

The tweaks made it better.

> > > Subject: [PATCH v2.11 04/27] aio: Add liburing dependency

> > > + [AC_DEFINE([USE_LIBURING], 1, [Define to build with io_uring support. (--with-liburing)])])
> > > +AC_MSG_RESULT([$with_liburing])
> > > +AC_SUBST(with_liburing)
> > >
> > > #
> > > # UUID library
> > > @@ -1463,6 +1471,9 @@ elif test "$with_uuid" = ossp ; then
> > > fi
> > > AC_SUBST(UUID_LIBS)
> > >
> > > +if test "$with_liburing" = yes; then
> > > + PKG_CHECK_MODULES(LIBURING, liburing)
> > > +fi
> >
> > We usually put this right after the AC_MSG_CHECKING ... AC_SUBST block.
>
> We don't really seem to do that for "dependency checks" in general, e.g.
> PGAC_CHECK_PERL_CONFIGS, PGAC_CHECK_PYTHON_EMBED_SETUP, PGAC_CHECK_READLINE,
> dependency dependent AC_CHECK_LIB calls, .. later in configure.ac than the
> defnition of the option.

AC_CHECK_LIB stays far away, yes.

> But you're right that the PKG_CHECK_MODULES calls are closer-by. And I'm happy
> to move towards having the code for each dep all in one place, so moved.
>
>
> A related thing: We seem to have no order of the $with_ checks that I can
> discern. Should the liburing check be at a different place?

No opinion on that one. It's fine.

> > This currently has unrelated stuff separating them. Also, with the
> > exception of icu, we follow PKG_CHECK_MODULES uses by absorbing flags from
> > pkg-config and use AC_CHECK_LIB to add the actual "-l".
>
> I think for liburing I was trying to follow ICU's example - injecting CFLAGS
> and LIBS just in the parts of the build dir that needs them.
>
> For LIBS I think I did so:
>
> diff --git a/src/backend/Makefile b/src/backend/Makefile
> ...
> +# The backend conditionally needs libraries that most executables don't need.
> +LIBS += $(LDAP_LIBS_BE) $(ICU_LIBS) $(LIBURING_LIBS)
>
> But ugh, for some reason I didn't do that for LIBURING_CFLAGS. In the v1.x
> version of aio I had
> aio:src/backend/storage/aio/Makefile:override CPPFLAGS += $(LIBURING_CFLAGS)
>
> but somehow lost that somewhere along the way to v2.x
>
>
> I think I like targetting where ${LIB}_LIBS and ${LIB}_CFLAGS are applied more
> narrowly better than just adding to the global CFLAGS, CPPFLAGS, LDFLAGS.

Agreed.

> somewhat inclined to add it LIBURING_CFLAGS in src/backend rather than
> src/backend/storage/aio/ though.
>
> But I'm also willing to do it entirely differently.

The CPPFLAGS addition, located wherever makes sense, resolves that point.

> > > --- a/doc/src/sgml/installation.sgml
> > > +++ b/doc/src/sgml/installation.sgml
> >
> > lz4 and other deps have a mention in <sect1 id="install-requirements">, in
> > addition to sections edited here.
>
> Good point.
>
> Although once more I feel defeated by the ordering used :)
>
> Hm, that list is rather incomplete. At least libxml, libxslt, selinux, curl,
> uuid, systemd, selinux and bonjour aren't listed.
>
> Not sure if it makes sense to add liburing, given that?

That's a lot of preexisting incompleteness. I withdraw the point about <sect1
id="install-requirements">.

Unrelated to the above, another question about io_uring:

commit da722699 wrote:
> +/*
> + * Need to submit staged but not yet submitted IOs using the fd, otherwise
> + * the IO would end up targeting something bogus.
> + */
> +void
> +pgaio_closing_fd(int fd)

An IO in PGAIO_HS_STAGED clearly blocks closing the IO's FD, and an IO in
PGAIO_HS_COMPLETED_IO clearly doesn't block that close. For io_method=worker,
closing in PGAIO_HS_SUBMITTED is okay. For io_method=io_uring, is there a
reference about it being okay to close during PGAIO_HS_SUBMITTED? I looked
awhile for an authoritative view on that, but I didn't find one. If we can
rely on io_uring_submit() returning only after the kernel has given the
io_uring its own reference to all applicable file descriptors, I expect it's
okay to close the process's FD. If the io_uring acquires its reference later
than that, I expect we shouldn't close before that later time.