From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7BB75E8B0 for ; Wed, 19 Jul 2023 14:09:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5DF6B69B1 for ; Wed, 19 Jul 2023 14:08:56 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 19 Jul 2023 14:08:52 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8629540F8A for ; Wed, 19 Jul 2023 14:08:52 +0200 (CEST) Date: Wed, 19 Jul 2023 14:08:45 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230706105421.54949-1-m.frank@proxmox.com> <20230706105421.54949-5-m.frank@proxmox.com> In-Reply-To: <20230706105421.54949-5-m.frank@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1689767039.4bobtqbdw7.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.070 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [memory.pm, qemuserver.pm, gitlab.com, proxmox.com] Subject: Re: [pve-devel] [PATCH qemu-server v6 1/3] feature #1027: virtio-fs support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Jul 2023 12:09:26 -0000 On July 6, 2023 12:54 pm, Markus Frank wrote: > adds support for sharing directorys with a guest vm >=20 > virtio-fs needs virtiofsd to be started. >=20 > In order to start virtiofsd as a process (despite being a daemon it is do= es not run > in the background), a double-fork is used. >=20 > virtiofsd should close itself together with qemu. >=20 > There are the parameters dirid & tag > and the optional parameters direct-io & cache. >=20 > The dirid gets mapped to the path on the current node. > The tag parameter is for choosing the tag-name that is used with the > mount command. >=20 > example config: > ``` > virtiofs0: foo,tag=3Dtag1,direct-io=3D1,cache=3Dalways > virtiofs1: dirid=3Dbar,tag=3Dtag2,cache=3Dnever > ``` >=20 > For information on the optional parameters see there: > https://gitlab.com/virtio-fs/virtiofsd/-/blob/main/README.md some information why virtiofsd is incompatible with hugepages, and why it needs a different memdev identifier would be nice to have, either in the commit message, or as a comment.. nits/questions below mainly if you respin with the guest-common related feedback adressed, if not, these can obviously be done as a follow up as well - but please also see the cover letter reply, the problems listed there likely mean this patch in particular still needs some rework anyway! >=20 > Signed-off-by: Markus Frank > --- > PVE/QemuServer.pm | 157 +++++++++++++++++++++++++++++++++++++++ > PVE/QemuServer/Memory.pm | 25 +++++-- > debian/control | 1 + > 3 files changed, 177 insertions(+), 6 deletions(-) >=20 > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 940cdac..3a8b4c5 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -43,6 +43,7 @@ use PVE::PBSClient; > use PVE::RESTEnvironment qw(log_warn); > use PVE::RPCEnvironment; > use PVE::Storage; > +use PVE::Mapping::DIR; > use PVE::SysFSTools; > use PVE::Systemd; > use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_= glob_foreach get_host_arch $IPV6RE); > @@ -276,6 +277,35 @@ my $rng_fmt =3D { > }, > }; > =20 > +my $virtiofs_fmt =3D { > + 'dirid' =3D> { > + type =3D> 'string', > + default_key =3D> 1, > + description =3D> "dirid of directory you want to share with the guest V= M", > + format_description =3D> "virtiofs-dirid", > + }, the other two mapping types call this 'mapping', set a format and different format_description. maybe we should be consistent? nit: the description could just be "Mapping identifier of the directory mapping to be shared with the guest." or something similar, "dirid of directory" is rather redundant, and "you" seems wrong for a property description. > + 'tag' =3D> { > + type =3D> 'string', > + description =3D> "tag name for mounting in the guest VM", > + format_description =3D> "virtiofs-tag", > + }, > + 'cache' =3D> { > + type =3D> 'string', > + description =3D> "The caching policy the file system should use" > + ." (auto, always, never).", > + format_description =3D> "virtiofs-cache", > + enum =3D> [qw(auto always never)], > + optional =3D> 1, > + }, > + 'direct-io' =3D> { > + type =3D> 'boolean', > + description =3D> "Honor the O_DIRECT flag passed down by guest applicat= ions", > + format_description =3D> "virtiofs-directio", > + optional =3D> 1, > + }, what about 'ro'? should we also add 'xattr', 'acl' and the submount part here to control whether those are actually enabled for this particular guest? > +}; > +PVE::JSONSchema::register_format('pve-qm-virtiofs', $virtiofs_fmt); > + > my $meta_info_fmt =3D { > 'ctime' =3D> { > type =3D> 'integer', > @@ -838,6 +868,7 @@ while (my ($k, $v) =3D each %$confdesc) { > } > =20 > my $MAX_NETS =3D 32; > +my $MAX_VIRTIOFS =3D 10; > my $MAX_SERIAL_PORTS =3D 4; > my $MAX_PARALLEL_PORTS =3D 3; > my $MAX_NUMA =3D 8; > @@ -982,6 +1013,21 @@ my $netdesc =3D { > =20 > PVE::JSONSchema::register_standard_option("pve-qm-net", $netdesc); > =20 > +my $virtiofsdesc =3D { > + optional =3D> 1, > + type =3D> 'string', format =3D> $virtiofs_fmt, > + description =3D> "share files between host and guest", > +}; > +PVE::JSONSchema::register_standard_option("pve-qm-virtiofs", $virtiofsde= sc); > + > +sub max_virtiofs { > + return $MAX_VIRTIOFS; > +} > + > +for (my $i =3D 0; $i < $MAX_VIRTIOFS; $i++) { > + $confdesc->{"virtiofs$i"} =3D $virtiofsdesc; > +} > + > my $ipconfig_fmt =3D { > ip =3D> { > type =3D> 'string', > @@ -4100,6 +4146,25 @@ sub config_to_command { > push @$devices, '-device', $netdevicefull; > } > =20 > + my $onevirtiofs =3D 0; nit: my $virtiofs_enabled , or something similar ('onevirtiofs' sounds like there is exactly one virtiofs configured). > + for (my $i =3D 0; $i < $MAX_VIRTIOFS; $i++) { > + my $virtiofsstr =3D "virtiofs$i"; nit: variable naming could be better here - we usually call this "$opt" or "$key" ;) > + > + next if !$conf->{$virtiofsstr}; > + my $virtiofs =3D parse_property_string('pve-qm-virtiofs', $conf->{$virt= iofsstr}); > + next if !$virtiofs; > + > + push @$devices, '-chardev', "socket,id=3Dvirtfs$i,path=3D/var/run/virti= ofsd/vm$vmid-fs$i"; > + push @$devices, '-device', 'vhost-user-fs-pci,queue-size=3D1024' > + .",chardev=3Dvirtfs$i,tag=3D$virtiofs->{tag}"; > + > + $onevirtiofs =3D 1; > + } > + > + if ($onevirtiofs && $conf->{hugepages}){ > + die "hugepages not supported in combination with virtiofs\n"; > + } > + > if ($conf->{ivshmem}) { > my $ivshmem =3D parse_property_string($ivshmem_fmt, $conf->{ivshmem}); > =20 > @@ -4159,6 +4224,14 @@ sub config_to_command { > } > push @$machineFlags, "type=3D${machine_type_min}"; > =20 > + if ($onevirtiofs && !$conf->{numa}) { > + # kvm: '-machine memory-backend' and '-numa memdev' properties are > + # mutually exclusive > + push @$devices, '-object', 'memory-backend-file,id=3Dvirtiofs-mem' > + .",size=3D$conf->{memory}M,mem-path=3D/dev/shm,share=3Don"; > + push @$machineFlags, 'memory-backend=3Dvirtiofs-mem'; > + } > + > push @$cmd, @$devices; > push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags); > push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machin= eFlags); > @@ -4185,6 +4258,72 @@ sub config_to_command { > return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cm= d; > } > =20 > +sub start_virtiofs { > + my ($vmid, $fsid, $virtiofs) =3D @_; > + > + my $dir_list =3D PVE::Mapping::DIR::find_on_current_node($virtiofs->= {dirid}); > + > + if (!$dir_list || scalar($dir_list->@*) !=3D 1) { > + die "virtiofs needs exactly one mapping for this node\n"; > + } > + > + eval { > + PVE::Mapping::DIR::assert_valid($dir_list->[0]); > + }; > + if (my $err =3D $@) { > + die "Directory Mapping invalid: $err\n"; > + } > + > + my $dir_cfg =3D $dir_list->[0]; > + my $path =3D $dir_cfg->{path}; > + my $socket_path_root =3D "/var/run/virtiofsd"; > + mkdir $socket_path_root; > + my $socket_path =3D "$socket_path_root/vm$vmid-fs$fsid"; > + unlink($socket_path); > + my $socket =3D IO::Socket::UNIX->new( > + Type =3D> SOCK_STREAM, > + Local =3D> $socket_path, > + Listen =3D> 1, > + ) or die "cannot create socket - $!\n"; > + > + my $flags =3D fcntl($socket, F_GETFD, 0) > + or die "failed to get file descriptor flags: $!\n"; > + fcntl($socket, F_SETFD, $flags & ~FD_CLOEXEC) > + or die "failed to remove FD_CLOEXEC from file descriptor\n"; > + > + my $fd =3D $socket->fileno(); > + > + my $virtiofsd_bin =3D '/usr/libexec/virtiofsd'; > + > + my $pid =3D fork(); > + if ($pid =3D=3D 0) { > + for my $fd_loop (3 .. POSIX::sysconf( &POSIX::_SC_OPEN_MAX )) { > + POSIX::close($fd_loop) if ($fd_loop !=3D $fd); > + } > + my $pid2 =3D fork(); > + if ($pid2 =3D=3D 0) { > + my $cmd =3D [$virtiofsd_bin, "--fd=3D$fd", "--shared-dir=3D$path"]; > + push @$cmd, '--xattr' if ($dir_cfg->{xattr}); > + push @$cmd, '--posix-acl' if ($dir_cfg->{acl}); > + push @$cmd, '--announce-submounts' if ($dir_cfg->{submounts}); > + push @$cmd, '--allow-direct-io' if ($virtiofs->{'direct-io'}); > + push @$cmd, "--cache=3D$virtiofs->{'cache'}" if ($virtiofs->{'cache= '}); > + run_command($cmd); > + POSIX::_exit(0); > + } elsif (!defined($pid2)) { > + die "could not fork to start virtiofsd\n"; > + } else { > + POSIX::_exit(0); > + } > + } elsif (!defined($pid)) { > + die "could not fork to start virtiofsd\n"; > + } > + > + # return socket to keep it alive, > + # so that qemu will wait for virtiofsd to start > + return $socket; > +} > + > sub check_rng_source { > my ($source) =3D @_; > =20 > @@ -5740,6 +5879,19 @@ sub vm_start_nolock { > my ($cmd, $vollist, $spice_port, $pci_devices) =3D config_to_command= ($storecfg, $vmid, > $conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'}); > =20 > + my @sockets; > + for (my $i =3D 0; $i < $MAX_VIRTIOFS; $i++) { > + my $virtiofsstr =3D "virtiofs$i"; > + > + next if !$conf->{$virtiofsstr}; > + my $virtiofs =3D parse_property_string('pve-qm-virtiofs', $conf->{$virt= iofsstr}); > + next if !$virtiofs; > + > + > + my $socket =3D start_virtiofs($vmid, $i, $virtiofs); > + push @sockets, $socket; > + } > + > my $migration_ip; > my $get_migration_ip =3D sub { > my ($nodename) =3D @_; > @@ -6093,6 +6245,11 @@ sub vm_start_nolock { > =20 > PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-start'); > =20 > + foreach my $socket (@sockets) { > + shutdown($socket, 2); > + close($socket); > + } > + > return $res; > } > =20 > diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm > index 0601dd6..3b58b36 100644 > --- a/PVE/QemuServer/Memory.pm > +++ b/PVE/QemuServer/Memory.pm > @@ -278,6 +278,16 @@ sub config { > =20 > die "numa needs to be enabled to use hugepages" if $conf->{hugepages= } && !$conf->{numa}; > =20 > + my $onevirtiofs =3D 0; > + for (my $i =3D 0; $i < PVE::QemuServer::max_virtiofs(); $i++) { > + my $virtiofsstr =3D "virtiofs$i"; > + next if !$conf->{$virtiofsstr}; > + my $virtiofs =3D PVE::JSONSchema::parse_property_string('pve-qm-virtiof= s', $conf->{$virtiofsstr}); > + if ($virtiofs) { > + $onevirtiofs =3D 1; > + } > + } > + > if ($conf->{numa}) { > =20 > my $numa_totalmemory =3D undef; > @@ -290,7 +300,8 @@ sub config { > my $numa_memory =3D $numa->{memory}; > $numa_totalmemory +=3D $numa_memory; > =20 > - my $mem_object =3D print_mem_object($conf, "ram-node$i", $numa_memo= ry); > + my $memdev =3D $onevirtiofs ? "virtiofs-mem$i" : "ram-node$i"; > + my $mem_object =3D print_mem_object($conf, $memdev, $numa_memory); > =20 > # cpus > my $cpulists =3D $numa->{cpus}; > @@ -315,7 +326,7 @@ sub config { > } > =20 > push @$cmd, '-object', $mem_object; > - push @$cmd, '-numa', "node,nodeid=3D$i,cpus=3D$cpus,memdev=3Dram-no= de$i"; > + push @$cmd, '-numa', "node,nodeid=3D$i,cpus=3D$cpus,memdev=3D$memde= v"; > } > =20 > die "total memory for NUMA nodes must be equal to vm static memory\n" > @@ -329,13 +340,13 @@ sub config { > die "host NUMA node$i doesn't exist\n" > if !host_numanode_exists($i) && $conf->{hugepages}; > =20 > - my $mem_object =3D print_mem_object($conf, "ram-node$i", $numa_memory)= ; > - push @$cmd, '-object', $mem_object; > - > my $cpus =3D ($cores * $i); > $cpus .=3D "-" . ($cpus + $cores - 1) if $cores > 1; > =20 > - push @$cmd, '-numa', "node,nodeid=3D$i,cpus=3D$cpus,memdev=3Dram-node$= i"; > + my $memdev =3D $onevirtiofs ? "virtiofs-mem$i" : "ram-node$i"; > + my $mem_object =3D print_mem_object($conf, $memdev, $numa_memory); > + push @$cmd, '-object', $mem_object; > + push @$cmd, '-numa', "node,nodeid=3D$i,cpus=3D$cpus,memdev=3D$memdev"; > } > } > } > @@ -364,6 +375,8 @@ sub print_mem_object { > my $path =3D hugepages_mount_path($hugepages_size); > =20 > return "memory-backend-file,id=3D$id,size=3D${size}M,mem-path=3D$path,s= hare=3Don,prealloc=3Dyes"; > + } elsif ($id =3D~ m/^virtiofs-mem/) { > + return "memory-backend-file,id=3D$id,size=3D${size}M,mem-path=3D/dev/sh= m,share=3Don"; > } else { > return "memory-backend-ram,id=3D$id,size=3D${size}M"; > } > diff --git a/debian/control b/debian/control > index 49f67b2..f008a9b 100644 > --- a/debian/control > +++ b/debian/control > @@ -53,6 +53,7 @@ Depends: dbus, > socat, > swtpm, > swtpm-tools, > + virtiofsd, > ${misc:Depends}, > ${perl:Depends}, > ${shlibs:Depends}, > --=20 > 2.39.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20