Skip to content

Commit

Permalink
Revert allocation transaction committed
Browse files Browse the repository at this point in the history 
This code has two notable drawbacks that make it better to remove.

1. The code is never used as intended. All requests are immediately
followed by a commit. This does not justify the complexity required to
maintain the feature.
2. There is an assumption that will likely be broken in future commits
related to changes in stacking and metadata. The assumption is that
allocations are always driven by the backstore. If this assumption is
broken and allocations to reserve space for metadata are performed at
layers lower than the backstore, these layers don't have protections
against reserving the same space for two separate allocations and then
commiting them both.
  • Loading branch information
@jbaublitz
jbaublitz committed Nov 1, 2023
1 parent e8c9365 commit 052e31d
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 361 deletions.
68 changes: 23 additions & 45 deletions src/engine/strat_engine/backstore/backstore.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use crate::{
data_tier::DataTier,
devices::UnownedDevices,
shared::BlockSizeSummary,
transaction::RequestTransaction,
},
dm::{get_dm, list_of_backstore_devices, remove_optional_devices},
metadata::{MDADataSize, BDA},
Expand Down Expand Up @@ -416,59 +415,40 @@ impl Backstore {
/// retained as a convenience to the caller.
/// Postcondition:
/// forall i, result_i.0 = result_(i - 1).0 + result_(i - 1).1
pub fn request_alloc(
///
/// WARNING: metadata changing event
pub fn alloc(
&mut self,
pool_uuid: PoolUuid,
sizes: &[Sectors],
) -> StratisResult<Option<RequestTransaction>> {
let mut transaction = match self.data_tier.alloc_request(sizes)? {
Some(t) => t,
None => return Ok(None),
};
) -> StratisResult<Option<Vec<(Sectors, Sectors)>>> {
let total_required = sizes.iter().cloned().sum();
let available = self.size() - self.next;
if available < total_required {
if self.data_tier.alloc(sizes) {
self.extend_cap_device(pool_uuid)?;
} else {
return Ok(None);
}
}

let mut next = self.next;
let mut chunks = Vec::new();
for size in sizes {
transaction.add_seg_req((next, *size));
next += *size
chunks.push((self.next, *size));
self.next += *size;
}

// Assert that the postcondition holds.
assert_eq!(
sizes,
transaction
.get_backstore()
chunks
.iter()
.map(|x| x.1)
.collect::<Vec<Sectors>>()
.as_slice()
);

Ok(Some(transaction))
}

/// Commit space requested by request_alloc() to metadata.
///
/// This method commits the newly allocated data segments and then extends the cap device
/// to be the same size as the allocated data size.
pub fn commit_alloc(
&mut self,
pool_uuid: PoolUuid,
transaction: RequestTransaction,
) -> StratisResult<()> {
let segs = transaction.get_backstore();
self.data_tier.alloc_commit(transaction)?;
// This must occur after the segments have been updated in the data tier
self.extend_cap_device(pool_uuid)?;

assert!(self.next <= self.size());

self.next += segs
.into_iter()
.fold(Sectors(0), |mut size, (_, next_size)| {
size += next_size;
size
});

Ok(())
Ok(Some(chunks))
}

/// Get only the datadevs in the pool.
Expand Down Expand Up @@ -1179,11 +1159,10 @@ mod tests {
invariant(&backstore);

// Allocate space from the backstore so that the cap device is made.
let transaction = backstore
.request_alloc(&[INITIAL_BACKSTORE_ALLOCATION])
backstore
.alloc(pool_uuid, &[INITIAL_BACKSTORE_ALLOCATION])
.unwrap()
.unwrap();
backstore.commit_alloc(pool_uuid, transaction).unwrap();

let cache_uuids = backstore
.init_cache(pool_name.clone(), pool_uuid, initcachedevs, None)
Expand Down Expand Up @@ -1295,11 +1274,10 @@ mod tests {
invariant(&backstore);

// Allocate space from the backstore so that the cap device is made.
let transaction = backstore
.request_alloc(&[INITIAL_BACKSTORE_ALLOCATION])
backstore
.alloc(pool_uuid, &[INITIAL_BACKSTORE_ALLOCATION])
.unwrap()
.unwrap();
backstore.commit_alloc(pool_uuid, transaction).unwrap();

let old_device = backstore.device();

Expand Down
16 changes: 2 additions & 14 deletions src/engine/strat_engine/backstore/blockdev.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use crate::{
crypt::CryptHandle,
devices::BlockSizes,
range_alloc::{PerDevSegments, RangeAllocator},
transaction::RequestTransaction,
},
device::blkdev_size,
metadata::{
Expand Down Expand Up @@ -267,19 +266,8 @@ impl StratBlockDev {

/// Find some sector ranges that could be allocated. If more
/// sectors are needed than are available, return partial results.
pub fn request_space(
&self,
size: Sectors,
transaction: &RequestTransaction,
) -> StratisResult<PerDevSegments> {
self.used.request(self.uuid(), size, transaction)
}

/// Commit allocation requested by request_space().
///
/// This method will record the requested allocations in the metadata.
pub fn commit_space(&mut self, segs: PerDevSegments) {
self.used.commit(segs);
pub fn alloc(&mut self, size: Sectors) -> PerDevSegments {
self.used.alloc(size)
}

// ALL SIZE METHODS (except size(), which is in BlockDev impl.)
Expand Down
72 changes: 15 additions & 57 deletions src/engine/strat_engine/backstore/blockdevmgr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ use crate::{
blockdev::StratBlockDev,
crypt::CryptHandle,
devices::{initialize_devices, wipe_blockdevs, UnownedDevices},
range_alloc::PerDevSegments,
shared::{BlkDevSegment, Segment},
transaction::RequestTransaction,
},
metadata::{MDADataSize, BDA},
serde_structs::{BaseBlockDevSave, Recordable},
Expand Down Expand Up @@ -194,80 +192,41 @@ impl BlockDevMgr {
/// not possible to satisfy the request.
/// This method is atomic, it either allocates all requested or allocates
/// nothing.
pub fn request_space(&self, sizes: &[Sectors]) -> StratisResult<Option<RequestTransaction>> {
let mut transaction = RequestTransaction::default();

pub fn alloc(&mut self, sizes: &[Sectors]) -> Option<Vec<Vec<BlkDevSegment>>> {
let total_needed: Sectors = sizes.iter().cloned().sum();
if self.avail_space() < total_needed {
return Ok(None);
return None;
}

for (idx, &needed) in sizes.iter().enumerate() {
let mut lists = Vec::new();

for &needed in sizes.iter() {
let mut alloc = Sectors(0);
let mut segs = Vec::new();
// TODO: Consider greater efficiency for allocation generally.
// Over time, the blockdevs at the start will be exhausted. It
// might be a good idea to keep an auxiliary structure, so that
// only blockdevs with some space left to allocate are accessed.
// In the context of this major inefficiency that ensues over time
// the obvious but more minor inefficiency of this inner loop is
// not worth worrying about.
for bd in &self.block_devs {
for bd in &mut self.block_devs {
if alloc == needed {
break;
}

let r_segs = bd.request_space(needed - alloc, &transaction)?;
for (&start, &length) in r_segs.iter() {
transaction.add_bd_seg_req(
idx,
BlkDevSegment::new(bd.uuid(), Segment::new(*bd.device(), start, length)),
);
}
let r_segs = bd.alloc(needed - alloc);
let blkdev_segs = r_segs.iter().map(|(&start, &length)| {
BlkDevSegment::new(bd.uuid(), Segment::new(*bd.device(), start, length))
});
segs.extend(blkdev_segs);
alloc += r_segs.sum();
}
assert_eq!(alloc, needed);
lists.push(segs);
}

Ok(Some(transaction))
}

/// Commit the allocations calculated by the request_space() method.
///
/// This method converts the block device segments into the necessary data
/// structure and dispatches them to the corresponding block devices to
/// update the internal records of allocated space.
pub fn commit_space(&mut self, mut transaction: RequestTransaction) -> StratisResult<()> {
let mut segs = transaction.drain_blockdevmgr().try_fold(
HashMap::<DevUuid, PerDevSegments>::new(),
|mut map, seg| -> StratisResult<_> {
if let Some(segs) = map.get_mut(&seg.uuid) {
segs.insert(&(seg.segment.start, seg.segment.length))?;
} else {
let mut segs = PerDevSegments::new(
self.block_devs
.iter()
.find(|bd| bd.uuid() == seg.uuid)
.expect(
"Block dev was determined to be present during allocation request",
)
.total_size()
.sectors(),
);
segs.insert(&(seg.segment.start, seg.segment.length))?;
map.insert(seg.uuid, segs);
}

Ok(map)
},
)?;

for (uuid, bd) in self.blockdevs_mut() {
if let Some(segs) = segs.remove(&uuid) {
bd.commit_space(segs);
}
}

Ok(())
Some(lists)
}

/// Write the given data to all blockdevs marking with current time.
Expand Down Expand Up @@ -472,8 +431,7 @@ mod tests {
assert_eq!(mgr.avail_space() + mgr.metadata_size(), mgr.size());

let allocated = Sectors(2);
let transaction = mgr.request_space(&[allocated]).unwrap().unwrap();
mgr.commit_space(transaction).unwrap();
mgr.alloc(&[allocated]).unwrap();
assert_eq!(
mgr.avail_space() + allocated + mgr.metadata_size(),
mgr.size()
Expand Down
35 changes: 13 additions & 22 deletions src/engine/strat_engine/backstore/cache_tier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,14 @@ impl CacheTier {
)));
}

let trans = self
let segments = self
.block_mgr
.request_space(&[avail_space])?
.expect("asked for exactly the space available, must get");
let segments = trans.get_blockdevmgr();
if let Err(e) = self.block_mgr.commit_space(trans) {
self.block_mgr.remove_blockdevs(&uuids)?;
return Err(StratisError::Msg(format!(
"Failed to commit metadata changes: {e}"
)));
}
.alloc(&[avail_space])
.expect("asked for exactly the space available, must get")
.iter()
.flat_map(|s| s.iter())
.cloned()
.collect::<Vec<_>>();
self.cache_segments.coalesce_blkdevsegs(&segments);

Ok((uuids, (true, false)))
Expand Down Expand Up @@ -170,21 +167,15 @@ impl CacheTier {
)));
}

let trans = block_mgr
.request_space(&[meta_space, avail_space - meta_space])?
let mut segments = block_mgr
.alloc(&[meta_space, avail_space - meta_space])
.expect("asked for exactly the space available, must get");
let meta_segments = AllocatedAbove {
inner: trans.get_segs_for_req(0).expect("segments.len() == 2"),
};
let cache_segments = AllocatedAbove {
inner: trans.get_segs_for_req(1).expect("segments.len() == 2"),
inner: segments.pop().expect("segments.len() == 2"),
};
let meta_segments = AllocatedAbove {
inner: segments.pop().expect("segments.len() == 1"),
};
if let Err(e) = block_mgr.commit_space(trans) {
block_mgr.destroy_all()?;
return Err(StratisError::Msg(format!(
"Failed to commit metadata changes: {e}"
)));
}

Ok(CacheTier {
block_mgr,
Expand Down
36 changes: 16 additions & 20 deletions src/engine/strat_engine/backstore/data_tier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use crate::{
blockdevmgr::BlockDevMgr,
devices::UnownedDevices,
shared::{metadata_to_segment, AllocatedAbove, BlkDevSegment, BlockDevPartition},
transaction::RequestTransaction,
},
serde_structs::{BaseDevSave, BlockDevSave, DataTierSave, Recordable},
types::BDARecordResult,
Expand Down Expand Up @@ -91,18 +90,20 @@ impl DataTier {
/// Allocate a region for all sector size requests from unallocated segments in
/// block devices belonging to the data tier. Return Some(_) if requested
/// amount or more was allocated, otherwise, None.
pub fn alloc_request(&self, requests: &[Sectors]) -> StratisResult<Option<RequestTransaction>> {
self.block_mgr.request_space(requests)
}

/// Commit an allocation that was determined to be valid by alloc_request()
/// to metadata.
pub fn alloc_commit(&mut self, transaction: RequestTransaction) -> StratisResult<()> {
let segments = transaction.get_blockdevmgr();
self.block_mgr.commit_space(transaction)?;
self.segments.coalesce_blkdevsegs(&segments);

Ok(())
pub fn alloc(&mut self, requests: &[Sectors]) -> bool {
self.block_mgr
.alloc(requests)
.map(|segments| {
self.segments.coalesce_blkdevsegs(
&segments
.iter()
.flat_map(|s| s.iter())
.cloned()
.collect::<Vec<_>>(),
);
true
})
.unwrap_or(false)
}

/// The sum of the lengths of all the sectors that have been mapped to an
Expand Down Expand Up @@ -260,8 +261,7 @@ mod tests {
let request_amount = data_tier.block_mgr.avail_space() / 2usize;
assert!(request_amount != Sectors(0));

let transaction = data_tier.alloc_request(&[request_amount]).unwrap().unwrap();
data_tier.alloc_commit(transaction).unwrap();
assert!(data_tier.alloc(&[request_amount]));
data_tier.invariant();

// A data tier w/ some amount allocated
Expand All @@ -279,11 +279,7 @@ mod tests {
size = data_tier.size();

// Allocate enough to get into the newly added block devices
let transaction = data_tier
.alloc_request(&[last_request_amount])
.unwrap()
.unwrap();
data_tier.alloc_commit(transaction).unwrap();
assert!(data_tier.alloc(&[last_request_amount]));
data_tier.invariant();

assert!(data_tier.allocated() >= request_amount + last_request_amount);
Expand Down
1 change: 0 additions & 1 deletion src/engine/strat_engine/backstore/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ mod data_tier;
mod devices;
mod range_alloc;
mod shared;
mod transaction;

pub use self::{
backstore::Backstore,
Expand Down
Loading

0 comments on commit 052e31d

Please sign in to comment.