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 On Sunday, May 17, 2020 at 5:52:01 AM UTC-4, Fotis Xenakis wrote: > > 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] <javascript:>> > --- > 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 > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/f1e5225f-56dc-4006-82a3-575fef14a0d4%40googlegroups.com.
