[PATCH v2 0/4] virtio-fs: initial support for DAX window

[Fotis Xenakis]

Changes since v1:
- Now targeting the development branch of virtio-fs QEMU [1]. This
is closer to upstream QEMU 5.0, e.g. in the FUSE version it supports.
Also, the DAX-related FUSE protocol operations are different in this
branch and, according to the virtio-fs devs should be more
future-proof. This is mainly reflected in the FUSE header
(fs/virtiofs/fuse_kernel.h).
- Compatible with latest upstream QEMU 5.0.
- The DAX window alignment requirements are communicated from the
device during FUSE_INIT instead of being hard-coded.
- When allocating the read buffer for FUSE_READ, contiguous physical
memory is ensured.

Note that for all operations except FUSE_READ, the arguments passed to
the virtqueues are much smaller than a page. That's why I have kept the
regular new() for memory allocations there (that's what other drivers
also do AFAICS). If someone was to shed more light into this, possibly
indicating all these allocations had better be made with
alloc_phys_contiguous_aligned(), I am willing to change to that.

To test it out:
1. Apply these patches.
2. Due to a bug in virtiofsd in [1], apply the patch in [2] to it (or
use the patched version from [3]). Hopefully this patch will be merged
soon.
3. Follow the virtio-fs QEMU how-to in [4]. Don't forget to add
"cache-size=" to the arguments passed to the respective QEMU device
flag.

References:
[1] virtio-fs QEMU development branch:
https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev
[2] virtio-fs QEMU patch:
https://www.redhat.com/archives/virtio-fs/2020-May/msg00053.html
[3] Patched virtio-fs QEMU:
https://github.com/foxeng/qemu/tree/fix-dax-fd
[4] Official virtio-fs QEMU how-to:
https://virtio-fs.gitlab.io/howto-qemu.html

Fotis Xenakis (4):
virtio-fs: update fuse protocol header
virtio-fs: implement FUSE_INIT map_alignment field
virtio-fs: add basic read using the DAX window
virtio-fs: refactor driver / fs

drivers/virtio-fs.cc | 44 +++-----
drivers/virtio-fs.hh | 34 ++++--
fs/virtiofs/fuse_kernel.h | 88 ++++++++++++++--
fs/virtiofs/virtiofs_i.hh | 24 +----
fs/virtiofs/virtiofs_vfsops.cc | 23 +++--
fs/virtiofs/virtiofs_vnops.cc | 184 +++++++++++++++++++++++++++------
6 files changed, 294 insertions(+), 103 deletions(-)

--
2.26.2

[Fotis Xenakis]

Copy from https://gitlab.com/virtio-fs/qemu
@21336c0f3d05a97f5c409bbc894c19d87259655c.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---
fs/virtiofs/fuse_kernel.h | 88 +++++++++++++++++++++++++++++++++++----
1 file changed, 81 insertions(+), 7 deletions(-)

diff --git a/fs/virtiofs/fuse_kernel.h b/fs/virtiofs/fuse_kernel.h
index 018a00a2..26e7de1b 100644
--- a/fs/virtiofs/fuse_kernel.h
+++ b/fs/virtiofs/fuse_kernel.h
@@ -38,6 +38,43 @@
*
* Protocol changelog:
*
+ * 7.1:
+ * - add the following messages:
+ * FUSE_SETATTR, FUSE_SYMLINK, FUSE_MKNOD, FUSE_MKDIR, FUSE_UNLINK,
+ * FUSE_RMDIR, FUSE_RENAME, FUSE_LINK, FUSE_OPEN, FUSE_READ, FUSE_WRITE,
+ * FUSE_RELEASE, FUSE_FSYNC, FUSE_FLUSH, FUSE_SETXATTR, FUSE_GETXATTR,
+ * FUSE_LISTXATTR, FUSE_REMOVEXATTR, FUSE_OPENDIR, FUSE_READDIR,
+ * FUSE_RELEASEDIR
+ * - add padding to messages to accommodate 32-bit servers on 64-bit kernels
+ *
+ * 7.2:
+ * - add FOPEN_DIRECT_IO and FOPEN_KEEP_CACHE flags
+ * - add FUSE_FSYNCDIR message
+ *
+ * 7.3:
+ * - add FUSE_ACCESS message
+ * - add FUSE_CREATE message
+ * - add filehandle to fuse_setattr_in
+ *
+ * 7.4:
+ * - add frsize to fuse_kstatfs
+ * - clean up request size limit checking
+ *
+ * 7.5:
+ * - add flags and max_write to fuse_init_out
+ *
+ * 7.6:
+ * - add max_readahead to fuse_init_in and fuse_init_out
+ *
+ * 7.7:
+ * - add FUSE_INTERRUPT message
+ * - add POSIX file lock support
+ *
+ * 7.8:
+ * - add lock_owner and flags fields to fuse_release_in
+ * - add FUSE_BMAP message
+ * - add FUSE_DESTROY message
+ *
* 7.9:
* - new fuse_getattr_in input argument of GETATTR
* - add lk_flags in fuse_lk_in
@@ -133,16 +170,14 @@
*
* 7.31
* - add FUSE_WRITE_KILL_PRIV flag
+ * - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING
+ * - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag
*/

#ifndef _LINUX_FUSE_H
#define _LINUX_FUSE_H

-#ifdef __KERNEL__
-#include <linux/types.h>
-#else
#include <stdint.h>
-#endif

