ZNS support #1298

MaisenbacherD · 2023-01-27T14:08:41Z

This draft PR addresses the issue #1297 and shall encourage discussion.
This should not be merged as is as there are two dependencies that need to be resolved before this can potentially be merged:

The OpenEBS fork of spdk needs a branch for v23.01 similar to https://github.com/MaisenbacherD/spdk/commits/wip-v23.01.x-mayastor when v23.01 is released.
The draft PR ZNS Support spdk-rs#24 for spdk-rs needs to be resolved and the submodule reference must be updated accordingly

MaisenbacherD · 2023-06-16T13:52:56Z

Rebased this PR onto develop

orville-wright · 2024-02-10T21:31:36Z

Hi @MaisenbacherD - I believe that SPDK v23.01 is now integrated into Mayastor.

orville-wright · 2024-02-10T21:34:19Z

@tiagolobocastro can you confirm that SPDK v23.01 is now the current default base SPDK version in the product?

orville-wright · 2024-02-14T11:20:52Z

Hi @MaisenbacherD I just chatted with @tiagolobocastro and he clarified that we will be updating to the latest release of SPDK soon, which is v24.01. and that we are already at v23.01 as of now.

MaisenbacherD · 2024-02-20T08:03:32Z

Hi @orville-wright
Thanks for confirming!
This draft was rebased onto the develop branch when SPDK v23.01 was already picked up by you - so the SPDK version requirement is fulfilled :)

Let me know if there is something else I can do.

tiagolobocastro

haven't finished, will resume tomorrow

tiagolobocastro · 2024-02-28T11:58:00Z

io-engine-tests/src/lib.rs

+    nr_seq_zones: u32,
+    max_active_zones: u32,
+    max_open_zones: u32,
+) -> Result<u32, (i32, String)> {


Could we return a new type Wrapper with RAII instead? example:

Suggested change

) -> Result<u32, (i32, String)> {

) -> Result<NullBlk, (i32, String)> {

struct NullBlk(u32); impl Drop for NullBlk { fn drop(&mut self) { delete_nullblk_device(self.0); } }

tiagolobocastro · 2024-02-28T11:59:20Z

io-engine-tests/src/lib.rs

@@ -541,3 +542,92 @@ macro_rules! test_diag {
        println!($($arg)*);
    }}
 }
+
+pub fn create_zoned_nullblk_device(


Please add some doc comments explaining what this nullblk is, for example, does it just ignore writes and return zeroes?

tiagolobocastro · 2024-02-28T12:00:28Z

io-engine/Cargo.toml

@@ -103,6 +103,7 @@ url = "2.2.2"
 gettid = "0.1.2"
 async-process = { version = "1.5.0" }
 rstack = { version = "0.3.2" }
+jemalloc-sys = "0.5.2+5.3.0-patched"


what is this used for? A patched version makes wonder if this is stable enough..

Thank you for mentioning this!
I needed a way to allocate memory for the spdk_bdev_zone_info c struct. I am using calloc and free of this allocator crate.

Can you suggest a nicer way to do this? :)

@hrudaya21 Unfortunately, there is no convenient way to allocate this struct within SPDK.

tiagolobocastro · 2024-02-28T12:01:59Z

io-engine/src/bdev/device.rs

