Skip to content

Commit

Permalink
fix: use auto-detected sector size for blockdev
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@dsharma-dc
dsharma-dc committed Jul 18, 2024
1 parent a7e00d7 commit 127fce4
Show file tree
Hide file tree
Showing 5 changed files with 195 additions and 28 deletions.
22 changes: 22 additions & 0 deletions io-engine-tests/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand Down
16 changes: 13 additions & 3 deletions io-engine/src/bdev/aio.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{
convert::TryFrom,
ffi::CString,
fmt::{Debug, Formatter},
os::unix::fs::FileTypeExt,
};

use async_trait::async_trait;
Expand All @@ -29,7 +30,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)
}
}

Expand All @@ -47,6 +48,10 @@ impl TryFrom<&Url> for Aio {
});
}

let path_is_blockdev = std::fs::metadata(url.path())
.ok()
.map_or(false, |meta| meta.file_type().is_block_device());

let mut parameters: HashMap<String, String> =
url.query_pairs().into_owned().collect();

Expand All @@ -58,9 +63,14 @@ 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(
bdev_api::UuidParamParseFailed {
uri: url.to_string(),
Expand Down
19 changes: 17 additions & 2 deletions io-engine/src/bdev/uring.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
use std::{collections::HashMap, convert::TryFrom, ffi::CString};
use std::{
collections::HashMap,
convert::TryFrom,
ffi::CString,
os::unix::fs::FileTypeExt,
};

use async_trait::async_trait;
use futures::channel::oneshot;
Expand Down Expand Up @@ -36,6 +41,10 @@ impl TryFrom<&Url> for Uring {
});
}

let path_is_blockdev = std::fs::metadata(url.path())
.ok()
.map_or(false, |meta| meta.file_type().is_block_device());

let mut parameters: HashMap<String, String> =
url.query_pairs().into_owned().collect();

