Re: [pve-devel] [PATCH qemu-server v2 6/6] feature #1027: virtio-9p & virtio-fs support

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

Without going too much into this series itself, I've been using
virtiofsd manually myself lately and immediately ran into an issue that
we also need to address, see below:

On Fri, Dec 23, 2022 at 02:10:07PM +0100, Markus Frank wrote:
> adds support for sharing directorys with a guest vm
> 
> virtio-9p can be simply started with qemu.
> virtio-fs needs virtiofsd to be started before qemu.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  PVE/API2/Qemu.pm  |  20 ++++++-
>  PVE/QemuServer.pm | 135 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 154 insertions(+), 1 deletion(-)
> 
> @@ -4158,6 +4244,40 @@ sub config_to_command {
>      return wantarray ? ($cmd, $vollist, $spice_port) : $cmd;
>  }
>  
> +sub extract_dir_path {
> +    my ($nodename, $dirid) = @_;
> +    my $dir_config = PVE::DirConfig::load_config($nodename);
> +    my $path = $dir_config->{$dirid};
> +    if (!$path) {
> +	die "Directory ID $dirid does not exist\n";
> +    }
> +    if (! -e $path) {
> +	die "Path $path does not exist\n";
> +    }
> +    if ((-e $path) && (! -d $path)) {
> +	die "Path $path exists but is not a directory\n"
> +    }
> +    return $path;
> +}
> +
> +sub start_virtiofs {
> +    my ($vmid, $path, $fsid) = @_;
> +    # virtiofsd does not run in background until vhost-user connects
> +    # to the socket, so it has to be started in a fork or with a tool
> +    # like daemonize

This is not really useful. The code now basically daemonizes it twice
and we have a race between virtiofsd opening the socket and qemu
connecting to it, which will cause qemu to fail and virtiofsd to linger.

Instead of using `--socket-path we should setup the socket ourselves and
pass `--fd` to virtiofsd, and not pass `--daemonize` because the point
where it happens is completely useless.

One *hacky* alternative would be to wait for its "Waiting for ..."
message.
Alternatively we could take this upstream and perhaps end up patching
it to daemonize between listen() and accept() as that would be a lot
more useful. If we do that, we should also consider a few more changes,
like an option to pass an already `accept()`ed fd, or an option to allow
multiple connections (so that each accept() will just fork off a new
instance - not necessarily useful for PVE, but useful for regular VMs,
iff done in a way that one can write a systemd service unit for shares
where virtiofsd is *correctly* daemonizing for it...)

> +
> +    my $pid = fork();
> +    my $socketstr = "--socket-path=/var/run/virtiofsd/vm$vmid-fs$fsid";
> +
> +    if ($pid == 0) {
> +	run_command(['/usr/lib/kvm/virtiofsd', '--daemonize', $socketstr,
> +		'-o', "source=$path", '-o', 'cache=always']);
> +	POSIX::_exit(0);
> +    } elsif (!defined($pid)) {
> +        die "could not fork to start virtiofsd";
> +    }
> +}
> +
>  sub check_rng_source {
>      my ($source) = @_;
>