Some metadata improvements #476
Conversation
mulkieran
force-pushed
the
master-read-exact
branch
from
4dd2863
to
d159cf9
Compare
mulkieran
changed the title
mulkieran
force-pushed
the
master-read-exact
branch
2 times, most recently
from
2182cb3
to
e685c70
Compare
|
rebased. |
mulkieran
force-pushed
the
master-read-exact
branch
from
b142d37
to
9e5d902
Compare
mulkieran
changed the title
|
@agrover review? |
|
It might actually be better to have an |
mulkieran
force-pushed
the
master-read-exact
branch
from
9e5d902
to
87dbb85
Compare
|
Ok, I think this is ready to go now. @agrover review? |
src/engine/strat_engine/metadata.rs
Outdated
| where F: Read + Seek | ||
| { | ||
| try!(f.seek(SeekFrom::Start(0))); | ||
| let mut buf = [0u8; _BDA_STATIC_HDR_SIZE]; | ||
| try!(f.read(&mut buf)); | ||
| Ok(buf) | ||
| let bytes = try!(f.read(&mut buf)); |
There was a problem hiding this comment.
I think read_exact may work well here? https://doc.rust-lang.org/std/io/trait.Read.html#method.read_exact
There was a problem hiding this comment.
That's what clippy suggested, so I tried it, then, when the existing tests failed I realized what was wrong. (I guess it is an archaism that the branch is called master-read-exact still).
What is wrong is that setup() is called in a context where it is not certain whether there is a static header or not. One perfectly good reason for there not being a static header is if we have some peculiar block device which we can't read a lot of data from and which does not belong to Stratis. The result of using read_exact() is to cause engine initialization to promptly fail, presumably in find_all when it tries to determine ownership on some slightly surprising device.
There was a problem hiding this comment.
Match on read_into_buf returning Err in determine_ownership and return DevOwnership::Theirs instead?
If you do stick with this PR's current approach, it really is a change from "read all expected bytes into a buffer or fail" semantics to "read as much as I can and return that", yes? If so, it's likely that functions shouldn't be taking or returning fixed size arrays any longer, but Vecs and slices, which track their own length. Shouldn't need bytes, instead just call .len() on the vec or slice.
There was a problem hiding this comment.
It never had that semantics in the first place. It read all the bytes it found, left the rest of the buffer filled in with zeros, and then fired away its various discovery mechanisms.
There was a problem hiding this comment.
It could be changed to use Read's take() function which returns a Take adaptor which can then be manipulated, although the adaptor ultimately reads into a buffer. That would be a bit more rustic.
Or read_into_buf() could be changed so that it returned a vector which is the length of bytes read, as you suggest.
|
I think that it is probably time to formalize the metadata reading behavior with a state machine, as the scenarios that are being considered are fairly sophisticated. So, this depends on: stratis-storage/stratis-docs#44 and I'm moving it back to "in progress". |
|
close this? obsoleted by #482? |
|
#482 was just the easy parts. |
mulkieran
force-pushed
the
master-read-exact
branch
from
87dbb85
to
54a9e88
Compare
|
rebased. |
mulkieran
force-pushed
the
master-read-exact
branch
from
54a9e88
to
1b1ea03
Compare
The current location is where the fix would have to happen. Signed-off-by: mulhern <[email protected]>
It's an action that must be done twice, and there are a lot of decisions involved, so make it a method. Signed-off-by: mulhern <[email protected]>
It's a bit clearer and the plan is to use them a bit more. Signed-off-by: mulhern <[email protected]>
When trying to read a StaticHeader it may be that the buffer doesn't get filled with data from the file or device being read. Signed-off-by: mulhern <[email protected]>
The last test indicates a desirable fix: if it is only possible to read two sectors of the BDA, loading the BDA should return an error. Signed-off-by: mulhern <[email protected]>
mulkieran
force-pushed
the
master-read-exact
branch
from
1b1ea03
to
afc11ad
Compare
There is ongoing work, see: stratis-storage#476, and the ultimate plan is to come up w/ better formalization of how errors in reading metadata are handled. Signed-off-by: mulhern <[email protected]>
There is ongoing work, see: stratis-storage#476, and the ultimate plan is to come up w/ better formalization of how errors in reading metadata are handled. Signed-off-by: mulhern <[email protected]>
|
closing due to inactivity. |
|
It is active again, but I don't seem to be able to reopen it. |
This PR does the following:
MDARegions::initialize().sigblock_from_buf()so that it takes into account the possibility of a partial read.MDARegions::load()by having it return an error if there is truly an error in reading an MDA region.