Expand All @@ -47,7 +56,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(
Expand Down
147 changes: 124 additions & 23 deletions io-engine/tests/lvs_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,33 @@ use std::pin::Pin;

pub mod common;

static DISKNAME1: &str = "/tmp/disk1.img";
static DISKNAME2: &str = "/tmp/disk2.img";
static TESTDIR: &str = "/tmp/io-engine-tests";
static DISKNAME1: &str = "/tmp/io-engine-tests/disk1.img";
static DISKNAME2: &str = "/tmp/io-engine-tests/disk2.img";
static DISKNAME3: &str = "/tmp/io-engine-tests/disk3.img";

#[tokio::test]
async fn lvs_pool_test() {
common::delete_file(&[DISKNAME1.into(), DISKNAME2.into()]);
// Create directory for placing test disk files
// todo: Create this from some common place and use for all other tests too.
let _ = std::process::Command::new("mkdir")
.args(["-p"])
.args([TESTDIR])
.output()
.expect("failed to execute mkdir");

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()
Expand All @@ -32,15 +51,17 @@ async fn lvs_pool_test() {

// should fail to import a pool that does not exist on disk
ms.spawn(async {
assert!(Lvs::import("tpool", "aio:///tmp/disk1.img").await.is_err())
assert!(Lvs::import("tpool", format!("aio://{DISKNAME1}").as_str())
.await
.is_err())
})
.await;

// should succeed to create a pool we can not import
ms.spawn(async {
Lvs::create_or_import(PoolArgs {
name: "tpool".into(),
disks: vec!["aio:///tmp/disk1.img".into()],
disks: vec![format!("aio://{DISKNAME1}")],
uuid: None,
cluster_size: None,
backend: PoolBackend::Lvs,
Expand All @@ -55,16 +76,23 @@ async fn lvs_pool_test() {
// have an idempotent snafu, we dont crash and
// burn
ms.spawn(async {
assert!(Lvs::create("tpool", "aio:///tmp/disk1.img", None, None)
.await
.is_err())
assert!(Lvs::create(
"tpool",
format!("aio://{DISKNAME1}").as_str(),
None,
None
)
.await
.is_err())
})
.await;

// should fail to import the pool that is already imported
// similar to above, we use the import directly
ms.spawn(async {
assert!(Lvs::import("tpool", "aio:///tmp/disk1.img").await.is_err())
assert!(Lvs::import("tpool", format!("aio://{DISKNAME1}").as_str())
.await
.is_err())
})
.await;

Expand All @@ -75,7 +103,7 @@ async fn lvs_pool_test() {
assert_eq!(pool.name(), "tpool");
assert_eq!(pool.used(), 0);
dbg!(pool.uuid());
assert_eq!(pool.base_bdev().name(), "/tmp/disk1.img");
assert_eq!(pool.base_bdev().name(), DISKNAME1);
})
.await;

Expand All @@ -90,9 +118,13 @@ async fn lvs_pool_test() {
// import and export implicitly destroy the base_bdev, for
// testing import and create we
// sometimes create the base_bdev manually
bdev_create("aio:///tmp/disk1.img").await.unwrap();
bdev_create(format!("aio://{DISKNAME1}").as_str())
.await
.unwrap();

assert!(Lvs::import("tpool", "aio:///tmp/disk1.img").await.is_ok());
assert!(Lvs::import("tpool", format!("aio://{DISKNAME1}").as_str())
.await
.is_ok());

let pool = Lvs::lookup("tpool").unwrap();
assert_eq!(pool.uuid(), uuid);
Expand All @@ -107,13 +139,22 @@ async fn lvs_pool_test() {
let uuid = pool.uuid();
pool.destroy().await.unwrap();

bdev_create("aio:///tmp/disk1.img").await.unwrap();
assert!(Lvs::import("tpool", "aio:///tmp/disk1.img").await.is_err());
bdev_create(format!("aio://{DISKNAME1}").as_str())
.await
.unwrap();
assert!(Lvs::import("tpool", format!("aio://{DISKNAME1}").as_str())
.await
.is_err());

assert_eq!(Lvs::iter().count(), 0);
assert!(Lvs::create("tpool", "aio:///tmp/disk1.img", None, None)
.await
.is_ok());
assert!(Lvs::create(
"tpool",
format!("aio://{DISKNAME1}").as_str(),
None,
None
)
.await
.is_ok());

let pool = Lvs::lookup("tpool").unwrap();
assert_ne!(uuid, pool.uuid());
Expand Down Expand Up @@ -181,7 +222,7 @@ async fn lvs_pool_test() {
pool.export().await.unwrap();
let pool = Lvs::create_or_import(PoolArgs {
name: "tpool".to_string(),
disks: vec!["aio:///tmp/disk1.img".to_string()],
disks: vec![format!("aio://{DISKNAME1}")],
uuid: None,
cluster_size: None,
backend: PoolBackend::Lvs,
Expand Down Expand Up @@ -297,8 +338,12 @@ async fn lvs_pool_test() {
// import the pool all shares should be there, but also validate
// the share that not shared to be -- not shared
ms.spawn(async {
bdev_create("aio:///tmp/disk1.img").await.unwrap();
let pool = Lvs::import("tpool", "aio:///tmp/disk1.img").await.unwrap();
bdev_create(format!("aio://{DISKNAME1}").as_str())
.await
.unwrap();
let pool = Lvs::import("tpool", format!("aio://{DISKNAME1}").as_str())
.await
.unwrap();

for l in pool.lvols().unwrap() {
if l.name() == "notshared" {
Expand All @@ -321,7 +366,7 @@ async fn lvs_pool_test() {

let pool = Lvs::create_or_import(PoolArgs {
name: "tpool".into(),
disks: vec!["aio:///tmp/disk1.img".into()],
disks: vec![format!("aio://{DISKNAME1}")],
uuid: None,
cluster_size: None,
backend: PoolBackend::Lvs,
Expand All @@ -336,6 +381,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
Expand All @@ -352,7 +451,7 @@ async fn lvs_pool_test() {
// importing a pool with the wrong name should fail
Lvs::create_or_import(PoolArgs {
name: "jpool".into(),
disks: vec!["aio:///tmp/disk1.img".into()],
disks: vec![format!("aio://{DISKNAME1}")],
uuid: None,
cluster_size: None,
backend: PoolBackend::Lvs,
Expand All @@ -369,7 +468,7 @@ async fn lvs_pool_test() {
ms.spawn(async {
let pool = Lvs::create_or_import(PoolArgs {
name: "tpool2".into(),
disks: vec!["/tmp/disk2.img".into()],
disks: vec![format!("aio://{DISKNAME2}")],
uuid: None,
cluster_size: None,
backend: PoolBackend::Lvs,
Expand All @@ -381,4 +480,6 @@ async fn lvs_pool_test() {
.await;

common::delete_file(&[DISKNAME2.into()]);
common::detach_loopdev(ldev.as_str());
common::delete_file(&[DISKNAME3.into()]);
}
19 changes: 19 additions & 0 deletions scripts/clean-cargo-tests.sh
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
#!/usr/bin/env bash

SCRIPT_DIR="$(dirname "$0")"
ROOT_DIR=$(realpath "$SCRIPT_DIR/..")

sudo nvme disconnect-all

# Detach any loop devices created for test purposes
for back_file in "/tmp/io-engine-tests"/*; do
# Find loop devices associated with the disk image
devices=$(losetup -j "$back_file" -O NAME --noheadings)

# Detach each loop device found
while IFS= read -r device; do
if [ -n "$device" ]; then
echo "Detaching loop device: $device"
losetup -d "$device"
fi
done <<< "$devices"
done
# Delete the directory too
rmdir --ignore-fail-on-non-empty "/tmp/io-engine-tests"


for c in $(docker ps -a --filter "label=io.composer.test.name" --format '{{.ID}}') ; do
docker kill "$c"
docker rm "$c"
Expand Down

0 comments on commit 127fce4

Please sign in to comment.