runtime-rs: fix the issues mentioned in the code review

Removed the `Debug` trait for the `ShareFs` and etc. Renamed
`ShareFsMount::upgrade()` and `ShareFsMount::downgrade()` to
`upgrade_to_rw()` and `downgrade_to_ro()`. Protected `mounted_info_set`
with a mutex to avoid race conditions.

Fixes: #5588

Signed-off-by: Xuewei Niu <justxuewei@apache.org>

src/runtime-rs/crates/resource/src/share_fs/mod.rs

@@ -11,11 +11,12 @@ use share_virtio_fs_inline::ShareVirtioFsInline;
 mod share_virtio_fs_standalone;
 use share_virtio_fs_standalone::ShareVirtioFsStandalone;
 mod utils;
+use tokio::sync::Mutex;
 pub use utils::{do_get_guest_path, do_get_guest_share_path, get_host_rw_shared_path};
 mod virtio_fs_share_mount;
 use virtio_fs_share_mount::VirtiofsShareMount;
 
-use std::{fmt::Debug, path::PathBuf, sync::Arc};
+use std::{collections::HashMap, fmt::Debug, path::PathBuf, sync::Arc};
 
 use agent::Storage;
 use anyhow::{anyhow, Context, Ok, Result};
@@ -38,26 +39,12 @@ pub const PASSTHROUGH_FS_DIR: &str = "passthrough";
 const RAFS_DIR: &str = "rafs";
 
 #[async_trait]
