fix: use auto-detected sector size for blockdev

This fixes the behaviour where we pass 512 as sector size if the disk uri doesn't contain blk_size parameter. This causes pool creation failure if the underlying disk has a different sector size e.g. 4096. Instead of passing 512, we now pass 0 which lets spdk detect the device's sector size and use that value. Signed-off-by: Diwakar Sharma <[email protected]>
openebs · Jul 17, 2024 · 769cc70 · 769cc70
1 parent 6b4def0
commit 769cc70
Show file tree

Hide file tree

Showing 7 changed files with 148 additions and 8 deletions.
diff --git a/io-engine-tests/src/lib.rs b/io-engine-tests/src/lib.rs
@@ -188,6 +188,28 @@ pub fn truncate_file_bytes(path: &str, size: u64) {
     assert!(output.status.success());
 }
 
+/// Automatically assign a loopdev to path
+pub fn setup_loopdev_file(path: &str, sector_size: Option<u64>) -> String {
+    let log_sec = sector_size.unwrap_or(512);
+
+    let output = Command::new("losetup")
+        .args(["-f", "--show", "-b", &format!("{log_sec}"), path])
+        .output()
+        .expect("failed exec losetup");
+    assert!(output.status.success());
+    // return the assigned loop device
+    String::from_utf8(output.stdout).unwrap().trim().to_string()
+}
+
+/// Detach the provided loop device.
+pub fn detach_loopdev(dev: &str) {
+    let output = Command::new("losetup")
+        .args(["-d", dev])
+        .output()
+        .expect("failed exec losetup");
+    assert!(output.status.success());
+}
+
 pub fn fscheck(device: &str) {
     let output = Command::new("fsck")
         .args([device, "-n"])

diff --git a/io-engine/src/bdev/aio.rs b/io-engine/src/bdev/aio.rs
@@ -14,7 +14,13 @@ use url::Url;
 use spdk_rs::libspdk::{bdev_aio_delete, create_aio_bdev};
 
 use crate::{
-    bdev::{dev::reject_unknown_parameters, util::uri, CreateDestroy, GetName},
+    bdev::{
+        dev::reject_unknown_parameters,
+        path_is_blockdev,
+        util::uri,
+        CreateDestroy,
+        GetName,
+    },
     bdev_api::{self, BdevError},
     core::{UntypedBdev, VerboseError},
     ffihelper::{cb_arg, done_errno_cb, ErrnoResult},
@@ -29,7 +35,7 @@ pub(super) struct Aio {
 
 impl Debug for Aio {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        write!(f, "Aio '{}'", self.name)
+        write!(f, "Aio '{}', 'blk_size: {}'", self.name, self.blk_size)
     }
 }
 
@@ -47,6 +53,7 @@ impl TryFrom<&Url> for Aio {
             });
         }
 
+        let path_is_blockdev = path_is_blockdev(url.path());
         let mut parameters: HashMap<String, String> =
             url.query_pairs().into_owned().collect();
 
@@ -58,7 +65,13 @@ impl TryFrom<&Url> for Aio {
                     value: value.clone(),
                 })?
             }
-            None => 512,
+            None => {
+                if path_is_blockdev {
+                    0
+                } else {
+                    512
+                }
+            }
         };
 
         let uuid = uri::uuid(parameters.remove("uuid")).context(

diff --git a/io-engine/src/bdev/mod.rs b/io-engine/src/bdev/mod.rs
@@ -2,13 +2,15 @@ use async_trait::async_trait;
 
 pub use dev::{device_create, device_destroy, device_lookup, device_open};
 pub use device::{bdev_event_callback, bdev_io_ctx_pool_init, SpdkBlockDevice};
+use libc::{stat, S_IFBLK, S_IFMT};
 pub use nexus::{Nexus, NexusInfo, NexusState};
 pub use nvmx::{
     nvme_io_ctx_pool_init,
     NvmeController,
     NvmeControllerState,
     NVME_CONTROLLERS,
 };
+use std::ffi::CString;
 
 mod aio;
 pub(crate) mod dev;
@@ -48,6 +50,30 @@ pub trait GetName {
     fn get_name(&self) -> String;
 }
 
+/// Check if the path indicates a blockdev, or a regular file. It will be
+/// helpful for cases where we use a regular file e.g. a tmpfs file, as a pool
+/// backend. XXX: If we can't clearly determine if a path is blockdev(error
+/// conditions), we return false today so that caller side can decide what to
+/// do.
+pub fn path_is_blockdev(path: &str) -> bool {
+    // Create a mutable stat struct
+    let mut stat_result: stat = unsafe { std::mem::zeroed() };
+    let path_cstring = match CString::new(path) {
+        Ok(cstr) => cstr,
+        Err(_e) => return false,
+    };
+
+    // Call the stat system call
+    let stat_ret =
+        unsafe { stat(path_cstring.as_ptr(), &mut stat_result as *mut stat) };
+
+    if stat_ret == 0 {
+        // Check if it's a block device
+        stat_result.st_mode & S_IFMT == S_IFBLK
+    } else {
+        false
+    }
+}
 /// Exposes functionality to prepare for persisting reservations in the event
 /// of a power loss.
 /// This can be implemented by each resource that deals with persistent nvme

diff --git a/io-engine/src/bdev/nvmx/qpair.rs b/io-engine/src/bdev/nvmx/qpair.rs
@@ -22,9 +22,9 @@ use spdk_rs::libspdk::{
     spdk_nvme_qpair_set_abort_dnr,
 };
 
+use std::mem::zeroed;
 #[cfg(feature = "spdk-async-qpair-connect")]
 use std::{os::raw::c_void, time::Duration};
-use std::mem::zeroed;
 
 #[cfg(feature = "spdk-async-qpair-connect")]
 use spdk_rs::{

diff --git a/io-engine/src/bdev/uring.rs b/io-engine/src/bdev/uring.rs
@@ -8,7 +8,13 @@ use url::Url;
 use spdk_rs::libspdk::{create_uring_bdev, delete_uring_bdev};
 
 use crate::{
-    bdev::{dev::reject_unknown_parameters, util::uri, CreateDestroy, GetName},
+    bdev::{
+        dev::reject_unknown_parameters,
+        path_is_blockdev,
+        util::uri,
+        CreateDestroy,
+        GetName,
+    },
     bdev_api::{self, BdevError},
     core::UntypedBdev,
     ffihelper::{cb_arg, done_errno_cb, ErrnoResult},
@@ -36,6 +42,7 @@ impl TryFrom<&Url> for Uring {
             });
         }
 
+        let path_is_blockdev = path_is_blockdev(url.path());
         let mut parameters: HashMap<String, String> =
             url.query_pairs().into_owned().collect();
 
@@ -47,7 +54,13 @@ impl TryFrom<&Url> for Uring {
                     value: value.clone(),
                 })?
             }
