feat(pool-expand): add logic to derive md_resv_ratio from absolute size

Signed-off-by: Abhilash Shetty <[email protected]>

Author: abhilashshetty04

.gitmodules

@@ -5,4 +5,4 @@
 [submodule "utils/dependencies"]
 	path = utils/dependencies
 	url = ../mayastor-dependencies.git
-	branch = pool_grow_changes
+	branch = develop

io-engine-tests/src/pool.rs

@@ -212,20 +212,17 @@ impl PoolBuilderRpc {
             .map(|r| r.into_inner())
     }
 
-    pub async fn grow(&mut self) -> Result<(Pool, Pool), Status> {
+    pub async fn grow(&mut self) -> Result<Pool, Status> {
         self.rpc()
             .lock()
             .await
             .pool
-            .grow_pool(GrowPoolRequest {
+            .grow_pool_v2(GrowPoolRequest {
                 name: self.name(),
                 uuid: Some(self.uuid()),
             })
             .await
-            .map(|r| {
-                let t = r.into_inner();
-                (t.previous_pool.unwrap(), t.current_pool.unwrap())
-            })
+            .map(|r| (r.into_inner()))
     }
 
     pub async fn get_pool(&self) -> Result<Pool, Status> {

io-engine/src/bin/io-engine-client/v1/pool_cli.rs

@@ -185,7 +185,7 @@ pub fn subcommands() -> Command {
         );
 
     let expand = Command::new("expand")
-        .about("Expand a storage pool to fill the entire underlying device")
+        .about("Expand a storage pool to span the entire underlying device")
         .arg(
             Arg::new("name")
                 .required(true)
@@ -569,6 +569,13 @@ async fn expand(mut ctx: Context, matches: &ArgMatches) -> crate::Result<()> {
         .await
         .context(GrpcStatus)?;
 
+    if list_response.get_ref().pools.is_empty() {
+        return Err(ClientError::GrpcStatus {
+            source: Status::not_found(format!("Pool {name} not found")),
+            backtrace: None,
+        });
+    }
+
     let pool = &list_response.get_ref().pools[0];
     let previous_capacity = pool.capacity;
 

io-engine/src/grpc/v0/mayastor_grpc.rs

@@ -250,8 +250,7 @@ impl From<LvsError> for tonic::Status {
             LvsError::SetProperty { .. } => Status::data_loss(e.to_string()),
             LvsError::WipeFailed { source } => source.into(),
             LvsError::ResourceLockFailed { .. } => Status::aborted(e.to_string()),
-            LvsError::MaxExpansionFactorFormat { .. } => Status::invalid_argument(e.to_string()),
-            LvsError::MaxExpansionFactorParse { .. } => Status::invalid_argument(e.to_string()),
+            LvsError::MaxExpansionParse { .. } => Status::invalid_argument(e.to_string()),
             _ => Status::internal(e.verbose()),
         }
     }

io-engine/src/grpc/v1/pool.rs

@@ -24,7 +24,7 @@ use std::{
     ops::Deref,
     panic::AssertUnwindSafe,
 };
-use tonic::{Request, Status};
+use tonic::{Code, Request, Status};
 
 pub type PoolCreateEncryptionParams = create_pool_request::Encryption;
 pub type PoolImportEncryptionParams = import_pool_request::Encryption;
@@ -448,7 +448,7 @@ impl From<&dyn PoolOps> for Pool {
             disk_capacity: value.disk_capacity(),
             md_info: value.md_props().map(|md| md.into()),
             encrypted: Some(value.encrypted()),
-            max_expandable_size: Some(value.max_expandable_size()),
+            max_expandable_size: value.max_expandable_size(),
         }
     }
 }
@@ -711,7 +711,10 @@ impl PoolRpc for PoolService {
     }
 
     async fn grow_pool(&self, _request: Request<GrowPoolRequest>) -> GrpcResult<GrowPoolResponse> {
-        unimplemented!()
+        Err(Status::new(
+            Code::Unimplemented,
+            "grow_pool is deprecated. Please use grow_pool_v2",
+        ))
     }
 
     #[named]

io-engine/src/lvm/mod.rs

@@ -326,8 +326,8 @@ impl IPoolProps for VolumeGroup {
         false
     }
 
-    fn max_expandable_size(&self) -> u64 {
-        0
+    fn max_expandable_size(&self) -> Option<u64> {
+        None
     }
 }
 

io-engine/src/lvs/lvs_error.rs

@@ -262,13 +262,9 @@ pub enum LvsError {
     ResourceLockFailed {
         msg: String,
     },
-    #[snafu(display("Failed to parse factor {} into float", factor))]
-    MaxExpansionFactorParse {
-        factor: String,
-    },
-    #[snafu(display("Max growth factor {} does not end with x", factor))]
-    MaxExpansionFactorFormat {
-        factor: String,
+    #[snafu(display("{msg}"))]
+    MaxExpansionParse {
+        msg: String,
     },
 }
 
@@ -305,8 +301,7 @@ impl ToErrno for LvsError {
             Self::CloneConfigFailed { .. } => Errno::EINVAL,
             Self::WipeFailed { .. } => Errno::EINVAL,
             Self::ResourceLockFailed { .. } => Errno::EBUSY,
-            Self::MaxExpansionFactorParse { .. } => Errno::EINVAL,
-            Self::MaxExpansionFactorFormat { .. } => Errno::EINVAL,
+            Self::MaxExpansionParse { .. } => Errno::EINVAL,
         }
     }
 }

io-engine/src/lvs/lvs_store.rs

@@ -1,5 +1,5 @@
 use core::f64;
-use std::{convert::TryFrom, fmt::Debug, os::raw::c_void, pin::Pin, ptr::NonNull};
+use std::{convert::TryFrom, fmt::Debug, os::raw::c_void, pin::Pin, ptr::NonNull, str::FromStr};
 
 use byte_unit::Byte;
 use events_api::event::EventAction;
@@ -210,9 +210,9 @@ impl Lvs {
     }
 
     /// Size upto which blobstore can be expanded.
-    pub fn max_expandable_size(&self) -> u64 {
+    pub fn max_expandable_size(&self) -> Option<u64> {
         // TODO: Use spdk function when changes gets merged.
-        0
+        Some(0)
     }
 
     /// returns the UUID of the lvs
@@ -422,21 +422,37 @@ impl Lvs {
         }
     }
 
-    /// Converts floating point metadata reservation ratio into SPDK's format.
-    fn mdp_ratio(args: &PoolArgs) -> Result<u32, LvsError> {
-        if let Some(mut h) = args.md_args.as_ref().and_then(|p| p.max_expansion.clone()) {
-            if h.ends_with("x") {
-                let _ = h.pop();
-                let factor_parsed = h
-                    .parse::<f64>()
-                    .map_err(|_| LvsError::MaxExpansionFactorParse { factor: h })?;
-                Ok((factor_parsed * 100.0) as u32)
-            } else {
-                Err(LvsError::MaxExpansionFactorFormat { factor: h })
-            }
+    /// Derive num_md_pages_per_cluster_ratio from max_expansion which can be a factor or absolute size.
+    fn md_resv_ratio(args: &PoolArgs, capacity: u64) -> Result<u32, LvsError> {
+        let param = match args.md_args.as_ref().and_then(|p| p.max_expansion.clone()) {
+            Some(p) => p,
+            None => return Ok(100),
+        };
+        let factor = if let Some(stripped) = param.strip_suffix('x') {
+            stripped
+                .parse::<f64>()
+                .map_err(|error| LvsError::MaxExpansionParse {
+                    msg: format!(
+                        "Failed to parse factor max_expansion {stripped} as float: {error}"
+                    ),
+                })?
+        } else if param.ends_with('B') {
+            let expand_bytes =
+                Byte::from_str(&param).map_err(|error| LvsError::MaxExpansionParse {
+                    msg: format!("Failed to parse max_expansion {param} as bytes: {error}"),
+                })?;
+            expand_bytes.as_u64() as f64 / capacity as f64
         } else {
-            Ok(0)
-        }
+            return Err(LvsError::MaxExpansionParse {
+                msg: format!("Max expansion factor {param} does not end with x or B"),
+            });
+        };
+        // The Blobstore ensures that we have enough pages in used_cluster_mask to track the current device size.
+        // So, If maxExpansion results is < 1x it still shouldn't matter. Its same as having
+        // default reservation. However, it does impact how many md pages per cluster are reserved.
+        // For ex. if the factor turns out to be 0.5 then blobstore reserves 1 page per 2 clusters.
+        // So lets ensures that we pass at least default reservation.
+        Ok((factor.max(1.0) * 100.0) as u32)
     }
 
     /// Creates a pool on base bdev.
@@ -450,7 +466,7 @@ impl Lvs {
         let bdev_name = if let Some(ref c) = args.crypto_vbdev_name {
             c.clone().into_cstring()
         } else {
-            bdev.into_cstring()
+            bdev.clone().into_cstring()
         };
 
         let cluster_size = if let Some(cluster_size) = args.cluster_size {
@@ -474,9 +490,14 @@ impl Lvs {
                 msg: format!("{cluster_size}, larger than max limit {MAX_CLUSTER_SIZE}"),
             });
         }
-
-        let mdp_ratio = Self::mdp_ratio(&args)?;
-
+        let bdev = UntypedBdev::lookup_by_name(&bdev).ok_or(LvsError::InvalidBdev {
+            source: BdevError::BdevNotFound {
+                name: bdev.to_string(),
+            },
+            name: bdev.to_string(),
+        })?;
+        let bdev_capacity = bdev.num_blocks() * bdev.block_len() as u64;
+        let mdp_ratio = Self::md_resv_ratio(&args, bdev_capacity)?;
         let (sender, receiver) = pair::<ErrnoResult<Lvs>>();
         unsafe {
             if let Some(uuid) = &args.uuid {

io-engine/src/lvs/mod.rs

@@ -232,7 +232,7 @@ impl IPoolProps for Lvs {
         self.encrypted()
     }
 
-    fn max_expandable_size(&self) -> u64 {
+    fn max_expandable_size(&self) -> Option<u64> {
         self.max_expandable_size()
     }
 }

io-engine/src/pool_backend.rs

@@ -234,7 +234,7 @@ pub trait IPoolProps {
     fn committed(&self) -> u64;
     fn md_props(&self) -> Option<PoolMetadataInfo>;
     fn encrypted(&self) -> bool;
-    fn max_expandable_size(&self) -> u64;
+    fn max_expandable_size(&self) -> Option<u64>;
 }
 
 /// A pool factory helper.