Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dragonball: introduce vhost-user-fs device #8429

Merged
merged 3 commits into from Dec 8, 2023
Merged

dragonball: introduce vhost-user-fs device #8429

merged 3 commits into from Dec 8, 2023

Conversation

adamqqqplay
Copy link
Contributor

@adamqqqplay adamqqqplay commented Nov 13, 2023 •
edited

dragonball: support vhost-user-fs

This patch implements the virtio-fs device used for filesystem sharing
and heavily based on the vhost-user protocol.

This vhost-user-fs device defines 5 parameters:
  - path: vhost-user socket path
  - tag: mount tag used from the guest to mount the filesystem
  - req_num_queues: number of request virtqueues
  - queue_size: depth of each virtqueue
  - cache_size: cache window size for dax

This device needs to be defined before the VM instance is started,
which can be done through the dbs-cli tool with --fs option:
--fs '{
    "sock_path":"/path/to/virtiofs.socket",
    "tag":"myfs",
    "num_queues":1,
    "queue_size":1024,
    "cache_size":0,
    "thread_pool_size":1,
    "cache_policy":"auto",
    "writeback_cache":true,
    "no_open":true,
    "xattr":true,
    "drop_sys_resource":false,
    "mode":"vhostuser",
    "fuse_killpriv_v2":true,
    "no_readdir":false,
}'

Fixes: #8428

@katacontainersbot katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Nov 13, 2023
@katacontainersbot
Copy link
Contributor

Can one of the admins verify this patch?

jiangliu
jiangliu reviewed Nov 13, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/src/device.rs Outdated
pub fn get_vring_notifier(&self) -> Vec<&EventFd> {
self.queues
.iter()
.map(|x| x.notifier.notifier().unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please help to add comments about why it's safe to call unwrap() here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we return Result<Vec<&EventFd>> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is duplicated with get_queue_interrupt_eventfds, so I removed it.

jiangliu
jiangliu reviewed Nov 13, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/Cargo.toml Outdated
@@ -32,6 +32,7 @@ serde = "1.0.27"
serde_json = "1.0.9"
thiserror = "1"
threadpool = "1"
vm-device = "0.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why using vm-device instead of dbs-device?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this was a misuse, I fixed it.

@adamqqqplay adamqqqplay force-pushed the support-vhost-user-fs branch 2 times, most recently from 2028d90 to e9211da Compare November 15, 2023 04:07
@studychao
Copy link
Member

/test

@adamqqqplay adamqqqplay force-pushed the support-vhost-user-fs branch 2 times, most recently from 27c7d83 to 4520a05 Compare November 22, 2023 03:52
jiangliu
jiangliu reviewed Nov 29, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/Cargo.toml Outdated
vhost-user-fs = ["vhost"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to define:

vhost-user = ["vhost"]
vhost-user-fs = ["vhost-user"]

Then it's flexible to support vhost-user-blk etc in future.

justxuewei
justxuewei reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks @adamqqqplay! A few comments here.

src/dragonball/src/dbs_virtio_devices/src/vhost/mod.rs Outdated
@@ -10,7 +10,7 @@ pub mod vhost_kern;
pub use vhost_rs::vhost_user::Error as VhostUserError;
pub use vhost_rs::Error as VhostError;

#[cfg(feature = "vhost-user")]
#[cfg(feature = "vhost-user-fs")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change it to vhost-user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let me improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/fs.rs
Comment on lines +363 to +366
MASTER_SLOT => {
// If virtiofsd crashes, vmm will exit too.
error!("{}: Master-slave disconnected, exiting...", self.id);
// TODO: how to make dragonball crash here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action of exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently no good way to fix it.

@mofishzz @adamqqqplay
dragonball: support vhost-user-fs
d6cfbe9 
This patch implements the virtio-fs device used for filesystem sharing
and heavily based on the vhost-user protocol.

This vhost-user-fs device defines 5 parameters:
  - path: vhost-user socket path
  - tag: mount tag used from the guest to mount the filesystem
  - req_num_queues: number of request virtqueues
  - queue_size: depth of each virtqueue
  - cache_size: cache window size for dax

This device needs to be defined before the VM instance is started,
which can be done through the dbs-cli tool with --fs option:
--fs '{
    "sock_path":"/path/to/virtiofs.socket",
    "tag":"myfs",
    "num_queues":1,
    "queue_size":1024,
    "cache_size":0,
    "thread_pool_size":1,
    "cache_policy":"auto",
    "writeback_cache":true,
    "no_open":true,
    "xattr":true,
    "drop_sys_resource":false,
    "mode":"vhostuser",
    "fuse_killpriv_v2":true,
    "no_readdir":false,
}'

Fixes: kata-containers#8428

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
Signed-off-by: Huang Jianan <jnhuang@linux.alibaba.com>
@mofishzz @adamqqqplay
dragonball: add unit test for vhost-user-fs
2a1fc29 
Add some test cases for vhost-user-fs function.

Signed-off-by: Beiyue <beiyue@linux.alibaba.com>
Signed-off-by: Huang Jianan <jnhuang@linux.alibaba.com>
@studychao
Copy link
Member

/test

@mofishzz @adamqqqplay
dragonball: support vhost-user-fs in device manager
5629b74 
This patch implements the virtio-fs device used for filesystem sharing
and heavily based on the vhost-user protocol.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
Signed-off-by: Huang Jianan <jnhuang@linux.alibaba.com>
Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>
@studychao
Copy link
Member

/test

justxuewei
justxuewei approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks!

@studychao studychao merged commit 5054e59 into kata-containers:main Dec 8, 2023
171 of 174 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiangliu jiangliu jiangliu left review comments

@justxuewei justxuewei justxuewei approved these changes

@studychao studychao studychao approved these changes

Assignees
No one assigned
Labels
ok-to-test size/huge Largest and most complex task (probably needs breaking into small pieces)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dragonball: support vhost-user-fs device
6 participants
@adamqqqplay @katacontainersbot @studychao @jiangliu @justxuewei @mofishzz