-
Notifications
You must be signed in to change notification settings - Fork 307
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
DAOS-16927 pool: Store server handles in RDBs #15846
Open
liw
wants to merge
1
commit into
master
Choose a base branch
from
liw/special-hdls
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm bit worried about this "delay the sp_srv_cont_hdl initialization in ds_pool_iv_refresh_hdl()", what if an IV or whatever request reached leader and called ds_cont_hdl_rdb_lookup() before the sp_srv_cont_hdl is initialized?
I also don't quite see why we need to compare the request handle with this global sp_srv_cont_hdl in ds_cont_hdl_rdb_lookup(), this function is only called by cont_iv_ent_fetch() which is for IV_CONT_CAPA, probably because we mistakenly regard global handle as a normal open handle when fetching IV_CONT_CAPA? (see cont_iv_hdl_fetch(), it's called by common request handler and doesn't distinguish global handle from normal handle).
Clearing the global handles in pool_iv_ent_invalid() looks not safe to me too, it looks to me this IV invalidate function could be called when IV request failed? Given that global handles are immutable now, I'm wondering if we still need to invalidate them?
Just raise some concerns here, they could be addressed in a separate ticket.
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.
@NiuYawei, agreed. When writing this patch, this IV code was the most difficult part to investigate. I tried to 1) maintain the current algorithm for the "2.8 code hosting 2.6 pool" case, and 2) avoid making big changes to code I don't confidently understand.
One change I'm confident about is that we need a new IV operation semantics: Update/invalidate an IV on the local node always synchronously, while update/invalidate it synchronously or asynchronously based on flags. Then, this trick here would no longer be necessary. Such semantics would also benefit other cases on a PS leader.
In 3.0, we will be able to drop the current algorithm because 2.6 pools will no longer be supported. We can gradually remove the complexities during (or even before) the 3.0 development.