-            None => 512,
+            None => {
+                if path_is_blockdev {
+                    0
+                } else {
+                    512
+                }
+            }
         };
 
         let uuid = uri::uuid(parameters.remove("uuid")).context(

diff --git a/io-engine/src/core/env.rs b/io-engine/src/core/env.rs
@@ -27,6 +27,7 @@ use spdk_rs::{
     libspdk::{
         spdk_app_shutdown_cb,
         spdk_env_dpdk_post_init,
+        spdk_env_dpdk_rte_eal_init,
         spdk_env_fini,
         spdk_log_close,
         spdk_log_level,
@@ -43,7 +44,6 @@ use spdk_rs::{
         spdk_thread_lib_fini,
         spdk_thread_send_critical_msg,
         spdk_trace_cleanup,
-        spdk_env_dpdk_rte_eal_init,
         SPDK_LOG_DEBUG,
         SPDK_LOG_INFO,
         SPDK_RPC_RUNTIME,

diff --git a/io-engine/tests/lvs_pool.rs b/io-engine/tests/lvs_pool.rs
@@ -18,12 +18,22 @@ pub mod common;
 
 static DISKNAME1: &str = "/tmp/disk1.img";
 static DISKNAME2: &str = "/tmp/disk2.img";
+static DISKNAME3: &str = "/tmp/disk3.img";
 
 #[tokio::test]
 async fn lvs_pool_test() {
-    common::delete_file(&[DISKNAME1.into(), DISKNAME2.into()]);
+    common::delete_file(&[
+        DISKNAME1.into(),
+        DISKNAME2.into(),
+        DISKNAME3.into(),
+    ]);
     common::truncate_file(DISKNAME1, 128 * 1024);
     common::truncate_file(DISKNAME2, 128 * 1024);
+    common::truncate_file(DISKNAME3, 128 * 1024);
+
+    //setup disk3 via loop device using a sector size of 4096.
+    let ldev = common::setup_loopdev_file(DISKNAME3, Some(4096));
+
     let args = MayastorCliArgs {
         reactor_mask: "0x3".into(),
         ..Default::default()
@@ -336,6 +346,60 @@ async fn lvs_pool_test() {
     })
     .await;
 
+    let pool_dev_aio = ldev.clone();
+    // should succeed to create an aio bdev pool on a loop blockdev of 4096
+    // bytes sector size.
+    ms.spawn(async move {
+        Lvs::create_or_import(PoolArgs {
+            name: "tpool_4k_aio".into(),
+            disks: vec![format!("aio://{pool_dev_aio}")],
+            uuid: None,
+            cluster_size: None,
+            backend: PoolBackend::Lvs,
+        })
+        .await
+        .unwrap();
+    })
+    .await;
+
+    // should be able to find our new LVS created on loopdev, and subsequently
+    // destroy it.
+    ms.spawn(async {
+        let pool = Lvs::lookup("tpool_4k_aio").unwrap();
+        assert_eq!(pool.name(), "tpool_4k_aio");
+        assert_eq!(pool.used(), 0);
+        dbg!(pool.uuid());
+        pool.destroy().await.unwrap();
+    })
+    .await;
+
+    let pool_dev_uring = ldev.clone();
+    // should succeed to create an uring pool on a loop blockdev of 4096 bytes
+    // sector size.
+    ms.spawn(async move {
+        Lvs::create_or_import(PoolArgs {
+            name: "tpool_4k_uring".into(),
+            disks: vec![format!("uring://{pool_dev_uring}")],
+            uuid: None,
+            cluster_size: None,
+            backend: PoolBackend::Lvs,
+        })
+        .await
+        .unwrap();
+    })
+    .await;
+
+    // should be able to find our new LVS created on loopdev, and subsequently
+    // destroy it.
+    ms.spawn(async {
+        let pool = Lvs::lookup("tpool_4k_uring").unwrap();
+        assert_eq!(pool.name(), "tpool_4k_uring");
+        assert_eq!(pool.used(), 0);
+        dbg!(pool.uuid());
+        pool.destroy().await.unwrap();
+    })
+    .await;
+
     // validate the expected state of mayastor
     ms.spawn(async {
         // no shares left except for the discovery controller
@@ -381,4 +445,6 @@ async fn lvs_pool_test() {
     .await;
 
     common::delete_file(&[DISKNAME2.into()]);
+    common::detach_loopdev(ldev.as_str());
+    common::delete_file(&[DISKNAME3.into()]);
 }