+        SPDK_BDEV_ZONE_STATE_OFFLINE => Ok(SPDK_NVME_ZONE_STATE_OFFLINE),
+        SPDK_BDEV_ZONE_STATE_EXP_OPEN => Ok(SPDK_NVME_ZONE_STATE_EOPEN),
+        _ => {
+            error!("Can not map SPDK_BDEV_ZONE_STATE {} to any SPDK_NVME_ZONE_STATE", bdev_zone_state);


Suggested change

error!("Can not map SPDK_BDEV_ZONE_STATE {} to any SPDK_NVME_ZONE_STATE", bdev_zone_state);

error!("Can't map SPDK_BDEV_ZONE_STATE {bdev_zone_state} to any SPDK_NVME_ZONE_STATE");

tiagolobocastro · 2024-02-28T12:02:14Z

io-engine/src/bdev/device.rs

+            error!("Can not map SPDK_BDEV_ZONE_STATE {} to any SPDK_NVME_ZONE_STATE", bdev_zone_state);
+            Err(CoreError::NvmeIoPassthruDispatch {
+                source: Errno::EINVAL,
+                opcode: 122,


What does 122 mean? and 121 below

Stands for Zone Management Receive and Zone Management Send respectively. I encoded those values into an enum.

tiagolobocastro · 2024-02-28T13:09:29Z

io-engine/src/core/block_device.rs

@@ -82,6 +85,14 @@ pub trait BlockDevice {
        &self,
        listener: DeviceEventSink,
    ) -> Result<(), CoreError>;
+
+    fn is_zoned(&self) -> bool;
+    fn get_zone_size(&self) -> u64;


nit: in rust getters, don't tend to have get_ prefix

tiagolobocastro · 2024-02-28T13:10:55Z

io-engine/src/bdev/nvme.rs

            bdev.remove_alias(self.url.as_ref());
            let errno = unsafe {
                bdev_nvme_delete(
                    self.name.clone().into_cstring().as_ptr(),
-                    std::ptr::null(),
+                    &mut path_id as *mut nvme_path_id,


needs to be rebased

tiagolobocastro · 2024-02-28T13:16:31Z

io-engine/src/bdev/nexus/nexus_io.rs

@@ -253,10 +266,20 @@ impl<'n> NexusBio<'n> {
        if status == IoCompletionStatus::Success {
            self.ctx_mut().successful += 1;
        } else {
+            error!(


we already have traces below? I'd prefer not to add too many traces in the IO path until we add a redirection. Currently traces are a blocking call so if stdout/err is not pulled fast enough it can block the task.

Sure, it makes sense. I will remove this line. :)

tiagolobocastro · 2024-02-28T13:18:15Z

io-engine/src/bdev/nexus/nexus_io.rs


-            self.completion_error(child, status);
+            // Don't take zoned child out on zoned related nvme errors


why don't we want to take it out in this case?

Zoned storage related status codes like TOO_MAY_OPEN_ZONES or ZONE_IS_FULL might be handled by the consuming application that is aware of the zoned storage interface.

Because no zoned replication is enabled yet, taking out the nexus is a bit harsh.
With zoned replication support later on we might want to take the nexus out again, depending on the logic of the replication.

tiagolobocastro · 2024-02-28T13:21:36Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+) -> Result<Vec<(String, String)>, Error> {
+    let mut children_devices = Vec::new();
+    for uri in children {
+        let device_name = device_create(uri).await.unwrap();


need to handle errors here rather than unwrap

MaisenbacherD · 2024-02-29T08:26:04Z

haven't finished, will resume tomorrow

@tiagolobocastro Thanks so far! I will address your comments soon after you are done :)

tiagolobocastro

@dsavitskiy @dsharma-dc @hrudaya21 please take a look as well

tiagolobocastro · 2024-03-04T14:17:46Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+    max_zone_append_size: u32,
+    max_open_zones: u32,
+    max_active_zones: u32,
+    optimal_open_zones: u32,


Would be good to have some doc comments here describing what these mean for the folks who are not very familiar with zones, such as myself :)

I will add doc comments. Struct moves to spdk-rs/src/bdev.rs as I refactored the with_zoned bdev builder function to take the ZoneInfo and spdk-rs should not depend on mayastor.

tiagolobocastro · 2024-03-04T14:18:25Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+impl Default for ZoneInfo {
+    fn default() -> ZoneInfo{
+        ZoneInfo{
+            zoned: false,
+            num_zones: Default::default(),
+            zone_size: Default::default(),
+            max_zone_append_size: Default::default(),
+            max_open_zones: Default::default(),
+            max_active_zones: Default::default(),
+            optimal_open_zones: Default::default(),
+        }
+    }
+}


Simply add Default to the derive above

Suggested change

impl Default for ZoneInfo {

fn default() -> ZoneInfo{

ZoneInfo{

zoned: false,

num_zones: Default::default(),

zone_size: Default::default(),

max_zone_append_size: Default::default(),

max_open_zones: Default::default(),

max_active_zones: Default::default(),

optimal_open_zones: Default::default(),

}

}

}

tiagolobocastro · 2024-03-04T14:19:36Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -387,6 +418,13 @@ impl<'n> Nexus<'n> {
            .with_block_count(0)
            .with_required_alignment(9)
            .with_data(n)
+            .with_zoned(zone_info.zoned)


nit: would be tidier to add a with_zone method which takes the zone_info rather than adding all these here 1by1?

I agree! Will change this :)

tiagolobocastro · 2024-03-04T14:21:01Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -404,6 +442,7 @@ impl<'n> Nexus<'n> {
                name.to_string(),
                n.bdev.as_mut().unwrap(),
            ));
+            n.is_zoned = zone_info.zoned;


can this not be set in the build()?

I will remove the is_zoned field as it was unused. Zoned information can be retrieved via the bdev.

tiagolobocastro · 2024-03-04T14:22:17Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+    let mut info_set = false;
+
+    for (_uri, device_name) in &*children_devices {
+        let dev = device_lookup(&device_name).unwrap();


should return an error

tiagolobocastro · 2024-03-04T14:24:52Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+    let mut zone_info = ZoneInfo::default();
+    let mut conventional = false;
+    let mut info_set = false;


Is there another name for non-zoned blk devices btw? (rather than conventional)

Suggested change

let mut zone_info = ZoneInfo::default();

let mut conventional = false;

let mut info_set = false;

// if we find non-zoned block devices.

let mut found_conventional = false;

let mut zone_info: Option<ZoneInfo> = None;

Usually, non-zoned blk devs are called "conventional". In the NVMe context, one could also label such a device as an NVM device (vs ZNS for zoned block devices), which refers to the I/O command set.

tiagolobocastro · 2024-03-04T14:27:18Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+
+        if dev.is_zoned() {
+            zone_info.zoned = true;
+            if !info_set {


if let Some(zone_info) = zone_info.as_mut() or match

tiagolobocastro · 2024-03-04T14:28:25Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+        }
+
+        if zone_info.zoned == conventional {
+            error!("Can not handle conventional and zoned storage at the same time in a nexus");


maybe include the nexus in the log too?

Suggested change

error!("Can not handle conventional and zoned storage at the same time in a nexus");

error!("{nexus:?} - can not handle conventional and zoned storage at the same time in a nexus");

The nexus does not exist at this point, but I can include the name of the requested nexus.

tiagolobocastro · 2024-03-04T14:34:37Z

io-engine/src/bdev/device.rs

+        }
+
+        unsafe {
+            free(bdev_zone_info_c_void);


can this be done with RAII?

Most likely yes. Let me wait on that with the discussion above about the jemalloc-sys crate. :)

tiagolobocastro · 2024-03-04T14:35:09Z

io-engine/src/bdev/device.rs

+
+        let bdev_zone_info_c_void = unsafe { calloc(1, size_of_spdk_bdev_zone_info) };
+        loop {
+            if zone >= max_num_zones_to_report {


Similar question I guess, should this be async?

Yes, I agree. Let me carry this change out when the discussion of the jamalloc-sys crate is resolved :)

dsharma-dc · 2024-03-13T05:39:11Z

io-engine/src/core/env.rs

@@ -814,6 +817,29 @@ impl MayastorEnvironment {
        None
    }

+    fn init_tracing(&self) {


init_spdk_tracing perhaps.

dsharma-dc · 2024-03-13T05:58:49Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+    for uri in children {
+        let device_name = device_create(uri).await.unwrap();
+        if device_lookup(&device_name).unwrap().is_zoned() && children.len() > 1 {
+            return Err(Error::ZonedReplicationNotImplemented {});


If we are returning without creating all devices, don't we want to destroy back what we would've already created?

hrudaya21 · 2024-03-13T06:52:39Z

io-engine/src/bdev/device.rs

+
+        // Bit 07:00 Dword 13 > Zone Send Action
+        let zsa = zone_send_action_to_bdev_zone_action(nvme_cmd.cdw13 as u8).unwrap();
+


Avoid unwrap() here.

hrudaya21 · 2024-03-13T07:05:07Z

io-engine/src/bdev/device.rs

+        let bdev_zone_infos;
+
+        let ret = unsafe {
+            bdev_zone_infos = calloc(max_num_zones_to_report as usize, size_of_spdk_bdev_zone_info);


is it possible to have this allcation inside the SPDK function, so that, if any failure happens, We can have a graceful exit here ?

Unfortunately, there is no convenient way to allocate this struct within SPDK.

Can you suggest a nicer way to allocate memory for this struct? :)

dsharma-dc · 2024-03-13T08:20:41Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+            if dev.is_zoned() {
+                //TODO: Implement partitioning zoned block devices. This requires handling drive resources like max active/open zones.
+                warn!("The device '{}' is zoned. Partitioning zoned block devices into smaller devices is not implemented. Using the whole device.", dev.device_name());
+                start_blk = 0;


This may not work correctly because we have to reserve some space in the child device for metadata. That space calculation is done by calc_data_partition and start/end blocks are hence adjusted.

Thanks for catching this! I was not aware that a metadata partition is necessary. I assumed that partitioning is just a feature that allows the creation of multiple Nexus from the same drive.

What kind of metadata needs space on the drive and where in the code can I find the IO requests being issued for those metadata operations? :)

Also, I am not sure if I fully understand what is happening in the match partition::calc_data_partition(self.req_size(), nb, bs) { code block. Does that mean the first self.req_size() bytes are reserved for metadata?

dsavitskiy · 2024-03-13T08:12:17Z

io-engine/src/core/block_device.rs

@@ -82,6 +85,14 @@ pub trait BlockDevice {
        &self,
        listener: DeviceEventSink,
    ) -> Result<(), CoreError>;
+
+    fn is_zoned(&self) -> bool;


I've the same suggestion.

dsavitskiy · 2024-03-13T08:14:36Z

io-engine/src/core/block_device.rs

@@ -62,6 +62,9 @@ pub trait BlockDevice {
    /// Checks whether target I/O type is supported by the device.
    fn io_type_supported(&self, io_type: IoType) -> bool;

+    /// Checks whether target I/O type is supported by the device.
+    fn io_type_supported_by_device(&self, io_type: IoType) -> bool;


How is this different from io_type_supported()? Please describe the difference in the comment.

This is a great question!
The io_type_supported_by_device still checks the supported io types of the drive.

For the ZNS feature, I had to implement NVMe IO passthrough support (see commit "feat(nexus): Add NVMe IO passthrough support") to enable Zone Management Send and Receive commands.

The io_type_supported returns always true for NvmeIo commands, because also zoned SpdkBlockDevices need to support the NvmeIo commands that are being issued to the zoned Nexus NVMe-oF target. Because SpdkBlockDevices do not natively support NvmeIo commands, the commit "feat(nexus): Emulate zone mgmt NvmeIo for SpdkBlockDevices" emulates this command.

The submit_io_passthru function takes the NvmeIo commands and either passes it on to the device if that command is natively supported or it checks if the NvmeIo.opc is emulated in software. Finally, the NvmeIo command gets rejected (errno::EOPNOTSUPP) if no emulation for that opcode is present.

I will adjust the docs of io_type_supported

Let me know if you have further questions or ideas on how to refactor this. :)

dsavitskiy · 2024-03-13T08:15:49Z

io-engine/src/core/block_device.rs

@@ -82,6 +85,14 @@ pub trait BlockDevice {
        &self,
        listener: DeviceEventSink,
    ) -> Result<(), CoreError>;
+
+    fn is_zoned(&self) -> bool;


Please add meaningful comments for all new methods, ideally with args and return value descriptions.

dsavitskiy · 2024-03-13T08:16:10Z

io-engine/src/core/block_device.rs

@@ -212,7 +223,51 @@ pub trait BlockDeviceHandle {
        cb_arg: IoCompletionCallbackArg,
    ) -> Result<(), CoreError>;

+    /// TODO


Please add meaningful comments for all new methods, ideally with args and return value descriptions.

dsavitskiy · 2024-03-13T08:16:54Z

io-engine/src/core/env.rs

@@ -199,7 +199,7 @@ impl Default for MayastorCliArgs {
            reactor_mask: "0x1".into(),
            mem_size: 0,
            rpc_address: "/var/tmp/mayastor.sock".to_string(),
-            no_pci: true,
+            no_pci: false,


Hm, why this change is needed? Is it only for ZNS?

This change would just be a 'nice to have'. It does not only target ZNS.
As part of the "feat(test): Enable PCIe device testing" commit, this enables testing on real PCIe devices. The idea is to later write some test cases that can run in a CI environment on real devices.

Let me know if I should drop this change :)

dsavitskiy · 2024-03-13T08:22:51Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -517,6 +556,10 @@ impl<'n> Nexus<'n> {
        unsafe { self.bdev().required_alignment() }
    }

+    pub fn is_zoned(&self) -> bool {


Please add a doc comment for all new fields, methods, etc.

dsavitskiy · 2024-03-13T08:25:23Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+        if dev.is_zoned() {
+            zone_info.zoned = true;
+            if !info_set {
+                zone_info.num_zones = dev.get_num_zones();


It looks like adding a couple of helper methods to copy and compare zone info would make code much cleaner.

dsavitskiy · 2024-03-13T08:26:13Z

io-engine/src/bdev/nexus/nexus_io.rs

            | IoType::Reset
            | IoType::Unmap
            | IoType::Flush => self.submit_all(),
+            IoType::ZoneAppend => {
+                warn!("ZoneAppend is explicitly disallowed, otherwise reading from different replicas won't work.");


Please add the nexus info in the warn! message.

dsavitskiy · 2024-03-13T08:29:29Z

io-engine/src/core/mod.rs

+pub fn is_zoned_nvme_error(status: IoCompletionStatus) -> bool {
+    match status {
+        IoCompletionStatus::NvmeError(NvmeStatus::CommandSpecific(cssc)) => match cssc {
+            CommandSpecificStatusCode::ZonedBoundaryError => true,


Pipe | operator would make this code clearer.

dsavitskiy · 2024-03-13T08:29:42Z

io-engine/src/core/mod.rs

@@ -350,6 +350,23 @@ impl From<NvmeStatus> for IoCompletionStatus {
    }
 }

+pub fn is_zoned_nvme_error(status: IoCompletionStatus) -> bool {


Please add a doc comment for all new fields, methods, etc.

dsharma-dc · 2024-03-13T08:45:33Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+                zone_info.max_active_zones = dev.get_max_active_zones();
+                zone_info.optimal_open_zones = dev.get_optimal_open_zones();
+                info_set = true;
+            } else if zone_info.num_zones != dev.get_num_zones()


Better implement or derive PartialEq trait for ZoneInfo

hrudaya21 · 2024-03-13T09:29:27Z

io-engine/src/bdev/nexus/nexus_bdev.rs

+    let mut children_devices = Vec::new();
+    for uri in children {
+        let device_name = device_create(uri).await.unwrap();
+        if device_lookup(&device_name).unwrap().is_zoned() && children.len() > 1 {


Avoid unwrap() here as well.

hrudaya21 · 2024-03-13T09:46:16Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -1371,6 +1480,14 @@ async fn nexus_create_internal(
        return Ok(());
    }

+    let mut children_devices = create_children_devices(children).await?;


For the normal child (not zoned device), whether this function will fail ?

In the normal case, the create_children_devices function is only failing if the device_create function returns an error.

orville-wright · 2024-03-13T09:47:26Z

hi @MaisenbacherD it looks like all the requested reviewers have given significant feedback on the PR. So you should have lots of guidance on moving this project forward now. Let me know of you need any more assistance. Happy to help.

MaisenbacherD · 2024-03-13T10:01:34Z

Thanks everyone for the reviews! I am addressing your comments in the following days :)

hrudaya21 · 2024-03-13T10:18:26Z

io-engine/src/bdev/nexus/nexus_io.rs

+        } else {
+            match orig_nvme_cmd.opc() {
+                // Zone Management Send
+                121 => return hdl.emulate_zone_mgmt_send_io_passthru(


Suggest to have opcode enum.

Abhinandan-Purkait · 2024-03-15T06:53:16Z

io-engine/src/bdev/nexus/mod.rs

@@ -7,7 +7,7 @@ use futures::{future::Future, FutureExt};

 mod nexus_bdev;
 mod nexus_bdev_children;
-mod nexus_bdev_error;
+pub mod nexus_bdev_error;


Can this be pub(crate) and maybe at other places?

Abhinandan-Purkait · 2024-03-15T06:58:05Z

io-engine/src/bdev/nvmx/device.rs

+        self.ns.is_zoned()
+    }
+
+    fn get_zone_size(&self) -> u64 {


We don't need to use get_ prefix?

Abhinandan-Purkait · 2024-03-15T07:02:47Z

nix/pkgs/btrfs-progs/default.nix

+
+stdenv.mkDerivation rec {
+  pname = "btrfs-progs";
+  version = "5.14.1";


Can we not pin the version? Also we use 5.6.x for btrfs operations on csi, so maybe it should be at least be in par with that.

orville-wright · 2024-05-28T13:24:16Z

hi @MaisenbacherD - checking in here on where things are on the project?

let's talk to see how we can move this forward

Init SPDK tracing when `MayastorEnvironment.num_enties` is set to a positive integer. This can be helpfull when developing new features. Traces can be found in `/dev/shm` and have the pid or if not negative the `MayastorEnvironment.shm_id` as a suffix. Further information about traces and how to read the captured traces can be found here: https://spdk.io/doc/nvmf_tgt_tracepoints.html Signed-off-by: Dennis Maisenbacher <[email protected]>

Add btrfs-progs package for btrfs support. This is a preperation commit for zoned storage support as btrfs also supports zoned storage. Signed-off-by: Dennis Maisenbacher <[email protected]>

Move children device creation out from `Nexus::new_child` into a helper function. Create the children devices before the Nexus is created and add them after the Nexus's initialization. This is in preperation for zoned storge support, as we need to inspect the children device features before creating a Nexus such that we can decide whether the Nexus must be zoned or not. Signed-off-by: Dennis Maisenbacher <[email protected]>

Adding setter and getter for zone storage related fields to the Nexus and BlockDevice. Inspect the children before creating the Nexus to decide whether the Nexus needs to be zoned or not. The Nexus inherrits the zoned related fields form its child. For now a zoned Nexus does not allow replication. Partitioning of a zoned Nexus is also not supported for now. The user will optain the whole zoned Nexus if requested. Signed-off-by: Dennis Maisenbacher <[email protected]>

Passthrough NvmeIo if the child (NVMe device) supports it. This is necessary for the Zoned Storage support, as the Zone Management Send and Receive commands are issued to the Nexus as NvmeIo. Those commands are issued when zones are reported or the state of a zone is actively changed. Signed-off-by: Dennis Maisenbacher <[email protected]>

Zoned block devices that do not support NvmeIo (zoned uring) need to get the zone management send and receive NvmeIo translated that is beeing issued on the Nexus. Those commands are issued when zones are reported or the state of a zone is actively changed. Inspect fields of the incoming NVMe command and fill the buffer according to the 'NVM Express Zoned Namespace Command Set Specification' (see https://nvmexpress.org/developers/nvme-command-set-specifications/) with the help of the SPDK zoned bdev wrapper functions. Signed-off-by: Dennis Maisenbacher <[email protected]>

Testing zoned storage support through zoned uring with a backing zoned nullblk device. Signed-off-by: Dennis Maisenbacher <[email protected]>

Signed-off-by: Dennis Maisenbacher <[email protected]>

Zoned related CommandSpecificStatusCodes's should not take out the nexus. Signed-off-by: Dennis Maisenbacher <[email protected]>

MaisenbacherD

Thanks, everyone for the helpful reviews, and my apologies for the late reply!
I hope that I addressed all comments with either a discussion or a direct code change.
There are some more open discussions to resolve :)

Also, I will address the btrfs nix package comments and post a fix for the Failed to write to /dev/nvme-fabrics: Invalid argument on a more recent kernel when I am back from vacation in two weeks. :)

Please also have a look at the PR for spdk-rs that is required by this series: openebs/spdk-rs#24

MaisenbacherD · 2024-06-05T12:33:10Z

io-engine/src/core/env.rs

@@ -199,7 +199,7 @@ impl Default for MayastorCliArgs {
            reactor_mask: "0x1".into(),
            mem_size: 0,
            rpc_address: "/var/tmp/mayastor.sock".to_string(),
-            no_pci: true,
+            no_pci: false,


This change would just be a 'nice to have'. It does not only target ZNS.
As part of the "feat(test): Enable PCIe device testing" commit, this enables testing on real PCIe devices. The idea is to later write some test cases that can run in a CI environment on real devices.

Let me know if I should drop this change :)

MaisenbacherD · 2024-06-06T07:30:17Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -1371,6 +1480,14 @@ async fn nexus_create_internal(
        return Ok(());
    }

+    let mut children_devices = create_children_devices(children).await?;


In the normal case, the create_children_devices function is only failing if the device_create function returns an error.

MaisenbacherD · 2024-06-06T08:09:52Z

io-engine/src/bdev/device.rs

@@ -83,7 +174,7 @@ impl SpdkBlockDevice {
    pub fn lookup_by_name(name: &str) -> Option<Box<dyn BlockDevice>> {
        debug!("Searching SPDK devices for '{}'...", name);
        let bdev = UntypedBdev::lookup_by_name(name)?;
-        debug!("SPDK {} device found: '{}'", bdev.driver(), name);
+        debug!("SPDK {} device found: '{}'", bdev.driver(), bdev.name());


There should be no difference between name and bdev.name().

This change stuck from debugging and was meant to clarify that the name of the found bdev is supposed to be printed out and match with the debug statement above.

I can revert this change if you want :)

MaisenbacherD · 2024-06-06T09:52:48Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -387,6 +418,13 @@ impl<'n> Nexus<'n> {
            .with_block_count(0)
            .with_required_alignment(9)
            .with_data(n)
+            .with_zoned(zone_info.zoned)


I agree! Will change this :)

MaisenbacherD · 2024-06-06T11:26:59Z

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -253,6 +256,7 @@ pub struct Nexus<'n> {
    pub(super) nexus_target: Option<NexusTarget>,
    /// Indicates if the Nexus has an I/O device.
    pub(super) has_io_device: bool,
+    pub(super) is_zoned: bool,


I will remove this field as it was unused. Zoned information can be retrieved via the bdev.

MaisenbacherD · 2024-06-07T19:00:48Z

io-engine/src/bdev/device.rs

+        }
+
+        unsafe {
+            free(bdev_zone_info_c_void);


Most likely yes. Let me wait on that with the discussion above about the jemalloc-sys crate. :)

MaisenbacherD · 2024-06-07T19:07:37Z

io-engine/src/bdev/device.rs

+
+        let bdev_zone_info_c_void = unsafe { calloc(1, size_of_spdk_bdev_zone_info) };
+        loop {
+            if zone >= max_num_zones_to_report {


Yes, I agree. Let me carry this change out when the discussion of the jamalloc-sys crate is resolved :)

MaisenbacherD · 2024-06-07T19:09:19Z

io-engine/src/bdev/device.rs

+
+        let mut result;
+        loop {
+            result = unsafe {


Yes, I agree. Let me carry this change out when the discussion of the jamalloc-sys crate is resolved :)

MaisenbacherD · 2024-06-07T19:51:52Z

io-engine/src/bdev/nexus/nexus_io.rs

@@ -253,10 +266,20 @@ impl<'n> NexusBio<'n> {
        if status == IoCompletionStatus::Success {
            self.ctx_mut().successful += 1;
        } else {
+            error!(


Sure, it makes sense. I will remove this line. :)

MaisenbacherD · 2024-06-07T20:00:49Z

io-engine/src/bdev/nexus/nexus_io.rs


-            self.completion_error(child, status);
+            // Don't take zoned child out on zoned related nvme errors


Zoned storage related status codes like TOO_MAY_OPEN_ZONES or ZONE_IS_FULL might be handled by the consuming application that is aware of the zoned storage interface.

Because no zoned replication is enabled yet, taking out the nexus is a bit harsh.
With zoned replication support later on we might want to take the nexus out again, depending on the logic of the replication.

orville-wright · 2024-06-12T03:06:32Z

Thanks @MaisenbacherD -
We are very grateful that you have taken the time and effort to enhance openEBS with such an innovative feature. Your contribution to the project & the community is very valuable and very appreciated.
It makes openEBS even better for everyone.

We also appreciate that an industry respected storage innovator (WD.com) supports openEBS with their resources & your time/expertise. Thank you WD.com

The team will review and pass on comments. Great progress.

MaisenbacherD · 2024-08-12T11:22:51Z

Hi @orville-wright,

Apart from the review comments, would it be possible to get some help with enabling the latest btrfs, libnvme and nvme-cli nix packages? I am not that fluent in nix yet. :)

orville-wright · 2024-08-12T11:25:11Z

Sure thing. I'll loop get you connected with the right people on the team.

orville-wright · 2024-08-17T18:21:10Z

Hey @tiagolobocastro @niladrih and @Abhinandan-Purkait - Dennis @MaisenbacherD needs some eng help with some of the packages... specifically btrfs, libnvme and nvme-cli nix.
He's said that he's not that fluent in nix yet.

Since this is an important community / partner led feature... can you guys sync up with Dennis and let's see if we can help him out.
We really need to get this awesome ZNS feature coded & released.

~Dave

Abhinandan-Purkait · 2024-08-19T08:18:58Z

Sure. FYI, btrfs support has already been added to mayastor, We just need to specify fstype: btrfs in storage class to have brtfs filesystem volumes backed by mayastor.

orville-wright · 2024-08-20T19:34:30Z

Hi @MaisenbacherD looks like some feedback was given by @Abhinandan-Purkait , but I'm not sure if he provided enough of the info you needed ?

MaisenbacherD · 2024-08-22T05:42:53Z

@orville-wright, @Abhinandan-Purkait great! I was not aware that btrfs is already an option :)

The current libnvme and nvme-cli nix packages in use look outdated. I would need help to get the latest nix upstream packages configured. I am not sure that adjusting e.g. nix/pkgs/nvmet-cli/default.nix is the correct way to do that :)

tiagolobocastro · 2024-08-27T09:29:33Z

@orville-wright, @Abhinandan-Purkait great! I was not aware that btrfs is already an option :)

The current libnvme and nvme-cli nix packages in use look outdated. I would need help to get the latest nix upstream packages configured. I am not sure that adjusting e.g. nix/pkgs/nvmet-cli/default.nix is the correct way to do that :)

To be honest, you can probably just remove the specific files we've got for those, and just rely on the versions from nixpkgs.
Otherwise you can edit those files to pull the latest versions you need.

tiagolobocastro · 2024-08-29T12:00:46Z

@MaisenbacherD sorry for short notice, would you be able to join our community meeting today: https://us05web.zoom.us/j/87535654586?pwd=CigbXigJPn38USc6Vuzt7qSVFoO79X.1
It's at 2pm UTC

MaisenbacherD · 2024-08-29T12:31:33Z

Hi @tiagolobocastro. Yes, I will join :) Talk to you soon!

MaisenbacherD force-pushed the zns-support branch from 56b6aa8 to cf95cde Compare June 16, 2023 13:51

orville-wright added Enhancement New feature or request Ready for review Ready to be reviewed by team labels Feb 27, 2024

tiagolobocastro requested review from dsavitskiy, hrudaya21, Abhinandan-Purkait and dsharma-dc February 28, 2024 11:52

tiagolobocastro reviewed Feb 28, 2024

View reviewed changes

tiagolobocastro reviewed Mar 4, 2024

View reviewed changes

dsharma-dc reviewed Mar 13, 2024

View reviewed changes

hrudaya21 reviewed Mar 13, 2024

View reviewed changes

dsharma-dc reviewed Mar 13, 2024

View reviewed changes

dsavitskiy requested changes Mar 13, 2024

View reviewed changes

dsharma-dc reviewed Mar 13, 2024

View reviewed changes

hrudaya21 reviewed Mar 13, 2024

View reviewed changes

orville-wright assigned MaisenbacherD Mar 13, 2024

hrudaya21 reviewed Mar 13, 2024

View reviewed changes

Abhinandan-Purkait reviewed Mar 15, 2024

View reviewed changes

tiagolobocastro removed the Ready for review Ready to be reviewed by team label May 1, 2024

MaisenbacherD added 2 commits June 5, 2024 14:55

feat(nix): Add btrfs-progs package

d79cc74

Add btrfs-progs package for btrfs support. This is a preperation commit for zoned storage support as btrfs also supports zoned storage. Signed-off-by: Dennis Maisenbacher <[email protected]>

MaisenbacherD mentioned this pull request Jun 7, 2024

ZNS Support openebs/spdk-rs#24

Draft

MaisenbacherD added 8 commits June 7, 2024 22:51

feat(test): Add cargo test for Zoned Storage Support

5e320a3

Testing zoned storage support through zoned uring with a backing zoned nullblk device. Signed-off-by: Dennis Maisenbacher <[email protected]>

docs: Improve docs for hacking on Mayastor

5995d31

Signed-off-by: Dennis Maisenbacher <[email protected]>

docs: Add doc for introduced zoned storage support

0e10011

Signed-off-by: Dennis Maisenbacher <[email protected]>

feat(nvme): On command completion handle zoned nvme errors

344c3fb

Zoned related CommandSpecificStatusCodes's should not take out the nexus. Signed-off-by: Dennis Maisenbacher <[email protected]>

MaisenbacherD commented Jun 7, 2024

View reviewed changes

MaisenbacherD force-pushed the zns-support branch from cf95cde to 344c3fb Compare June 7, 2024 21:17

orville-wright added Ready for review Ready to be reviewed by team reviewing PR Pending labels Jun 12, 2024

orville-wright assigned orville-wright and tiagolobocastro Jun 12, 2024

orville-wright added Selected for development Accepted into roadmap for product integration External Contrib High priority Community contrib code labels Jun 16, 2024

	) -> Result<u32, (i32, String)> {
	) -> Result<NullBlk, (i32, String)> {

	error!("Can not map SPDK_BDEV_ZONE_STATE {} to any SPDK_NVME_ZONE_STATE", bdev_zone_state);
	error!("Can't map SPDK_BDEV_ZONE_STATE {bdev_zone_state} to any SPDK_NVME_ZONE_STATE");


		self.completion_error(child, status);
		// Don't take zoned child out on zoned related nvme errors

	error!("Can not handle conventional and zoned storage at the same time in a nexus");
	error!("{nexus:?} - can not handle conventional and zoned storage at the same time in a nexus");


		// Bit 07:00 Dword 13 > Zone Send Action
		let zsa = zone_send_action_to_bdev_zone_action(nvme_cmd.cdw13 as u8).unwrap();

ZNS support #1298

Are you sure you want to change the base?

ZNS support #1298

Conversation

MaisenbacherD commented Jan 27, 2023

MaisenbacherD commented Jun 16, 2023

orville-wright commented Feb 10, 2024

orville-wright commented Feb 10, 2024

orville-wright commented Feb 14, 2024

MaisenbacherD commented Feb 20, 2024

tiagolobocastro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaisenbacherD commented Feb 29, 2024

tiagolobocastro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrudaya21 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

hrudaya21 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orville-wright commented Mar 13, 2024

MaisenbacherD commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orville-wright commented May 28, 2024

MaisenbacherD left a comment

Choose a reason for hiding this comment

hrudaya21 Mar 13, 2024 •

edited

Loading

hrudaya21 Mar 13, 2024 •

edited

Loading