> > 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? > 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 <https://gitlab.com/virtio-fs/qemu/-/blob/21336c0f3d05a97f5c409bbc894c19d87259655c/tools/virtiofsd/passthrough_ll.c#L2905> ). 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
> > 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]> >> --- >> 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/a8a58573-bded-4c24-b3d3-64dbcbabe59f%40googlegroups.com.