/*
* Version negotiation:
@@ -274,6 +309,7 @@ struct fuse_file_lock {
* FUSE_CACHE_SYMLINKS: cache READLINK responses
* FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
* FUSE_EXPLICIT_INVAL_DATA: only invalidate cached pages on explicit request
+ * FUSE_MAP_ALIGNMENT: map_alignment field is valid
*/
#define FUSE_ASYNC_READ (1 << 0)
#define FUSE_POSIX_LOCKS (1 << 1)
@@ -301,6 +337,7 @@ struct fuse_file_lock {
#define FUSE_CACHE_SYMLINKS (1 << 23)
#define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
#define FUSE_EXPLICIT_INVAL_DATA (1 << 25)
+#define FUSE_MAP_ALIGNMENT (1 << 26)

/**
* CUSE INIT request/reply flags
@@ -422,9 +459,15 @@ enum fuse_opcode {
FUSE_RENAME2 = 45,
FUSE_LSEEK = 46,
FUSE_COPY_FILE_RANGE = 47,
+ FUSE_SETUPMAPPING = 48,
+ FUSE_REMOVEMAPPING = 49,

/* CUSE specific operations */
- CUSE_INIT = 4096
+ CUSE_INIT = 4096,
+
+ /* Reserved opcodes: helpful to detect structure endian-ness */
+ CUSE_INIT_BSWAP_RESERVED = 1048576, /* CUSE_INIT << 8 */
+ FUSE_INIT_BSWAP_RESERVED = 436207616, /* FUSE_INIT << 24 */
};

enum fuse_notify_code {
@@ -434,7 +477,7 @@ enum fuse_notify_code {
FUSE_NOTIFY_STORE = 4,
FUSE_NOTIFY_RETRIEVE = 5,
FUSE_NOTIFY_DELETE = 6,
- FUSE_NOTIFY_CODE_MAX
+ FUSE_NOTIFY_CODE_MAX,
};

/* The read buffer is required to be at least 8k, but may be much larger */
@@ -453,6 +496,11 @@ struct fuse_entry_out {
struct fuse_attr attr;
};

+struct fuse_entryver_out {
+ uint64_t version_index;
+ int64_t initial_version;
+};
+
struct fuse_forget_in {
uint64_t nlookup;
};
@@ -652,7 +700,7 @@ struct fuse_init_out {
uint32_t max_write;
uint32_t time_gran;
uint16_t max_pages;
- uint16_t padding;
+ uint16_t map_alignment;
uint32_t unused[8];
};

@@ -845,4 +893,30 @@ struct fuse_copy_file_range_in {
uint64_t flags;
};

+#define FUSE_SETUPMAPPING_FLAG_WRITE (1ull << 0)
+struct fuse_setupmapping_in {
+ /* An already open handle */
+ uint64_t fh;
+ /* Offset into the file to start the mapping */
+ uint64_t foffset;
+ /* Length of mapping required */
+ uint64_t len;
+ /* Flags, FUSE_SETUPMAPPING_FLAG_* */
+ uint64_t flags;
+ /* memory offset in to dax window */
+ uint64_t moffset;
+};
+
+struct fuse_removemapping_in {
+ /* number of fuse_removemapping_one follows */
+ uint32_t count;
+};
+
+struct fuse_removemapping_one {
+ /* Offset into the dax to start the unmapping */
+ uint64_t moffset;
+ /* Length of mapping required */
+ uint64_t len;
+};
+
#endif /* _LINUX_FUSE_H */
--
2.26.2

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-fs.cc | 2 +-
drivers/virtio-fs.hh | 7 +++++++
fs/virtiofs/virtiofs_vfsops.cc | 8 +++++++-
3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
index b7363040..ca9b00fc 100644
--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -103,7 +103,7 @@ bool fs::ack_irq()
}

