vircommand: Various mass close improvements

[PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

Posted by Michal Privoznik 2 years, 5 months ago

The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
enough bitmap so that subsequent call to
virCommandMassCloseGetFDsDir() can just set the bit instead of
expanding memory (this code runs in a forked off child and thus
using async-signal-unsafe functions like malloc() is a bit
tricky).

But on some systems the limit for opened FDs is virtually
non-existent (typically macOS Ventura started reporting EINVAL).

But with both glibc and musl using malloc() after fork() is safe.
And with sufficiently new glib too, as it's using malloc() with
newer releases instead of their own allocator.

Therefore, pick a sufficiently large value (glibc falls back to
256, [1], so 1024 should be good enough) to fall back to and make
the error non-fatal.

1: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getdtsz.c;h=4c5a6208067d2f9eaaac6dba652702fb4af9b7e3;hb=HEAD
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/vircommand.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 822b9487f9..8c06e19723 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -500,7 +500,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds,
             return -1;
         }
 
-        ignore_value(virBitmapSetBit(fds, fd));
+        virBitmapSetBitExpand(fds, fd);
     }
 
     if (rc < 0)
@@ -544,10 +544,8 @@ virCommandMassCloseFrom(virCommand *cmd,
      * Therefore we can safely allocate memory here (and transitively call
      * opendir/readdir) without a deadlock. */
 
-    if (openmax < 0) {
-        virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed"));
-        return -1;
-    }
+    if (openmax <= 0)
+        openmax = 1024;
 
     fds = virBitmapNew(openmax);
 
-- 
2.41.0

Re: [PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

Posted by Daniel P. Berrangé 2 years, 5 months ago

On Tue, Aug 29, 2023 at 05:13:08PM +0200, Michal Privoznik wrote:
> The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
> enough bitmap so that subsequent call to
> virCommandMassCloseGetFDsDir() can just set the bit instead of
> expanding memory (this code runs in a forked off child and thus
> using async-signal-unsafe functions like malloc() is a bit
> tricky).
> 
> But on some systems the limit for opened FDs is virtually
> non-existent (typically macOS Ventura started reporting EINVAL).
> 
> But with both glibc and musl using malloc() after fork() is safe.
> And with sufficiently new glib too, as it's using malloc() with
> newer releases instead of their own allocator.
> 
> Therefore, pick a sufficiently large value (glibc falls back to
> 256, [1], so 1024 should be good enough) to fall back to and make
> the error non-fatal.

That's not suitable for macOS. THey have a constant OPEN_MAX we
should be using that is way bigger than 1024.

> 
> 1: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getdtsz.c;h=4c5a6208067d2f9eaaac6dba652702fb4af9b7e3;hb=HEAD
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/vircommand.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 822b9487f9..8c06e19723 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -500,7 +500,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds,
>              return -1;
>          }
>  
> -        ignore_value(virBitmapSetBit(fds, fd));
> +        virBitmapSetBitExpand(fds, fd);
>      }
>  
>      if (rc < 0)
> @@ -544,10 +544,8 @@ virCommandMassCloseFrom(virCommand *cmd,
>       * Therefore we can safely allocate memory here (and transitively call
>       * opendir/readdir) without a deadlock. */
>  
> -    if (openmax < 0) {
> -        virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed"));
> -        return -1;
> -    }
> +    if (openmax <= 0)
> +        openmax = 1024;
>  
>      fds = virBitmapNew(openmax);
>  
> -- 
> 2.41.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

Posted by Michal Prívozník 2 years, 5 months ago

On 8/29/23 17:20, Daniel P. Berrangé wrote:
> On Tue, Aug 29, 2023 at 05:13:08PM +0200, Michal Privoznik wrote:
>> The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
>> enough bitmap so that subsequent call to
>> virCommandMassCloseGetFDsDir() can just set the bit instead of
>> expanding memory (this code runs in a forked off child and thus
>> using async-signal-unsafe functions like malloc() is a bit
>> tricky).
>>
>> But on some systems the limit for opened FDs is virtually
>> non-existent (typically macOS Ventura started reporting EINVAL).
>>
>> But with both glibc and musl using malloc() after fork() is safe.
>> And with sufficiently new glib too, as it's using malloc() with
>> newer releases instead of their own allocator.
>>
>> Therefore, pick a sufficiently large value (glibc falls back to
>> 256, [1], so 1024 should be good enough) to fall back to and make
>> the error non-fatal.
> 
> That's not suitable for macOS. THey have a constant OPEN_MAX we
> should be using that is way bigger than 1024.

Yeah, it's 2^63-1 per:

https://github.com/eradman/entr/issues/63#issuecomment-776758771

If we really use that it's going to take ages to iterate over all FDs.
OTOH, the next commit switches over to /dev/fd where we can learn about
opened FDs, so it's not that bad. Except we would be allocating
humongous virBitmap (1 EiB). what seems to be the right default then?

Michal

Re: [PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

Posted by Daniel P. Berrangé 2 years, 5 months ago

On Wed, Aug 30, 2023 at 08:26:02AM +0200, Michal Prívozník wrote:
> On 8/29/23 17:20, Daniel P. Berrangé wrote:
> > On Tue, Aug 29, 2023 at 05:13:08PM +0200, Michal Privoznik wrote:
> >> The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
> >> enough bitmap so that subsequent call to
> >> virCommandMassCloseGetFDsDir() can just set the bit instead of
> >> expanding memory (this code runs in a forked off child and thus
> >> using async-signal-unsafe functions like malloc() is a bit
> >> tricky).
> >>
> >> But on some systems the limit for opened FDs is virtually
> >> non-existent (typically macOS Ventura started reporting EINVAL).
> >>
> >> But with both glibc and musl using malloc() after fork() is safe.
> >> And with sufficiently new glib too, as it's using malloc() with
> >> newer releases instead of their own allocator.
> >>
> >> Therefore, pick a sufficiently large value (glibc falls back to
> >> 256, [1], so 1024 should be good enough) to fall back to and make
> >> the error non-fatal.
> > 
> > That's not suitable for macOS. THey have a constant OPEN_MAX we
> > should be using that is way bigger than 1024.
> 
> Yeah, it's 2^63-1 per:
> 
> https://github.com/eradman/entr/issues/63#issuecomment-776758771

IIUC, that's referring to the result of sysconf(_SC_OPEN_MAX)
which is returning -1.

I was refering to a literal constant OPEN_MAX which is reported
as being 10k based on the debug logs shown in commit message of

  https://gitlab.com/libvirt/libvirt/-/merge_requests/283


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|