virtiofsd currently depends on vhost 0.13.3, which does not contain important fixes, e.g. https://github.com/rust-vmm/vhost/pull/290

Bumping the dependency to 0.15.0 triggered several related dependency version bumps and a couple of minor code refactorings.

Signed-off-by: Peter Oskolkov [email protected]

Hi @posk, thanks for doing this. We just merged !306 (merged) that also updates the vhost crate. So, I'll close this MR. Feel free to open a new one if we missed something.

When used with libvirt, the new rust daemon crashes with this message:

virtiofsd[10356]: Use of deprecated option format '-o': Please specify options without it (e.g., '--cache auto' instead of '-o cache=auto')
virtiofsd[10356]: Waiting for vhost-user socket connection...
virtiofsd[10356]: Client connected, servicing requests
virtiofsd[10356]: Waiting for daemon failed: HandleRequest(ReqHandlerError(Custom { kind: Other, error: SeekEnd(Os { code: 29, kind: NotSeekable, message: "Illegal seek" }) }))

The config file used:

<filesystem type="mount" accessmode="passthrough">
  <driver type="virtiofs" queue="1024"/>
  <binary path="/home/launch_virtiofs">
    <thread_pool size="1"/>
  </binary>
  <source dir="/home/Windows"/>
  <target dir="Exchange"/>
  <address type="pci" domain="0x0000" bus="0x0a" slot="0x00" function="0x0"/>
</filesystem>

The launch_virtiofs script:

#!/bin/bash
/usr/lib/virtiofsd --syslog --log-level debug "$@"

Host operating system: EndeavourOS, Kernel: 6.2.12-arch1-1, Guest operating system: Windows 11, ISO installer virtio-win-0.1.229.iso was used in the guest OS.

Update rust-vmm dependencies, necessitating these changes:

• VhostUserDaemon::start() now takes a mutable reference to the Listener instead of ownership,
• The exit event EventFd has been replaced by an EventConsumer and EventNotifier pair. We used to store kill_evt as an EventFd to write to in VhostUserFsBackend::drop(), but this should actually be already done by VhostUserHandler::drop() (in vhost-user-backend) ever since vhost commit f6b7e49e[1], so I think it’s time to drop it. VhostUserHandler directly owns our VhostUserFsBackend without further wrapping, so both are dropped at exactly the same time.

Note that this keeps vm-memory on 0.17.1 instead of 0.17.2 or 0.18.0: To update to 0.18.0, we’d need some further changes to other rust-vmm crates, and updating to 0.17.2 creates some conflicts in rust-vmm crates because its re-export of GuestAddressSpace isn’t very useful (its memory type is the new GuestMemory, but it would need to be the old GuestMemory aka now GuestMemoryBackend to be useful). I’ll send a PR for the 0.17.2 issue to vm-memory, but for now just cap the version at 0.17.1.

[1] https://github.com/rust-vmm/vhost/commit/f6b7e49e3f9ea (“handler: send exit event when destructing”)

German Maglione (7250c773) at 12 Mar 18:18

... and 1 more commit

Update rust-vmm dependencies, necessitating these changes:

• VhostUserDaemon::start() now takes a mutable reference to the Listener instead of ownership,
• The exit event EventFd has been replaced by an EventConsumer and EventNotifier pair. We used to store kill_evt as an EventFd to write to in VhostUserFsBackend::drop(), but this should actually be already done by VhostUserHandler::drop() (in vhost-user-backend) ever since vhost commit f6b7e49e[1], so I think it’s time to drop it. VhostUserHandler directly owns our VhostUserFsBackend without further wrapping, so both are dropped at exactly the same time.

Note that this keeps vm-memory on 0.17.1 instead of 0.17.2 or 0.18.0: To update to 0.18.0, we’d need some further changes to other rust-vmm crates, and updating to 0.17.2 creates some conflicts in rust-vmm crates because its re-export of GuestAddressSpace isn’t very useful (its memory type is the new GuestMemory, but it would need to be the old GuestMemory aka now GuestMemoryBackend to be useful). I’ll send a PR for the 0.17.2 issue to vm-memory, but for now just cap the version at 0.17.1.

[1] https://github.com/rust-vmm/vhost/commit/f6b7e49e3f9ea (“handler: send exit event when destructing”)

it is, so I think is safe to close this one and merge !306 (merged) instead.

is the ppc64 issue solved?

German Maglione (bb1cb8d7) at 11 Feb 09:49

... and 1 more commit

German Maglione (bb1cb8d7) at 09 Feb 11:10

... and 1 more commit

The newfstatat syscall is not available on sparc64.

I don’t know where newfstatat would even be used right now, I think we actually always use statx

Sometimes even if we don't use a specific syscall, if we call it through libc it can happen that libc could call that syscall on different situations/archs, and/or musl calling a different one than glibc.

I don't think is the case here, so thanks @glaubitz

Merge Request Required Information

Summary of Changes

When giving event FDs to the vhost-user back-end, QEMU currently does not await the back-end completing that request. As a result, it is possible to lose guest interrupts because QEMU assumes the back-end already uses the new FD when it actually doesn’t yet. A single lost interrupt is enough to make a vhost-user device like virtio-fs hang irrecoverably.

The backported commit makes QEMU wait until the back-end has installed the new FD via the REPLY_ACK feature, which virtiofsd does support.

git-backport-diff against upstream:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/1:[----] [--] 'vhost-user: make vhost_set_vring_file() synchronous'

Approved Development Ticket(s)

Resolves: RHEL-147422

I think a comment explaining why this requires consuming self, will be nice

(not particular to this piece of code)

Thinking from the lib's POV, shouldn't it be the main/caller who tests this? (even though the lib provides the probe function)

Even understanding that features should be additive, I don't really like this (I mean not only here, but I just mention here), we can expect in the future something that requires the iouring feature to be disabled, so I think these allows are too broad

I'm not sure, if for clarity to move this type of definition to its own file, honestly mod.rs is getting a bit messy (it was already before not because of this :) )

so, we will use this one even without the "iouring" feature?

not for now, but do you think we can encode this using the type system?

also, in a couple of places when this function is called (file_traits_async.rs), there is no //SAFETY: comment, and in passthough/mod.rs this invariant is not mentioned

not asking to change anything, can you just remind me what alternatives we managed besides makes these Send?