[osv-dev] Re: [PATCH 6/6] virtio-fs: refactor driver / fs
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?
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.
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 >
> ---
> 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(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(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(queue->get_buf_elem(&len))) !=
> +while ((req =
> static_cast(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
> #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_i
[osv-dev] Re: [PATCH 3/6] virtio-fs: update fuse protocol header
On Monday, April 20, 2020 at 5:04:27 PM UTC-4, Fotis Xenakis wrote:
>
> Copy from virtiofsd @ 32006c66f2578af4121d7effaccae4aa4fa12e46. This
> includes the definitions for FUSE_SETUPMAPPING AND FUSE_REMOVEMAPPING.
>
> Signed-off-by: Fotis Xenakis >
> ---
> fs/virtiofs/fuse_kernel.h | 82 ++-
> 1 file changed, 38 insertions(+), 44 deletions(-)
>
> diff --git a/fs/virtiofs/fuse_kernel.h b/fs/virtiofs/fuse_kernel.h
> index 018a00a2..ce46046a 100644
> --- a/fs/virtiofs/fuse_kernel.h
> +++ b/fs/virtiofs/fuse_kernel.h
> @@ -44,7 +44,6 @@
> * - add lock_owner field to fuse_setattr_in, fuse_read_in and
> fuse_write_in
> * - add blksize field to fuse_attr
> * - add file flags field to fuse_read_in and fuse_write_in
> - * - Add ATIME_NOW and MTIME_NOW flags to fuse_setattr_in
> *
> * 7.10
> * - add nonseekable open flag
> @@ -55,7 +54,7 @@
> * - add POLL message and NOTIFY_POLL notification
> *
> * 7.12
> - * - add umask flag to input argument of create, mknod and mkdir
> + * - add umask flag to input argument of open, mknod and mkdir
> * - add notification messages for invalidation of inodes and
> *directory entries
> *
> @@ -120,19 +119,6 @@
> *
> * 7.28
> * - add FUSE_COPY_FILE_RANGE
> - * - add FOPEN_CACHE_DIR
> - * - add FUSE_MAX_PAGES, add max_pages to init_out
> - * - add FUSE_CACHE_SYMLINKS
> - *
> - * 7.29
> - * - add FUSE_NO_OPENDIR_SUPPORT flag
> - *
> - * 7.30
> - * - add FUSE_EXPLICIT_INVAL_DATA
> - * - add FUSE_IOCTL_COMPAT_X32
> - *
> - * 7.31
> - * - add FUSE_WRITE_KILL_PRIV flag
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -168,7 +154,7 @@
> #define FUSE_KERNEL_VERSION 7
>
> /** Minor version number of this interface */
> -#define FUSE_KERNEL_MINOR_VERSION 31
> +#define FUSE_KERNEL_MINOR_VERSION 27
>
I have applied this patch but when I started testing your later patches
that enable DAX logic I would get error messages about the wrong protocol
version:
OSv v0.54.0-179-g2f92fc91
4 CPUs detected
Firmware vendor: SeaBIOS
bsd: initializing - done
VFS: mounting ramfs at /
VFS: mounting devfs at /dev
net: initializing - done
vga: Add VGA device instance
eth0: ethernet address: 52:54:00:12:34:56
virtio-blk: Add blk device instances 0 as vblk0, devsize=6470656
random: virtio-rng registered as a source.
virtio-fs: Detected device with tag: [myfs] and num_queues: 1
virtio-fs: Detected DAX window with length 67108864
virtio-fs: Add device instance 0 as [virtiofs1]
random: intel drng, rdrand registered as a source.
random: initialized
VFS: unmounting /dev
VFS: mounting rofs at /rofs
VFS: mounting devfs at /dev
VFS: mounting procfs at /proc
VFS: mounting sysfs at /sys
VFS: mounting ramfs at /tmp
VFS: mounting virtiofs at /virtiofs
[virtiofs] Failed to initialize fuse filesystem!
failed to mount virtiofs, error = Protocol error
[I/43 dhcp]: Broadcasting DHCPDISCOVER message with xid: [1603537588]
[I/43 dhcp]: Waiting for IP...
[I/55 dhcp]: Received DHCPOFFER message from DHCP server: 192.168.122.1
regarding offerred IP address: 192.168.122.15
[I/55 dhcp]: Broadcasting DHCPREQUEST message with xid: [1603537588] to
SELECT offered IP: 192.168.122.15
[I/55 dhcp]: Received DHCPACK message from DHCP server: 192.168.122.1
regarding offerred IP address: 192.168.122.15
[I/55 dhcp]: Server acknowledged IP 192.168.122.15 for interface eth0 with
time to lease in seconds: 86400
eth0: 192.168.122.15
[I/55 dhcp]: Configuring eth0: ip 192.168.122.15 subnet mask 255.255.255.0
gateway 192.168.122.1 MTU 1500
Booted up in 140.48 ms
Cmdline: /virtiofs/hello
Failed to load object: /virtiofs/hello. Powering off.
# and from virtiofsd [7426562093843] [ID: 0008] INIT: 7.27
[7426562097664] [ID: 0008] flags=0x
[7426562100498] [ID: 0008] max_readahead=0x1000
[7426562104503] [ID: 0008] fuse: unsupported protocol version: 7.27
[7426562119457] [ID: 0008]unique: 1, error: -71 (Protocol error),
outsize: 16
[7426562125006] [ID: 0008] virtio_send_msg: elem 0: with 2 in desc of
length 80
[7426577096593] [ID: 0001] virtio_loop: Got VU event
This happens when I use stock QEMU 5.0 (just released a couple of days ago,
which seems to have not DAX support yet) and qemu version from
https://gitlab.com/virtio-fs/qemu/-/commits/virtio-dev (see virtio-dev
branch).
I had to bump the version to 31 and then it works. Could you please
investigate?
Waldek
>
> /** The node ID of the root inode */
> #define FUSE_ROOT_ID 1
> @@ -236,14 +222,10 @@ struct fuse_file_lock {
> * FOPEN_DIRECT_IO: bypass page cache for this open file
> * FOPEN_KEEP_CACHE: don't invalidate the data cache on open
> * FOPEN_NONSEEKABLE: the file is not seekable
> - * FOPEN_CACHE_DIR: allow caching this directory
> - * FOPEN_STREAM: the file is stream-like (no file position at all)
> */
> #define FOPEN_DIRECT_IO(1 << 0)
> #define FOPEN_KEEP_CACHE