fs::fs(virtio_device& virtio_dev)
- : virtio_driver(virtio_dev)
+ : virtio_driver(virtio_dev), _map_align(-1)
{
_driver_name = "virtio-fs";
_id = _instance++;
diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh
index d1c116de..fdbab5d0 100644
--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -44,6 +44,12 @@ public:
dax_window* get_dax() {
return (_dax.addr != mmio_nullptr) ? &_dax : nullptr;
}
+ // Set map alignment for DAX window. @map_align should be
+ // log2(byte_alignment), e.g. 12 for a 4096 byte alignment.
+ void set_map_alignment(int map_align) { _map_align = map_align; }
+ // Returns the map alignment for the DAX window as preiously set with
+ // set_map_alignment(), or < 0 if it has not been set.
+ int get_map_alignment() const { return _map_align; }

void req_done();
int64_t size();
@@ -63,6 +69,7 @@ private:
std::string _driver_name;
fs_config _config;
dax_window _dax;
+ int _map_align;

// maintains the virtio instance number for multiple drives
static int _instance;
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc
index 968f93fc..85cbf868 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc
@@ -12,6 +12,7 @@
#include <iostream>
#include "virtiofs.hh"
#include "virtiofs_i.hh"
+#include "drivers/virtio-fs.hh"

static std::atomic<uint64_t> fuse_unique_id(1);

@@ -85,7 +86,7 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,
in_args->major = FUSE_KERNEL_VERSION;
in_args->minor = FUSE_KERNEL_MINOR_VERSION;
in_args->max_readahead = PAGE_SIZE;
- in_args->flags = 0; // TODO: Verify that we need not set any flag
+ in_args->flags |= FUSE_MAP_ALIGNMENT;

auto* strategy = static_cast<fuse_strategy*>(device->private_data);
error = fuse_req_send_and_receive_reply(strategy, FUSE_INIT, FUSE_ROOT_ID,
@@ -99,6 +100,11 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,
virtiofs_debug("Initialized fuse filesystem with version major: %d, "
"minor: %d\n", out_args->major, out_args->minor);

+ if (out_args->flags & FUSE_MAP_ALIGNMENT) {
+ auto* drv = static_cast<virtio::fs*>(strategy->drv);
+ drv->set_map_alignment(out_args->map_alignment);
+ }
+
auto* root_node {new (std::nothrow) virtiofs_inode()};
if (!root_node) {
return ENOMEM;
--
2.26.2

[Fotis Xenakis]

When the DAX window is available from the device, the filesystem prefers
to use it instead of the regular FUSE_READ request. If that fails,
FUSE_READ is used as a fallback.

To use the DAX window, a part of the file is mapped to it with
FUSE_SETUPMAPPING, the contents are copied from it to the user buffers
and the mapping is cleaned-up with FUSE_REMOVEMAPPING. In this naive
implementation, the window is used for a single mapping at a time, with
no caching or readahead.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

fs/virtiofs/virtiofs_vnops.cc | 171 +++++++++++++++++++++++++++++-----
1 file changed, 148 insertions(+), 23 deletions(-)

diff --git a/fs/virtiofs/virtiofs_vnops.cc b/fs/virtiofs/virtiofs_vnops.cc
index 7fbb2cd2..dc75410f 100644
--- a/fs/virtiofs/virtiofs_vnops.cc
+++ b/fs/virtiofs/virtiofs_vnops.cc
@@ -23,9 +23,12 @@
#include <sys/types.h>
#include <osv/device.h>
#include <osv/sched.hh>
+#include <osv/mmio.hh>
+#include <osv/contiguous_alloc.hh>

#include "virtiofs.hh"
#include "virtiofs_i.hh"
+#include "drivers/virtio-fs.hh"

static constexpr uint32_t OPEN_FLAGS = O_RDONLY;

@@ -183,14 +186,142 @@ static int virtiofs_readlink(struct vnode* vnode, struct uio* uio)
return uiomove(link_path.get(), strlen(link_path.get()), uio);
}

+// Read @read_amt bytes from @inode, using the DAX window.
+static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,
+ u64 read_amt, fuse_strategy& strategy, struct uio& uio)
+{
+ auto* drv = static_cast<virtio::fs*>(strategy.drv);
+ auto* dax = drv->get_dax();
+ // Enter the critical path: setup mapping -> read -> remove mapping
+ std::lock_guard<mutex> guard {dax->lock};
+
+ // Setup mapping
+ // NOTE: There are restrictions on the arguments to FUSE_SETUPMAPPING, from
+ // the spec: "Alignment constraints for FUSE_SETUPMAPPING and
+ // FUSE_REMOVEMAPPING requests are communicated during FUSE_INIT
+ // negotiation"):
+ // - foffset: multiple of map_alignment from FUSE_INIT
+ // - len: not larger than remaining file?
+ // - moffset: multiple of map_alignment from FUSE_INIT
+ // In practice, map_alignment is the host's page size, because foffset and
+ // moffset are passed to mmap() on the host.
+ std::unique_ptr<fuse_setupmapping_in> in_args {
+ new (std::nothrow) fuse_setupmapping_in()};
+ if (!in_args) {
+ return ENOMEM;
+ }
+ in_args->fh = file_handle;
+ in_args->flags = 0;
+ uint64_t moffset = 0;
+ in_args->moffset = moffset;
+
+ auto map_align = drv->get_map_alignment();
+ if (map_align < 0) {
+ kprintf("[virtiofs] inode %lld, map alignment not set\n", inode.nodeid);
+ return ENOTSUP;
+ }
+ uint64_t alignment = 1ul << map_align;
+ auto foffset = align_down(static_cast<uint64_t>(uio.uio_offset), alignment);
+ in_args->foffset = foffset;
+
+ // The possible excess part of the file mapped due to alignment constraints
+ // NOTE: map_excess <= alignemnt
+ auto map_excess = uio.uio_offset - foffset;
+ if (moffset + map_excess >= dax->len) {
+ // No usable room in DAX window due to map_excess
+ return ENOBUFS;
+ }
+ // Actual read amount is read_amt, or what fits in the DAX window
+ auto read_amt_act = std::min<uint64_t>(read_amt,
+ dax->len - moffset - map_excess);
+ in_args->len = read_amt_act + map_excess;
+
+ virtiofs_debug("inode %lld, setting up mapping (foffset=%lld, len=%lld, "
+ "moffset=%lld)\n", inode.nodeid, in_args->foffset,
+ in_args->len, in_args->moffset);
+ auto error = fuse_req_send_and_receive_reply(&strategy, FUSE_SETUPMAPPING,
+ inode.nodeid, in_args.get(), sizeof(*in_args), nullptr, 0);
+ if (error) {
+ kprintf("[virtiofs] inode %lld, mapping setup failed\n", inode.nodeid);
+ return error;
+ }
+
+ // Read from the DAX window
+ // NOTE: It shouldn't be necessary to use the mmio* interface (i.e. volatile
+ // accesses). From the spec: "Drivers map this shared memory region with
+ // writeback caching as if it were regular RAM."
+ // The location of the requested data in the DAX window
+ auto req_data = dax->addr + moffset + map_excess;
+ error = uiomove(const_cast<void*>(req_data), read_amt_act, &uio);
+ if (error) {
+ kprintf("[virtiofs] inode %lld, uiomove failed\n", inode.nodeid);
+ return error;
+ }
+
+ // Remove mapping
+ // NOTE: This is only necessary when FUSE_SETUPMAPPING fails. From the spec:
+ // "If the device runs out of resources the FUSE_SETUPMAPPING request fails
+ // until resources are available again following FUSE_REMOVEMAPPING."
+ auto r_in_args_size = sizeof(fuse_removemapping_in) +
+ sizeof(fuse_removemapping_one);
+ std::unique_ptr<u8> r_in_args {new (std::nothrow) u8[r_in_args_size]};
+ if (!r_in_args) {
+ return ENOMEM;
+ }
+ auto r_in = new (r_in_args.get()) fuse_removemapping_in();
+ auto r_one = new (r_in_args.get() + sizeof(fuse_removemapping_in))
+ fuse_removemapping_one();
+ r_in->count = 1;
+ r_one->moffset = in_args->moffset;
+ r_one->len = in_args->len;
+
+ virtiofs_debug("inode %lld, removing mapping (moffset=%lld, len=%lld)\n",
+ inode.nodeid, r_one->moffset, r_one->len);
+ error = fuse_req_send_and_receive_reply(&strategy, FUSE_REMOVEMAPPING,
+ inode.nodeid, r_in_args.get(), r_in_args_size, nullptr, 0);
+ if (error) {
+ kprintf("[virtiofs] inode %lld, mapping removal failed\n",
+ inode.nodeid);
+ return error;
+ }
+
+ return 0;
+}
+
+// Read @read_amt bytes from @inode, using the fallback FUSE_READ mechanism.
+static int virtiofs_read_fallback(virtiofs_inode& inode, u64 file_handle,
+ u32 read_amt, u32 flags, fuse_strategy& strategy, struct uio& uio)
+{
+ std::unique_ptr<fuse_read_in> in_args {new (std::nothrow) fuse_read_in()};
+ std::unique_ptr<void, std::function<void(void*)>> buf {
+ memory::alloc_phys_contiguous_aligned(read_amt,
+ alignof(std::max_align_t)), memory::free_phys_contiguous_aligned };
+ if (!in_args | !buf) {
+ return ENOMEM;
+ }
+ in_args->fh = file_handle;
+ in_args->offset = uio.uio_offset;
+ in_args->size = read_amt;
+ in_args->flags = flags;
+
+ virtiofs_debug("inode %lld, reading %lld bytes at offset %lld\n",
+ inode.nodeid, read_amt, uio.uio_offset);
+ auto error = fuse_req_send_and_receive_reply(&strategy, FUSE_READ,
+ inode.nodeid, in_args.get(), sizeof(*in_args), buf.get(), read_amt);
+ if (error) {
+ kprintf("[virtiofs] inode %lld, read failed\n", inode.nodeid);
+ return error;
+ }
+
+ return uiomove(buf.get(), read_amt, &uio);
+}
+
// TODO: Optimize it to reduce number of exits to host (each
// fuse_req_send_and_receive_reply()) by reading eagerly "ahead/around" just
// like ROFS does and caching it
static int virtiofs_read(struct vnode* vnode, struct file* fp, struct uio* uio,
int ioflag)
{
- auto* inode = static_cast<virtiofs_inode*>(vnode->v_data);
-
// Can't read directories
if (vnode->v_type == VDIR) {
return EISDIR;
@@ -212,32 +343,26 @@ static int virtiofs_read(struct vnode* vnode, struct file* fp, struct uio* uio,
return 0;
}

+ auto* inode = static_cast<virtiofs_inode*>(vnode->v_data);
+ auto* file_data = static_cast<virtiofs_file_data*>(fp->f_data);
+ auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);
+
// Total read amount is what they requested, or what is left
auto read_amt = std::min<uint64_t>(uio->uio_resid,
inode->attr.size - uio->uio_offset);
- std::unique_ptr<u8[]> buf {new (std::nothrow) u8[read_amt]};
- std::unique_ptr<fuse_read_in> in_args {new (std::nothrow) fuse_read_in()};
- if (!buf || !in_args) {
- return ENOMEM;
- }
- auto* f_data = static_cast<virtiofs_file_data*>(file_data(fp));
- in_args->fh = f_data->file_handle;
- in_args->offset = uio->uio_offset;
- in_args->size = read_amt;
- in_args->flags = ioflag;

- virtiofs_debug("inode %lld, reading %lld bytes at offset %lld\n",
- inode->nodeid, read_amt, uio->uio_offset);

+ auto* drv = static_cast<virtio::fs*>(strategy->drv);

+ if (drv->get_dax()) {
+ // Try to read from DAX
+ if (!virtiofs_read_direct(*inode, file_data->file_handle, read_amt,
+ *strategy, *uio)) {

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);
- auto error = fuse_req_send_and_receive_reply(strategy, FUSE_READ,
- inode->nodeid, in_args.get(), sizeof(*in_args), buf.get(), read_amt);
- if (error) {
- kprintf("[virtiofs] inode %lld, read failed\n", inode->nodeid);
- return error;
+ return 0;
+ }
}
-
- return uiomove(buf.get(), read_amt, uio);
+ // DAX unavailable or failed, use fallback
+ return virtiofs_read_fallback(*inode, file_data->file_handle, read_amt,
+ ioflag, *strategy, *uio);
}

static int virtiofs_readdir(struct vnode* vnode, struct file* fp,
@@ -307,7 +432,7 @@ struct vnops virtiofs_vnops = {
virtiofs_truncate, /* truncate - returns error when called */
virtiofs_link, /* link - returns error when called */
virtiofs_arc, /* arc */ //TODO: Implement to allow memory re-use when
- // mapping files, investigate using virtio-fs DAX
+ // mapping files
virtiofs_fallocate, /* fallocate - returns error when called */
virtiofs_readlink, /* read link */
virtiofs_symlink /* symbolic link - returns error when called */
--
2.26.2

[Fotis Xenakis]

Since in virtio-fs the filesystem is very tightly coupled with the
driver, this tries to make clear the dependence of the first on the
second, as well as simplify.

This includes:
- The definition of fuse_request is moved from the fs to the driver,
since it is part of the interface it provides. Also, it is enhanced
with methods, somewhat promoting it to a "proper" class.
- fuse_strategy, as a redirection to the driver is removed and instead
the dependence on the driver is made explicit.
- Last, virtio::fs::fs_req is removed and fuse_request is used in its
place, since it offered no value with fuse_request now defined in the
driver.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-fs.cc | 42 +++++++++++++---------------------
drivers/virtio-fs.hh | 27 +++++++++++++++-------
fs/virtiofs/virtiofs_i.hh | 24 ++-----------------
fs/virtiofs/virtiofs_vfsops.cc | 17 +++++++-------
fs/virtiofs/virtiofs_vnops.cc | 39 +++++++++++++++----------------
5 files changed, 64 insertions(+), 85 deletions(-)

diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
index ca9b00fc..e0e090bc 100644
--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -28,25 +28,23 @@

using namespace memory;

-void fuse_req_wait(fuse_request* req)
-{
- WITH_LOCK(req->req_mutex) {
- req->req_wait.wait(req->req_mutex);
- }
-}
+using fuse_request = virtio::fs::fuse_request;

namespace virtio {

-static int fuse_make_request(void* driver, fuse_request* req)
+// Wait for the request to be marked as completed.
+void fs::fuse_request::wait()
{
- auto fs_driver = static_cast<fs*>(driver);
- return fs_driver->make_request(req);
+ WITH_LOCK(req_mutex) {
+ req_wait.wait(req_mutex);
+ }
}

-static void fuse_req_done(fuse_request* req)
+// Mark the request as completed.
+void fs::fuse_request::done()
{
- WITH_LOCK(req->req_mutex) {
- req->req_wait.wake_one(req->req_mutex);
+ WITH_LOCK(req_mutex) {
+ req_wait.wake_one(req_mutex);
}
}

@@ -87,7 +85,7 @@ static struct devops fs_devops {
struct driver fs_driver = {
"virtio_fs",
&fs_devops,
- sizeof(struct fuse_strategy),
+ sizeof(fs*),
};

bool fs::ack_irq()
@@ -161,10 +159,7 @@ fs::fs(virtio_device& virtio_dev)
dev_name += std::to_string(_disk_idx++);

struct device* dev = device_create(&fs_driver, dev_name.c_str(), D_BLK); // TODO Should it be really D_BLK?
- auto* strategy = static_cast<fuse_strategy*>(dev->private_data);
- strategy->drv = this;
- strategy->make_request = fuse_make_request;
-
+ dev->private_data = this;
debugf("virtio-fs: Add device instance %d as [%s]\n", _id,
dev_name.c_str());
}
@@ -201,13 +196,12 @@ void fs::req_done()
while (true) {
virtio_driver::wait_for_queue(queue, &vring::used_ring_not_empty);

- fs_req* req;
+ fuse_request* req;
u32 len;
- while ((req = static_cast<fs_req*>(queue->get_buf_elem(&len))) !=
+ while ((req = static_cast<fuse_request*>(queue->get_buf_elem(&len))) !=
nullptr) {

- fuse_req_done(req->fuse_req);
- delete req;
+ req->done();
queue->get_buf_finalize();
}

@@ -231,11 +225,7 @@ int fs::make_request(fuse_request* req)
fuse_req_enqueue_input(queue, req);
fuse_req_enqueue_output(queue, req);

- auto* fs_request = new (std::nothrow) fs_req(req);
- if (!fs_request) {
- return ENOMEM;
- }
- queue->add_buf_wait(fs_request);
+ queue->add_buf_wait(req);
queue->kick();

return 0;
diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh
index fdbab5d0..d78d7962 100644
--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -12,7 +12,7 @@
#include <osv/waitqueue.hh>
#include "drivers/virtio.hh"
#include "drivers/virtio-device.hh"
-#include "fs/virtiofs/virtiofs_i.hh"
+#include "fs/virtiofs/fuse_kernel.h"

namespace virtio {

@@ -23,6 +23,24 @@ enum {

class fs : public virtio_driver {
public:
+ struct fuse_request {
+ struct fuse_in_header in_header;
+ struct fuse_out_header out_header;
+
+ void* input_args_data;
+ size_t input_args_size;
+
+ void* output_args_data;
+ size_t output_args_size;
+
+ void wait();
+ void done();
+
+ private:
+ mutex_t req_mutex;
+ waitqueue req_wait;
+ };
+
struct fs_config {
char tag[36];
u32 num_queues;
@@ -59,13 +77,6 @@ public:
static hw_driver* probe(hw_device* dev);

private:
- struct fs_req {
- fs_req(fuse_request* f) : fuse_req(f) {};
- ~fs_req() {};
-
- fuse_request* fuse_req;
- };
-

std::string _driver_name;
fs_config _config;
dax_window _dax;

diff --git a/fs/virtiofs/virtiofs_i.hh b/fs/virtiofs/virtiofs_i.hh
index 17fbcd36..76533d74 100644
--- a/fs/virtiofs/virtiofs_i.hh
+++ b/fs/virtiofs/virtiofs_i.hh
@@ -11,30 +11,10 @@
#include "fuse_kernel.h"
#include <osv/mutex.h>
#include <osv/waitqueue.hh>
+#include "drivers/virtio-fs.hh"

-struct fuse_request {
- struct fuse_in_header in_header;
- struct fuse_out_header out_header;
-
- void* input_args_data;
- size_t input_args_size;
-
- void* output_args_data;
- size_t output_args_size;
-
- mutex_t req_mutex;
- waitqueue req_wait;
-};
-
-struct fuse_strategy {
- void* drv;
- int (*make_request)(void*, fuse_request*);
-};
-
-int fuse_req_send_and_receive_reply(fuse_strategy* strategy, uint32_t opcode,
+int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,
uint64_t nodeid, void* input_args_data, size_t input_args_size,
void* output_args_data, size_t output_args_size);

-void fuse_req_wait(fuse_request* req);
-
#endif
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc
index 85cbf868..30daa42b 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc
@@ -14,9 +14,11 @@
#include "virtiofs_i.hh"
#include "drivers/virtio-fs.hh"

+using fuse_request = virtio::fs::fuse_request;
+

static std::atomic<uint64_t> fuse_unique_id(1);

-int fuse_req_send_and_receive_reply(fuse_strategy* strategy, uint32_t opcode,
+int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode,
uint64_t nodeid, void* input_args_data, size_t input_args_size,
void* output_args_data, size_t output_args_size)
{
@@ -36,9 +38,9 @@ int fuse_req_send_and_receive_reply(fuse_strategy* strategy, uint32_t opcode,
req->output_args_data = output_args_data;
req->output_args_size = output_args_size;

- assert(strategy->drv);
- strategy->make_request(strategy->drv, req.get());
- fuse_req_wait(req.get());
+ assert(drv);
+ drv->make_request(req.get());
+ req->wait();

int error = -req->out_header.error;

@@ -88,8 +90,8 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,
in_args->max_readahead = PAGE_SIZE;
in_args->flags |= FUSE_MAP_ALIGNMENT;

- auto* strategy = static_cast<fuse_strategy*>(device->private_data);
- error = fuse_req_send_and_receive_reply(strategy, FUSE_INIT, FUSE_ROOT_ID,
+ auto* drv = static_cast<virtio::fs*>(device->private_data);
+ error = fuse_req_send_and_receive_reply(drv, FUSE_INIT, FUSE_ROOT_ID,
in_args.get(), sizeof(*in_args), out_args.get(), sizeof(*out_args));
if (error) {
kprintf("[virtiofs] Failed to initialize fuse filesystem!\n");
@@ -101,7 +103,6 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,

"minor: %d\n", out_args->major, out_args->minor);

if (out_args->flags & FUSE_MAP_ALIGNMENT) {

- auto* drv = static_cast<virtio::fs*>(strategy->drv);
drv->set_map_alignment(out_args->map_alignment);
}

@@ -114,7 +115,7 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,

virtiofs_set_vnode(mp->m_root->d_vnode, root_node);

- mp->m_data = strategy;
+ mp->m_data = drv;
mp->m_dev = device;

return 0;
diff --git a/fs/virtiofs/virtiofs_vnops.cc b/fs/virtiofs/virtiofs_vnops.cc
index dc75410f..38c000ec 100644
--- a/fs/virtiofs/virtiofs_vnops.cc
+++ b/fs/virtiofs/virtiofs_vnops.cc
@@ -28,7 +28,6 @@

#include "virtiofs.hh"
#include "virtiofs_i.hh"
-#include "drivers/virtio-fs.hh"

static constexpr uint32_t OPEN_FLAGS = O_RDONLY;

@@ -60,8 +59,8 @@ static int virtiofs_lookup(struct vnode* vnode, char* name, struct vnode** vpp)
}
strcpy(in_args.get(), name);

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);

- auto error = fuse_req_send_and_receive_reply(strategy, FUSE_LOOKUP,
+ auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data);
+ auto error = fuse_req_send_and_receive_reply(drv, FUSE_LOOKUP,
inode->nodeid, in_args.get(), in_args_len, out_args.get(),
sizeof(*out_args));
if (error) {
@@ -111,8 +110,8 @@ static int virtiofs_open(struct file* fp)
}
in_args->flags = OPEN_FLAGS;

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);

- auto error = fuse_req_send_and_receive_reply(strategy, FUSE_OPEN,
+ auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data);
+ auto error = fuse_req_send_and_receive_reply(drv, FUSE_OPEN,
inode->nodeid, in_args.get(), sizeof(*in_args), out_args.get(),
sizeof(*out_args));
if (error) {
@@ -146,8 +145,8 @@ static int virtiofs_close(struct vnode* vnode, struct file* fp)

in_args->fh = f_data->file_handle;

in_args->flags = OPEN_FLAGS; // need to be same as in FUSE_OPEN

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);

- auto error = fuse_req_send_and_receive_reply(strategy, FUSE_RELEASE,
+ auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data);
+ auto error = fuse_req_send_and_receive_reply(drv, FUSE_RELEASE,
inode->nodeid, in_args.get(), sizeof(*in_args), nullptr, 0);
if (error) {
kprintf("[virtiofs] inode %lld, close failed\n", inode->nodeid);
@@ -173,8 +172,8 @@ static int virtiofs_readlink(struct vnode* vnode, struct uio* uio)
return ENOMEM;

}

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);

- auto error = fuse_req_send_and_receive_reply(strategy, FUSE_READLINK,
+ auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data);
+ auto error = fuse_req_send_and_receive_reply(drv, FUSE_READLINK,
inode->nodeid, nullptr, 0, link_path.get(), PATH_MAX);
if (error) {
kprintf("[virtiofs] inode %lld, readlink failed\n", inode->nodeid);
@@ -188,10 +187,9 @@ static int virtiofs_readlink(struct vnode* vnode, struct uio* uio)

// Read @read_amt bytes from @inode, using the DAX window.

static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,

- u64 read_amt, fuse_strategy& strategy, struct uio& uio)
+ u64 read_amt, virtio::fs& drv, struct uio& uio)
{
- auto* drv = static_cast<virtio::fs*>(strategy.drv);
- auto* dax = drv->get_dax();
+ auto* dax = drv.get_dax();

// Enter the critical path: setup mapping -> read -> remove mapping

std::lock_guard<mutex> guard {dax->lock};

@@ -215,7 +213,7 @@ static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,
uint64_t moffset = 0;
in_args->moffset = moffset;

- auto map_align = drv->get_map_alignment();
+ auto map_align = drv.get_map_alignment();
if (map_align < 0) {

kprintf("[virtiofs] inode %lld, map alignment not set\n", inode.nodeid);

return ENOTSUP;
@@ -239,7 +237,7 @@ static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,

virtiofs_debug("inode %lld, setting up mapping (foffset=%lld, len=%lld, "

"moffset=%lld)\n", inode.nodeid, in_args->foffset,

in_args->len, in_args->moffset);
- auto error = fuse_req_send_and_receive_reply(&strategy, FUSE_SETUPMAPPING,
+ auto error = fuse_req_send_and_receive_reply(&drv, FUSE_SETUPMAPPING,

inode.nodeid, in_args.get(), sizeof(*in_args), nullptr, 0);

if (error) {

kprintf("[virtiofs] inode %lld, mapping setup failed\n", inode.nodeid);

@@ -277,7 +275,7 @@ static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,

virtiofs_debug("inode %lld, removing mapping (moffset=%lld, len=%lld)\n",

inode.nodeid, r_one->moffset, r_one->len);

- error = fuse_req_send_and_receive_reply(&strategy, FUSE_REMOVEMAPPING,
+ error = fuse_req_send_and_receive_reply(&drv, FUSE_REMOVEMAPPING,

inode.nodeid, r_in_args.get(), r_in_args_size, nullptr, 0);

if (error) {

kprintf("[virtiofs] inode %lld, mapping removal failed\n",

@@ -290,7 +288,7 @@ static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle,

// Read @read_amt bytes from @inode, using the fallback FUSE_READ mechanism.

static int virtiofs_read_fallback(virtiofs_inode& inode, u64 file_handle,

- u32 read_amt, u32 flags, fuse_strategy& strategy, struct uio& uio)
+ u32 read_amt, u32 flags, virtio::fs& drv, struct uio& uio)
{

std::unique_ptr<fuse_read_in> in_args {new (std::nothrow) fuse_read_in()};

std::unique_ptr<void, std::function<void(void*)>> buf {

@@ -306,7 +304,7 @@ static int virtiofs_read_fallback(virtiofs_inode& inode, u64 file_handle,

virtiofs_debug("inode %lld, reading %lld bytes at offset %lld\n",

inode.nodeid, read_amt, uio.uio_offset);
- auto error = fuse_req_send_and_receive_reply(&strategy, FUSE_READ,
+ auto error = fuse_req_send_and_receive_reply(&drv, FUSE_READ,

inode.nodeid, in_args.get(), sizeof(*in_args), buf.get(), read_amt);

if (error) {

kprintf("[virtiofs] inode %lld, read failed\n", inode.nodeid);

@@ -345,24 +343,23 @@ static int virtiofs_read(struct vnode* vnode, struct file* fp, struct uio* uio,

auto* inode = static_cast<virtiofs_inode*>(vnode->v_data);

auto* file_data = static_cast<virtiofs_file_data*>(fp->f_data);

- auto* strategy = static_cast<fuse_strategy*>(vnode->v_mount->m_data);

+ auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data);

// Total read amount is what they requested, or what is left
auto read_amt = std::min<uint64_t>(uio->uio_resid,
inode->attr.size - uio->uio_offset);

- auto* drv = static_cast<virtio::fs*>(strategy->drv);
if (drv->get_dax()) {

// Try to read from DAX

if (!virtiofs_read_direct(*inode, file_data->file_handle, read_amt,

- *strategy, *uio)) {
+ *drv, *uio)) {

return 0;

}
}
// DAX unavailable or failed, use fallback

return virtiofs_read_fallback(*inode, file_data->file_handle, read_amt,

- ioflag, *strategy, *uio);
+ ioflag, *drv, *uio);

}

static int virtiofs_readdir(struct vnode* vnode, struct file* fp,

--
2.26.2

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: update fuse protocol header

Copy from https://gitlab.com/virtio-fs/qemu
@21336c0f3d05a97f5c409bbc894c19d87259655c.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>

Message-Id: <VI1PR03MB43838F0F34...@VI1PR03MB4383.eurprd03.prod.outlook.com>

---
diff --git a/fs/virtiofs/fuse_kernel.h b/fs/virtiofs/fuse_kernel.h

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: implement FUSE_INIT map_alignment field

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <VI1PR03MB4383AE2F61...@VI1PR03MB4383.eurprd03.prod.outlook.com>

---
diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc

--- a/drivers/virtio-fs.cc
+++ b/drivers/virtio-fs.cc
@@ -103,7 +103,7 @@ bool fs::ack_irq()
}

fs::fs(virtio_device& virtio_dev)
- : virtio_driver(virtio_dev)
+ : virtio_driver(virtio_dev), _map_align(-1)
{
_driver_name = "virtio-fs";
_id = _instance++;
diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh

--- a/drivers/virtio-fs.hh
+++ b/drivers/virtio-fs.hh
@@ -44,6 +44,12 @@ public:
dax_window* get_dax() {
return (_dax.addr != mmio_nullptr) ? &_dax : nullptr;
}
+ // Set map alignment for DAX window. @map_align should be
+ // log2(byte_alignment), e.g. 12 for a 4096 byte alignment.
+ void set_map_alignment(int map_align) { _map_align = map_align; }
+ // Returns the map alignment for the DAX window as preiously set with
+ // set_map_alignment(), or < 0 if it has not been set.
+ int get_map_alignment() const { return _map_align; }

void req_done();
int64_t size();
@@ -63,6 +69,7 @@ private:
std::string _driver_name;
fs_config _config;
dax_window _dax;
+ int _map_align;

// maintains the virtio instance number for multiple drives
static int _instance;
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc

[Waldek Kozaczuk]

Hi,

I think both 3 and 4 parts of your patches look good. But I guess it would not hurt if Nadav could scrutinize at them from C++ perspective as well.

However, I am having a bit of trouble testing those. I do not think anything is wrong with your patches but possibly something has changed on QEMU side virtiofs implementation or something else.

I am not Ubuntu 20.04 and using your QEMU branch - fix-dax-fd,

Whenever I run virtiofsd and QEMU and I get the error:

./virtiofsd --socket-path=/tmp/vhostqemu -o source=/home/wkozaczuk/projects/osv/apps/native-example -o cache=always -d

[2240367089675] [ID: 00018845] virtio_session_mount: Waiting for vhost-user socket connection...

[2249948371337] [ID: 00018845] virtio_session_mount: Received vhost-user socket connection

[2249949209337] [ID: 00000001] tmpdir(virtiofsd-nuZoui): Permission denied

/home/wkozaczuk/projects/qemu/build/x86_64-softmmu/qemu-system-x86_64 \

-m 2G \

-smp 4 \

-vnc :1 \

-gdb tcp::1234,server,nowait \

-kernel /home/wkozaczuk/projects/osv/build/last/kernel.elf \

-append "--mount-fs=virtiofs,/dev/virtiofs1,/tmp/virtiofs /tmp/virtiofs/hello" \

-device virtio-blk-pci,id=blk0,drive=hd0,scsi=off \

-drive file=/home/wkozaczuk/projects/osv/build/last/usr.img,if=none,id=hd0,cache=none,aio=native \

-chardev socket,id=char0,path=/tmp/vhostqemu \

-device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \

-object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm,share=on \

-numa node,memdev=mem \

-netdev user,id=un0,net=192.168.122.0/24,host=192.168.122.1 \

-device virtio-net-pci,netdev=un0 \

-device virtio-rng-pci \

-enable-kvm \

-cpu host,+x2apic \

-chardev stdio,mux=on,id=stdio,signal=off \

-mon chardev=stdio,mode=readline \

-device isa-serial,chardev=stdio

qemu-system-x86_64: -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs: Failed to read msg header. Read 0 instead of 12. Original request 1.

qemu-system-x86_64: -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs: vhost_dev_init failed: Operation not permitted

I know virtiofs is a moving target so I must be missing some settings, it looks like a permission error. Any ideas?

Regards,

Waldek

[Fotis Xenakis]

Truth is I am getting the exact same errors and I haven't yet wrapped my head around the security / sandboxing functionality of virtiofsd, so unfortunately no ideas on this (apart from that it seems to originate in virtiofsd, and what fails is the mkdtemp call here).

Not porud to say this, but I have been running with elevated privileges locally. I will reach out to the virtio-fs devs to ask though and hopefully come back with a proper answer!

Thank you for pointing this out

[Fotis Xenakis]

Update: the answer from the devs is that it's preferred to run virtiofsd with elevated privileges indeed, as it's supposed to drop all excess privileges at runtime.

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: add basic read using the DAX window

When the DAX window is available from the device, the filesystem prefers
to use it instead of the regular FUSE_READ request. If that fails,
FUSE_READ is used as a fallback.

To use the DAX window, a part of the file is mapped to it with
FUSE_SETUPMAPPING, the contents are copied from it to the user buffers
and the mapping is cleaned-up with FUSE_REMOVEMAPPING. In this naive
implementation, the window is used for a single mapping at a time, with
no caching or readahead.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>

Message-Id: <VI1PR03MB438337071D...@VI1PR03MB4383.eurprd03.prod.outlook.com>

---
diff --git a/fs/virtiofs/virtiofs_vnops.cc b/fs/virtiofs/virtiofs_vnops.cc

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: refactor driver / fs

Since in virtio-fs the filesystem is very tightly coupled with the
driver, this tries to make clear the dependence of the first on the
second, as well as simplify.

This includes:
- The definition of fuse_request is moved from the fs to the driver,
since it is part of the interface it provides. Also, it is enhanced
with methods, somewhat promoting it to a "proper" class.
- fuse_strategy, as a redirection to the driver is removed and instead
the dependence on the driver is made explicit.
- Last, virtio::fs::fs_req is removed and fuse_request is used in its
place, since it offered no value with fuse_request now defined in the
driver.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>

Message-Id: <VI1PR03MB4383C1D8E6...@VI1PR03MB4383.eurprd03.prod.outlook.com>

---
diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc

[Waldek Kozaczuk]

All works fine and looks good. So I have committed your patches. Thanks.

It is sad that to test it I had to run both qemu and virtiofsd as root! I think I did not have to do it with the previous unreleased versions of qemu. I hope the QEMU team will work out something better.

Waldek