Τη Τετάρτη, 29 Απριλίου 2020 - 9:21:13 μ.μ. UTC+3, ο χρήστης Waldek
Kozaczuk έγραψε:
>
> I think your patch looks good and I like your simplifications.
>
> Couple of things to make sure we have covered all bases.
>
> 1) Are we sure none of these changes break any thread-safety?
>
I had checked for this, both trying to reason about the code and testing
with a program with concurrent read()ers, but by chance of the second
version, I shall check again, paying more attention to these changes.
> 2) Are we certain we do not need to use "*free_phys_contiguous_aligned*"
> in some places to make sure the host sees contiguous physical memory?
> Currently, we use new in all virtiofs related code which uses regular
> malloc behind the scenes.
>
This is actually a valid point I hadn't given much thought into. I will
look into it, thank you!
>
> On Monday, April 20, 2020 at 5:07:18 PM 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.
>>
> Agree.
>
>>
>> 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.
>>
> I like this.
>
>> - 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 | 16 +++++++------
>> fs/virtiofs/virtiofs_vnops.cc | 37 ++++++++++++++----------------
>> 5 files changed, 63 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc
>> index b7363040..af1246c1 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 d1c116de..f35fd710 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;
>> @@ -53,13 +71,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 968f93fc..ee5725e4 100644
>> --- a/fs/virtiofs/virtiofs_vfsops.cc
>> +++ b/fs/virtiofs/virtiofs_vfsops.cc
>> @@ -13,9 +13,11 @@
>> #include "virtiofs.hh"
>> #include "virtiofs_i.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)
>> {
>> @@ -35,9 +37,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;
>>
>> @@ -87,8 +89,8 @@ static int virtiofs_mount(struct mount* mp, const char*
>> dev, int flags,
>> in_args->max_readahead = PAGE_SIZE;
>> in_args->flags = 0; // TODO: Verify that we need not set any flag
>>
>> - 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");
>> @@ -108,7 +110,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 9551ff07..6779eb93 100644
>> --- a/fs/virtiofs/virtiofs_vnops.cc
>> +++ b/fs/virtiofs/virtiofs_vnops.cc
>> @@ -27,7 +27,6 @@
>>
>> #include "virtiofs.hh"
>> #include "virtiofs_i.hh"
>> -#include "drivers/virtio-fs.hh"
>>
>> static constexpr uint32_t OPEN_FLAGS = O_RDONLY;
>>
>> @@ -59,8 +58,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) {
>> @@ -110,8 +109,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) {
>> @@ -145,8 +144,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);
>> @@ -172,8 +171,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);
>> @@ -187,10 +186,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};
>>
>> @@ -240,7 +238,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), out_args.get(),
>> sizeof(*out_args));
>> if (error) {
>> @@ -275,7 +273,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, iargs->moffset, iargs->len);
>> - error = fuse_req_send_and_receive_reply(&strategy,
>> FUSE_REMOVEMAPPING,
>> + error = fuse_req_send_and_receive_reply(&drv, FUSE_REMOVEMAPPING,
>> inode.nodeid, iargs.get(), sizeof(*iargs), nullptr, 0);
>> if (error) {
>> kprintf("[virtiofs] inode %lld, mapping removal failed\n",
>> @@ -288,7 +286,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<u8[]> buf {new (std::nothrow) u8[read_amt]};
>> @@ -302,7 +300,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);
>> @@ -341,24 +339,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.1
>>
>>
--
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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/bf269ce7-dd05-46da-ae1a-f04128c7ccea%40googlegroups.com.
