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 <[email protected]>
Message-Id: <VI1PR03MB4383C1D8E60219C273114E4AA6BB0@VI1PR03MB4383.eurprd03.prod.outlook.com>

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;

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;

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

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;

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,