Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tasleson read bda signature #1440

Closed
wants to merge 2 commits into from
Closed

Tasleson read bda signature #1440

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4aaf21c
BDA::read change function return values
tasleson Feb 21, 2019
700ac15
Return an error under two more sets of circumstances
mulkieran Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 108 additions & 54 deletions src/engine/strat_engine/backstore/metadata.rs
View file
Original file line number Diff line number Diff line change
@@ -46,7 +46,7 @@ impl BDA {
/// Read the BDA from the device and return 2 SECTORS worth of data, one for each BDA returned
/// in the order of layout on disk (location 1, location 2).
/// Only the BDA sectors are read up from disk, zero areas are *not* read.
fn read<F>(f: &mut F) -> io::Result<([u8; SECTOR_SIZE], [u8; SECTOR_SIZE])>
fn read<F>(f: &mut F) -> (io::Result<[u8; SECTOR_SIZE]>, io::Result<[u8; SECTOR_SIZE]>)
where
F: Read + Seek,
{
@@ -69,13 +69,10 @@ impl BDA {
Ok(())
}

let loc_1_read_result = read_sector_at_offset(f, SECTOR_SIZE, &mut buf_loc_1);
let loc_2_read_result = read_sector_at_offset(f, 9 * SECTOR_SIZE, &mut buf_loc_2);

match (loc_1_read_result, loc_2_read_result) {
(Err(loc_1_err), Err(_)) => Err(loc_1_err),
_ => Ok((buf_loc_1, buf_loc_2)),
}
(
read_sector_at_offset(f, SECTOR_SIZE, &mut buf_loc_1).map(|_| buf_loc_1),
read_sector_at_offset(f, 9 * SECTOR_SIZE, &mut buf_loc_2).map(|_| buf_loc_2),
)
}

// Writes bda_buf according to the value of which.
@@ -267,69 +264,126 @@ impl StaticHeader {
/// Return the latest copy that validates as a Stratis BDA, however verify both
/// copies and if one validates but one does not, re-write the one that is incorrect. If both
/// copies are valid, but one is newer than the other, rewrite the older one to match.
/// Return None if the static header's magic does not match for *both* copies.
/// Return None if it's not a Stratis device.
/// Return an error if the metadata seems to indicate that the device is
/// a Stratis device, but no well-formed signature block could be read.
Copy link
Member Author

@mulkieran mulkieran Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns an error in two situations:

  • If the metadata seems to indicate that the device is a Stratis deivce, but no well-formed signature could be found.
  • If a well-formed signature was found but an error occurred during the repair of a second, ill-formed, old, or unreadable signature.

I think it is right to return an error in the first case, but I think the second should possibly be handled differently. I think a good option is to enrich the return type to
StratisResult<Option<StaticHeader>, Option<some error thing if repair fails>>.
Alternatively, it is fine to open an issue to address this question in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New issue: #1443

/// Return an error if neither sigblock location can be read.
/// Return an error if the sigblocks differ in some unaccountable way.
/// Returns an error if a write intended to repair an ill-formed,
/// unreadable, or stale signature block failed.
fn setup<F>(f: &mut F) -> StratisResult<Option<StaticHeader>>
where
F: Read + Seek + SyncAll,
{
let (buf_loc_1, buf_loc_2) = BDA::read(f)?;

match (
StaticHeader::sigblock_from_buf(&buf_loc_1),
StaticHeader::sigblock_from_buf(&buf_loc_2),
) {
(Ok(loc_1), Ok(loc_2)) => {
match (loc_1, loc_2) {
(Some(loc_1), Some(loc_2)) => {
if loc_1 == loc_2 {
Ok(Some(loc_1))
} else if loc_1.initialization_time > loc_2.initialization_time {
match BDA::read(f) {
(Ok(buf_loc_1), Ok(buf_loc_2)) => {
// We read both copies without an IO error.
match (
StaticHeader::sigblock_from_buf(&buf_loc_1),
StaticHeader::sigblock_from_buf(&buf_loc_2),
) {
(Ok(loc_1), Ok(loc_2)) => {
match (loc_1, loc_2) {
(Some(loc_1), Some(loc_2)) => {
if loc_1 == loc_2 {
Ok(Some(loc_1))
} else if loc_1.initialization_time == loc_2.initialization_time {
// Inexplicable disagreement among static headers
let err_str = "Appeared to be a Stratis device, but signature blocks disagree.";
Err(StratisError::Engine(ErrorEnum::Invalid, err_str.into()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this would be extremely unlikely, but if it did happen I would like to have the contents of each super blocks dumped to the logs.

} else if loc_1.initialization_time > loc_2.initialization_time {
// If the first header block is newer, overwrite second with
// contents of first.
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
Ok(Some(loc_1))
} else {
// The second header block must be newer, so overwrite first
// with contents of second.
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(Some(loc_2))
}
}
(None, None) => Ok(None),
(Some(loc_1), None) => {
// Copy 1 has valid Stratis BDA, copy 2 has no magic, re-write copy 2
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
Ok(Some(loc_1))
}
(None, Some(loc_2)) => {
// Copy 2 has valid Stratis BDA, copy 1 has no magic, re-write copy 1
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(Some(loc_2))
}
}
}
(Ok(loc_1), Err(loc_2)) => {
// Re-write copy 2
if loc_1.is_some() {
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
Ok(Some(loc_1))
Ok(loc_1)
} else {
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(Some(loc_2))
// Location 1 doesn't have a signature, but location 2 did, but it got an error,
// lets return the error instead as this appears to be a stratis device that
// has gotten in a bad state.
Err(loc_2)
}
}
(None, None) => Ok(None),
(Some(loc_1), None) => {
// Copy 1 has valid Stratis BDA, copy 2 has no magic, re-write copy 2
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
Ok(Some(loc_1))
(Err(loc_1), Ok(loc_2)) => {
// Re-write copy 1
if loc_2.is_some() {
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(loc_2)
} else {
// Location 2 doesn't have a signature, but location 1 did, but it got an error,
// lets return the error instead as this appears to be a stratis device that
// has gotten in a bad state.
Err(loc_1)
}
}
(None, Some(loc_2)) => {
// Copy 2 has valid Stratis BDA, copy 1 has no magic, re-write copy 1
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(Some(loc_2))
(Err(_), Err(_)) => {
let err_str =
"Appeared to be a Stratis device, but no valid sigblock found";
Copy link
Member Author

@mulkieran mulkieran Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well enhance this error message. The contents of both buffers are available, so they might as well be included. Actually, better to include both errors, and improve the error messages in the errors returned from sigblock_from_buf. I'ld say that it makes sense to file an issue for improving sigblock_from_buf error messages rather than address that in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New issue: #1444

Err(StratisError::Engine(ErrorEnum::Invalid, err_str.into()))
}
}
}
(Ok(loc_1), Err(loc_2)) => {
// Re-write copy 2
if loc_1.is_some() {
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
Ok(loc_1)
} else {
// Location 1 doesn't have a signature, but location 2 did, but it got an error,
// lets return the error instead as this appears to be a stratis device that
// has gotten in a bad state.
Err(loc_2)
(Ok(buf_loc_1), Err(_)) => {
// Copy 1 read OK, 2 resulted in an IO error
match StaticHeader::sigblock_from_buf(&buf_loc_1) {
Ok(loc_1) => {
if loc_1.is_some() {
BDA::write(f, &buf_loc_1, MetadataLocation::Second)?;
}
Ok(loc_1)
}
Err(e) => {
// Unable to determine if location 2 has a signature, but location 1 did,
// but it got an error, lets return the error instead as this appears to
// be a stratis device that has gotten in a bad state.
Err(e)
}
}
}
(Err(loc_1), Ok(loc_2)) => {
// Re-write copy 1
if loc_2.is_some() {
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
Ok(loc_2)
} else {
// Location 2 doesn't have a signature, but location 1 did, but it got an error,
// lets return the error instead as this appears to be a stratis device that
// has gotten in a bad state.
Err(loc_1)
(Err(_), Ok(buf_loc_2)) => {
// Copy 2 read OK, 1 resulted in IO Error
match StaticHeader::sigblock_from_buf(&buf_loc_2) {
Ok(loc_2) => {
if loc_2.is_some() {
BDA::write(f, &buf_loc_2, MetadataLocation::First)?;
}
Ok(loc_2)
}
Err(e) => {
// Unable to determine if location 1 has a signature, but location 2 did,
// but it got an error, lets return the error instead as this appears to
// be a stratis device that has gotten in a bad state.
Err(e)
}
}
}
(Err(_), Err(_)) => {
let err_str = "Appeared to be a Stratis device, but no valid sigblock found";
// Unable to read the device at all.
let err_str = "Unable to read data at sigblock locations.";
Err(StratisError::Engine(ErrorEnum::Invalid, err_str.into()))
Copy link
Contributor

@tasleson tasleson Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning None was the previous behavior. scratch that, if we got a read error from both we would have bubbled up the read error from the first one.

}
}