-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[TieredStorage] Make TieredStorage::write_accounts() thread-safe #35143
Conversation
a5aae48
to
72fa85d
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #35143 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 832 832
Lines 224856 224861 +5
=======================================
+ Hits 183561 183583 +22
+ Misses 41295 41278 -17 |
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.
Overall this runtime checking is what I was hoping to avoid back when I made this comment here #32579 (comment).
Are there any runtime checks on the reader side? I did a quick look, and it appears "no". We should aim to not have any locks/atomics checking for "is ready only" on the reader side. (Or not have more than one when first opening/mapping the file.)
That makes sense. Let me apply to only the writer side and use the reader instance itself for the read. |
Or have |
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.
Code looks good, some nits/comments to address
…e comments and add assertions.
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.
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143)
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344)
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344) [TieredStorage] Deprecate the use of account-hash in HotStorage (solana-labs#93)
Problem
While accounts-db might not invoke appends_account twice
for the same AccountsFile, TieredStorage::write_accounts()
itself isn't thread-safe, and it depends on the above accounts-db
assumption.
Summary of Changes
This PR makes TieredStorage::write_accounts() thread-safe.
So only the first thread who successfully updates the readonly
flag can proceed and writes the input accounts. All subsequent
calls to write_accounts() will be no-op and return AttemptToUpdateReadOnly
Error.