-pub trait ShareFs: Send + Sync + Debug {
+pub trait ShareFs: Send + Sync {
     fn get_share_fs_mount(&self) -> Arc<dyn ShareFsMount>;
     async fn setup_device_before_start_vm(&self, h: &dyn Hypervisor) -> Result<()>;
     async fn setup_device_after_start_vm(&self, h: &dyn Hypervisor) -> Result<()>;
     async fn get_storages(&self) -> Result<Vec<Storage>>;
-
+    fn mounted_info_set(&self) -> Arc<Mutex<HashMap<String, MountedInfo>>>;
-    /// Get a mounted info from ShareFs.
-    /// The source is an original path on the host (not in the `/run/kata-containers/...`).
-    async fn get_mounted_info(&self, source: &str) -> Option<MountedInfo>;
-    /// Set a mounted info to ShareFS.
-    /// The source is the same as get_mounted_info's.
-    async fn set_mounted_info(&self, source: &str, mounted_info: MountedInfo) -> Result<()>;
-    /// Remove a mounted info from ShareFs.
-    /// The source is the same as get_mounted_info's.
-    async fn rm_mounted_info(&self, source: &str) -> Result<Option<MountedInfo>>;
-    /// Get a mounted info by guest path.
-    async fn get_mounted_info_by_guest_path(
-        &self,
-        guest_path: &str,
-    ) -> Option<(String, MountedInfo)>;
 }
 
 #[derive(Debug)]
@@ -132,13 +119,13 @@ impl MountedInfo {
 }
 
 #[async_trait]
-pub trait ShareFsMount: Send + Sync + Debug {
+pub trait ShareFsMount: Send + Sync {
     async fn share_rootfs(&self, config: ShareFsRootfsConfig) -> Result<ShareFsMountResult>;
     async fn share_volume(&self, config: ShareFsVolumeConfig) -> Result<ShareFsMountResult>;
     /// Upgrade to readwrite permission
-    async fn upgrade(&self, file_name: &str) -> Result<()>;
+    async fn upgrade_to_rw(&self, file_name: &str) -> Result<()>;
     /// Downgrade to readonly permission
-    async fn downgrade(&self, file_name: &str) -> Result<()>;
+    async fn downgrade_to_ro(&self, file_name: &str) -> Result<()>;
     /// Umount the volume
     async fn umount(&self, file_name: &str) -> Result<()>;
 }

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs_inline.rs

@@ -11,7 +11,7 @@ use anyhow::{Context, Result};
 use async_trait::async_trait;
 use hypervisor::Hypervisor;
 use kata_types::config::hypervisor::SharedFsInfo;
-use tokio::sync::RwLock;
+use tokio::sync::Mutex;
 
 use super::{
     share_virtio_fs::{
@@ -30,16 +30,10 @@ pub struct ShareVirtioFsInlineConfig {
     pub id: String,
 }
 
-#[derive(Default, Debug)]
-pub struct ShareVirtioFsInlineInner {
-    mounted_info_set: HashMap<String, MountedInfo>,
-}
-
-#[derive(Debug)]
 pub struct ShareVirtioFsInline {
     config: ShareVirtioFsInlineConfig,
     share_fs_mount: Arc<dyn ShareFsMount>,
-    inner: Arc<RwLock<ShareVirtioFsInlineInner>>,
+    mounted_info_set: Arc<Mutex<HashMap<String, MountedInfo>>>,
 }
 
 impl ShareVirtioFsInline {
@@ -47,7 +41,7 @@ impl ShareVirtioFsInline {
         Ok(Self {
             config: ShareVirtioFsInlineConfig { id: id.to_string() },
             share_fs_mount: Arc::new(VirtiofsShareMount::new(id)),
-            inner: Arc::new(RwLock::new(ShareVirtioFsInlineInner::default())),
+            mounted_info_set: Arc::new(Mutex::new(HashMap::new())),
         })
     }
 }
@@ -89,33 +83,7 @@ impl ShareFs for ShareVirtioFsInline {
         Ok(storages)
     }
 
-    async fn get_mounted_info(&self, source: &str) -> Option<MountedInfo> {
+    fn mounted_info_set(&self) -> Arc<Mutex<HashMap<String, MountedInfo>>> {
-        let inner = self.inner.read().await;
+        self.mounted_info_set.clone()
-        inner.mounted_info_set.get(source).cloned()
-    }
-
-    async fn set_mounted_info(&self, source: &str, mounted_info: MountedInfo) -> Result<()> {
-        let mut inner = self.inner.write().await;
-        inner
-            .mounted_info_set
-            .insert(source.to_owned(), mounted_info.clone());
-        Ok(())
-    }
-
-    async fn rm_mounted_info(&self, source: &str) -> Result<Option<MountedInfo>> {
-        let mut inner = self.inner.write().await;
-        Ok(inner.mounted_info_set.remove(source))
-    }
-
-    async fn get_mounted_info_by_guest_path(
-        &self,
-        guest_path: &str,
-    ) -> Option<(String, MountedInfo)> {
-        let inner = self.inner.read().await;
-        inner
-            .mounted_info_set
-            .iter()
-            .find(|m| m.1.guest_path.as_os_str().to_str().unwrap() == guest_path)
-            .map(|m| (m.0.to_owned(), m.1.clone()))
     }
 }

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs_standalone.rs

@@ -16,7 +16,7 @@ use tokio::{
     process::{Child, Command},
     sync::{
         mpsc::{channel, Receiver, Sender},
-        RwLock,
+        Mutex, RwLock,
     },
 };
 
@@ -41,14 +41,13 @@ pub struct ShareVirtioFsStandaloneConfig {
 #[derive(Default, Debug)]
 struct ShareVirtioFsStandaloneInner {
     pid: Option<u32>,
-    mounted_info_set: HashMap<String, MountedInfo>,
 }
 
-#[derive(Debug)]
 pub(crate) struct ShareVirtioFsStandalone {
     inner: Arc<RwLock<ShareVirtioFsStandaloneInner>>,
     config: ShareVirtioFsStandaloneConfig,
     share_fs_mount: Arc<dyn ShareFsMount>,
+    mounted_info_set: Arc<Mutex<HashMap<String, MountedInfo>>>,
 }
 
 impl ShareVirtioFsStandalone {
@@ -63,6 +62,7 @@ impl ShareVirtioFsStandalone {
                 virtio_fs_extra_args: config.virtio_fs_extra_args.clone(),
             },
             share_fs_mount: Arc::new(VirtiofsShareMount::new(id)),
+            mounted_info_set: Arc::new(Mutex::new(HashMap::new())),
         })
     }
 
@@ -176,33 +176,7 @@ impl ShareFs for ShareVirtioFsStandalone {
         Ok(vec![])
     }
 
-    async fn get_mounted_info(&self, source: &str) -> Option<MountedInfo> {
+    fn mounted_info_set(&self) -> Arc<Mutex<HashMap<String, MountedInfo>>> {
-        let inner = self.inner.read().await;
+        self.mounted_info_set.clone()
-        inner.mounted_info_set.get(source).cloned()
-    }
-
-    async fn set_mounted_info(&self, source: &str, mounted_info: MountedInfo) -> Result<()> {
-        let mut inner = self.inner.write().await;
-        inner
-            .mounted_info_set
-            .insert(source.to_owned(), mounted_info.clone());
-        Ok(())
-    }
-
-    async fn rm_mounted_info(&self, source: &str) -> Result<Option<MountedInfo>> {
-        let mut inner = self.inner.write().await;
-        Ok(inner.mounted_info_set.remove(source))
-    }
-
-    async fn get_mounted_info_by_guest_path(
-        &self,
-        guest_path: &str,
-    ) -> Option<(String, MountedInfo)> {
-        let inner = self.inner.read().await;
-        inner
-            .mounted_info_set
-            .iter()
-            .find(|m| m.1.guest_path.as_os_str().to_str().unwrap() == guest_path)
-            .map(|m| (m.0.to_owned(), m.1.clone()))
     }
 }

src/runtime-rs/crates/resource/src/share_fs/virtio_fs_share_mount.rs

@@ -170,7 +170,7 @@ impl ShareFsMount for VirtiofsShareMount {
         })
     }
 
-    async fn upgrade(&self, file_name: &str) -> Result<()> {
+    async fn upgrade_to_rw(&self, file_name: &str) -> Result<()> {
         // Remount readonly directory with readwrite permission
         let host_dest = do_get_host_path(file_name, &self.id, "", true, true);
         bind_remount(&host_dest, false)
@@ -182,7 +182,7 @@ impl ShareFsMount for VirtiofsShareMount {
         Ok(())
     }
 
-    async fn downgrade(&self, file_name: &str) -> Result<()> {
+    async fn downgrade_to_ro(&self, file_name: &str) -> Result<()> {
         // Remount readwrite directory with readonly permission
         let host_dest = do_get_host_path(file_name, &self.id, "", true, false);
         bind_remount(&host_dest, true)
@@ -195,10 +195,9 @@ impl ShareFsMount for VirtiofsShareMount {
     }
 
     async fn umount(&self, file_name: &str) -> Result<()> {
-        let host_dest = do_get_host_path(file_name, &self.id, "", true, false);
-        umount_timeout(&host_dest, 0).context("Umount readonly host dest")?;
         let host_dest = do_get_host_path(file_name, &self.id, "", true, true);
         umount_timeout(&host_dest, 0).context("Umount readwrite host dest")?;
+        // Umount event will be propagated to ro directory
         Ok(())
     }
 }

src/runtime-rs/crates/resource/src/volume/mod.rs

@@ -18,7 +18,7 @@ use tokio::sync::RwLock;
 use crate::share_fs::ShareFs;
 
 #[async_trait]
-pub trait Volume: Send + Sync + std::fmt::Debug {
+pub trait Volume: Send + Sync {
     fn get_volume_mount(&self) -> Result<Vec<oci::Mount>>;
     fn get_storage(&self) -> Result<Vec<agent::Storage>>;
     async fn cleanup(&self) -> Result<()>;

src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs

@@ -23,7 +23,6 @@ use kata_types::mount;
 // only regular files in /dev. It does not make sense to pass the host
 // device nodes to the guest.
 // skip the volumes whose source had already set to guest share dir.
-#[derive(Debug)]
 pub(crate) struct ShareFsVolume {
     share_fs: Option<Weak<dyn ShareFs>>,
     mounts: Vec<oci::Mount>,
@@ -71,7 +70,9 @@ impl ShareFsVolume {
                 let readonly = m.options.iter().any(|opt| opt == "ro");
 
                 let share_fs_mount = share_fs.get_share_fs_mount();
-                if let Some(mut mounted_info) = share_fs.get_mounted_info(&m.source).await {
+                let mounted_info_set = share_fs.mounted_info_set();
+                let mut mounted_info_set = mounted_info_set.lock().await;
+                if let Some(mut mounted_info) = mounted_info_set.get(&m.source).cloned() {
                     // Mounted at least once
                     let guest_path = mounted_info
                         .guest_path
@@ -87,7 +88,7 @@ impl ShareFsVolume {
                             "The mount will be upgraded, mount = {:?}, cid = {}", m, cid
                         );
                         share_fs_mount
-                            .upgrade(
+                            .upgrade_to_rw(
                                 &mounted_info
                                     .file_name()
                                     .context("get name of mounted info")?,
@@ -100,10 +101,7 @@ impl ShareFsVolume {
                     } else {
                         mounted_info.rw_ref_count += 1;
                     }
-                    share_fs
+                    mounted_info_set.insert(m.source.clone(), mounted_info);
-                        .set_mounted_info(&m.source, mounted_info)
-                        .await
-                        .context("set mounted info")?;
 
                     volume.mounts.push(oci::Mount {
                         destination: m.destination.clone(),
@@ -131,10 +129,7 @@ impl ShareFsVolume {
                             .context("convert guest path")?,
                         readonly,
                     );
-                    share_fs
+                    mounted_info_set.insert(m.source.clone(), mounted_info);
-                        .set_mounted_info(&m.source, mounted_info)
-                        .await
-                        .context("set mounted info")?;
                     // set storages for the volume
                     volume.storages = mount_result.storages;
 
@@ -171,18 +166,23 @@ impl Volume for ShareFsVolume {
             None => return Err(anyhow!("The share_fs was released unexpectedly")),
         };
 
+        let mounted_info_set = share_fs.mounted_info_set();
+        let mut mounted_info_set = mounted_info_set.lock().await;
         for m in self.mounts.iter() {
-            let (host_source, mut mounted_info) =
+            let (host_source, mut mounted_info) = match mounted_info_set
-                match share_fs.get_mounted_info_by_guest_path(&m.source).await {
+                .iter()
-                    Some(entry) => entry,
+                .find(|entry| entry.1.guest_path.as_os_str().to_str().unwrap() == m.source)
-                    None => {
+                .map(|entry| (entry.0.to_owned(), entry.1.clone()))
-                        warn!(
+            {
-                            sl!(),
+                Some(entry) => entry,
-                            "The mounted info for guest path {} not found", m.source
+                None => {
-                        );
+                    warn!(
-                        continue;
+                        sl!(),
-                    }
+                        "The mounted info for guest path {} not found", m.source
-                };
+                    );
+                    continue;
+                }
+            };
 
             let old_readonly = mounted_info.readonly();
 
@@ -206,23 +206,17 @@ impl Volume for ShareFsVolume {
                 if !old_readonly && mounted_info.readonly() {
                     info!(sl!(), "Downgrade {} to readonly due to no container that needs readwrite permission", host_source);
                     share_fs_mount
-                        .downgrade(&file_name)
+                        .downgrade_to_ro(&file_name)
                         .await
                         .context("Downgrade volume")?;
                 }
-                share_fs
+                mounted_info_set.insert(host_source.clone(), mounted_info);
-                    .set_mounted_info(&host_source, mounted_info)
-                    .await
-                    .context("Update mounted info")?;
             } else {
                 info!(
                     sl!(),
                     "The path will be umounted due to no references, host_source = {}", host_source
                 );
-                share_fs
+                mounted_info_set.remove(&host_source);
-                    .rm_mounted_info(&host_source)
-                    .await
-                    .context("Rm mounted info due to no reference")?;
                 // Umount the volume
                 share_fs_mount
                     .umount(&file_name)

src/runtime-rs/crates/runtimes/virt_container/src/container_manager/container_inner.rs

@@ -277,7 +277,12 @@ impl ContainerInner {
         for v in self.volumes.iter() {
             if let Err(err) = v.cleanup().await {
                 unhandled.push(Arc::clone(v));
-                warn!(sl!(), "Failed to clean volume {:?}, error = {:?}", v, err);
+                warn!(
+                    sl!(),
+                    "Failed to clean volume {:?}, error = {:?}",
+                    v.get_volume_mount(),
+                    err
+                );
             }
         }
         if !unhandled.is_empty() {