-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
Make metadata allocation eager and leave data allocation lazy #3097
Conversation
48cc4be
to
a5e9a35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few requests.
fd26039
to
d9e048e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah never mind - I had an old version of the diff open!
data_extend_size: Sectors, | ||
) -> StratisResult<Sectors> { | ||
thinpooldev.suspend(get_dm(), DmOptions::default())?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the event of an error in this function? Either one of the not Ok(Some(transaction))
branches, or an early return from say commit_alloc()
?
What performs the resume on the thinpooldev, or is it left suspended in that case?
Since we don't start manipulating the thinpooldev until the Ok(Some(...))
branch could the suspend me moved into that branch, leaving the error cases a no-op wrt to the device state?
@jbaublitz Plz rebase and squash as you like and I'll merge. |
@mulkieran We decided we'd address this:
in another PR, right? |
Yes. That was my understanding. |
This commit builds on the previous thin provisioning rework. The simplified, eager metadata allocation allows us to decrease fragmentation introduced into the previous change and the lazy data allocation allows us to provide users with the same flexibility around changing requirements that we previously provided.
d9e048e
to
2b828ec
Compare
Related to #3086