Add accounts hard-link files into the bank snapshot directory #29496

xiangzhu70 · 2023-01-03T21:21:36Z

Problem

To construct a bank from the bank snapshot directory, we need to build up the snapshot directory to contain the full state of the snapshot. This is one of the major step, to add the accounts appendvec files into the snapshot as hardlinks.

Because hardlinking only works on the same file system partition, a hardlink directory snapshot/ is created for every slot and every account_path provided. Each <account_path>/snapshot// contains the hardlinks of the account files in run/. The bank snapshot accounts_hardlinks contain symlinks to these directories.

For example, if the accounts_paths vector has the following:
~/ledger/accounts/
/mnt/accounts/

The bank snapshot directory snapshot/ will have an accounts_hardlinks/ directory, which contains symlinks
account_path_0 -> ~/ledger/accounts/snapshot/. # hardlinks of files in ~/ledger/accounts/run
account_path_1 -> /mnt/accounts/snapshot # hardlinks of files in /mnt/accounts/run

This is a split of the PR #28745

Summary of Changes

In add_bank_snapshot, add the hard-links of the accounts/ appendvec files.
Hold the reference counts of the appendvecs for the latest snapshot, to prevent the appendvecs from being recycled. The recycling would change the content of the hardlinked files. For now, a vector of SnapshotStorages is used to hold the reference counts. It could be changed to use option. But some data corruption error happened. Will debug this issue later.
There are also some other small debugging info changes to help debug the appendvec change history.

Fixes #

runtime/src/snapshot_utils.rs

runtime/src/accounts_background_service.rs

runtime/src/accounts_db.rs

runtime/src/append_vec.rs

runtime/src/snapshot_utils.rs

apfitzge

@brooksprumo hit all & more of the things I would have commented on here

runtime/src/bank.rs

runtime/src/accounts_background_service.rs

runtime/src/snapshot_package.rs

runtime/src/snapshot_utils.rs

runtime/src/accounts_background_service.rs

runtime/src/snapshot_utils.rs

apfitzge

Will need to go over this again, but submitting some initial thoughts + nits.

@xiangzhu70 I think one thing that would be really valuable for the PR (and for people looking back without as much context in the future) is if you added an example of what the new snapshot & accounts directories look like in the PR description - preferrably with multiple accounts_dirs.

core/tests/snapshots.rs

core/src/snapshot_packager_service.rs

runtime/src/accounts_background_service.rs

runtime/src/snapshot_utils.rs

runtime/src/accounts_background_service.rs

apfitzge · 2023-02-01T19:48:45Z

runtime/src/accounts_background_service.rs

@@ -141,14 +141,16 @@ pub struct SnapshotRequestHandler {
    pub accounts_package_sender: Sender<AccountsPackage>,
 }

+type AccountStorages = Vec<Arc<AccountStorageEntry>>;
+
 impl SnapshotRequestHandler {
    // Returns the latest requested snapshot slot, if one exists


Comment is out of date; we also return the snapshot storages.

Actually...is this comment even correct either? It returns blockheight, not slot. @brooksprumo

This is not related to the changes in this PR. not sure if I should make this change here.

updated the comment.

Thanks for fixing + updating the comment. I was originally suggesting it should be done here until I noticed the blockheight/slot mismatch - but better to have it fixed now that we've seen it!

runtime/src/snapshot_utils.rs

apfitzge · 2023-02-01T20:28:20Z

runtime/src/snapshot_utils.rs

+/// Hard-link the files from accounts/ to snapshot/<bank_slot>/accounts/
+/// This keeps the appendvec files alive and with the bank snapshot.  The slot and id
+/// in the file names are also updated in case its file is a recycled one with inconsistent slot
+/// and id.


I think I'm missing where are the slot and id in the file names updated

great finding! I had the code which does the updating. Somehow it was lost when making the multi account_path changes. Now I get the code back. Thanks!

runtime/src/snapshot_utils.rs

…t_dirs

brooksprumo

lgtm!

brooksprumo · 2023-02-15T16:48:07Z

heh, do you want me to re-review after I've approved the PR?

xiangzhu70 · 2023-02-15T17:34:04Z

do you want me to re-review after I've approved the PR?

Oh, I didn't notice your approval earlier. No need to re-review again. Many thanks for all the review work!

…-labs#29496) * Add accounts hard-link files into the bank snapshot directory * Small adjustments and fixes. * Address some of the review issues * Fix compilation issues * Change the latest slot snapshot storage from VecDeque to Option * IoWithSourceAndFile and expanded comments on accounts * last_slot_snapshot_storages in return value * Update comments following the review input * rename dir_accounts_hard_links to hard_link_path * Add dir_full_state flag for add_bank_snapshot * Let appendvec files hardlinking work with multiple accounts paths across multiple partitions * Fixes for rebasing * fix tests which generates account_path without adding run/ * rebasing fixes * fix account path test failures * fix test test_concurrent_snapshot_packaging * review comments. renamed the path setup function * Addressed most of the review comments * update with more review comments * handle error from create_accounts_run_and_snapshot_dirs * fix rebasing duplicate * minor accounts_dir path cleanup * minor cleanup, remove commented code * misc review comments * build error fix * Fix test_incremental_snapshot_download_with_crossing_full_snapshot_interval_at_startup * fix build error on MAX_BANK_SNAPSHOTS_TO_RETAIN * rebase fix, update hardlink filename * minor comment spelling fix * rebasing fixes * fix rebase issues; with_extension * comments changes for review * misc minor review issues * bank.fill_bank_with_ticks_for_tests * error handling on appendvec path * fix use_jit * minor comments refining * Remove type AccountStorages * get_account_path_from_appendvec_path return changed to Option * removed appendvec_path.to_path_buf in create_accounts_run_and_snapshot_dirs * add test_get_snapshot_accounts_hardlink_dir * update last_snapshot_storages comment * update last_snapshot_storages comment * symlink map_err * simplify test_get_snapshot_accounts_hardlink_dir with fake paths * log last_snapshot_storages at the end of the loop

xiangzhu70 marked this pull request as ready for review January 4, 2023 04:57

xiangzhu70 requested review from jeffwashington, brooksprumo and apfitzge January 4, 2023 04:57

brooksprumo reviewed Jan 4, 2023

View reviewed changes

apfitzge reviewed Jan 4, 2023

View reviewed changes

brooksprumo reviewed Jan 5, 2023

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

brooksprumo mentioned this pull request Jan 5, 2023

Adds SnapshotError::IoWithSourceAndFile #29527

Merged

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch from 8de9cde to 340f1c9 Compare January 5, 2023 16:15

xiangzhu70 mentioned this pull request Jan 5, 2023

Refine appendvec sanitize error message to include path #29541

Merged

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch from a45da45 to 379df31 Compare January 5, 2023 22:49

brooksprumo self-requested a review January 6, 2023 00:05

brooksprumo reviewed Jan 9, 2023

View reviewed changes

brooksprumo reviewed Jan 11, 2023

View reviewed changes

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch 2 times, most recently from 0194e90 to aa74ca6 Compare January 19, 2023 01:33

xiangzhu70 mentioned this pull request Jan 20, 2023

Add run parent directory for accounts files #29794

Merged

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch from bd94a92 to 196c679 Compare January 25, 2023 01:48

xiangzhu70 requested review from apfitzge and brooksprumo January 25, 2023 07:07

xiangzhu70 mentioned this pull request Jan 25, 2023

Revert "Add run parent directory for accounts files (#29794)" #29899

Merged

apfitzge reviewed Jan 25, 2023

View reviewed changes

brooksprumo reviewed Jan 25, 2023

View reviewed changes

runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved

runtime/src/accounts_background_service.rs Outdated Show resolved Hide resolved

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch 2 times, most recently from d991fe5 to 72868f0 Compare January 31, 2023 18:42

apfitzge reviewed Feb 1, 2023

View reviewed changes

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch 3 times, most recently from 36f26b0 to 3b2dc90 Compare February 2, 2023 16:24

xiangzhu70 requested a review from brooksprumo February 2, 2023 19:06

xiangzhu70 added 17 commits February 15, 2023 07:56

rebasing fixes

822d365

fix rebase issues; with_extension

9a23695

comments changes for review

b74a906

misc minor review issues

216d3fe

bank.fill_bank_with_ticks_for_tests

6e8fa77

error handling on appendvec path

7e4347d

fix use_jit

857398f

minor comments refining

ba92b43

Remove type AccountStorages

47524f3

get_account_path_from_appendvec_path return changed to Option

5546504

removed appendvec_path.to_path_buf in create_accounts_run_and_snapsho…

8ba8652

…t_dirs

add test_get_snapshot_accounts_hardlink_dir

5bf5f49

update last_snapshot_storages comment

c7e4285

update last_snapshot_storages comment

719f3ec

symlink map_err

257a9e8

simplify test_get_snapshot_accounts_hardlink_dir with fake paths

38dda74

log last_snapshot_storages at the end of the loop

5799adf

xiangzhu70 force-pushed the add_accounts_to_bank_snapshot_dir branch from d1478f9 to 5799adf Compare February 15, 2023 15:58

brooksprumo self-requested a review February 15, 2023 16:38

brooksprumo approved these changes Feb 15, 2023

View reviewed changes

xiangzhu70 requested review from brooksprumo and apfitzge February 15, 2023 16:45

xiangzhu70 merged commit 4909267 into solana-labs:master Feb 15, 2023

xiangzhu70 mentioned this pull request Feb 22, 2023

Clean up old account snapshot directories to avoid the file existing hardlink error #30426

Merged

xiangzhu70 mentioned this pull request Mar 9, 2023

Clean orphaned account snapshot dirs #30645

Merged

apfitzge mentioned this pull request Jun 12, 2023

Bugfix: Account Shrink Paths must conform to account directory structure #32088

Merged

brooksprumo mentioned this pull request Jun 13, 2023

Removes unnecessary call to purge_old_bank_snapshots() in local-cluster test #32111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add accounts hard-link files into the bank snapshot directory #29496

Add accounts hard-link files into the bank snapshot directory #29496

xiangzhu70 commented Jan 3, 2023 •

edited

apfitzge left a comment

apfitzge left a comment

apfitzge Feb 1, 2023

apfitzge Feb 1, 2023

xiangzhu70 Feb 2, 2023

xiangzhu70 Feb 2, 2023

apfitzge Feb 3, 2023

apfitzge Feb 1, 2023

xiangzhu70 Feb 2, 2023

brooksprumo left a comment

brooksprumo commented Feb 15, 2023

xiangzhu70 commented Feb 15, 2023

Add accounts hard-link files into the bank snapshot directory #29496

Add accounts hard-link files into the bank snapshot directory #29496

Conversation

xiangzhu70 commented Jan 3, 2023 • edited

Problem

Summary of Changes

apfitzge left a comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

apfitzge Feb 1, 2023

Choose a reason for hiding this comment

apfitzge Feb 1, 2023

Choose a reason for hiding this comment

xiangzhu70 Feb 2, 2023

Choose a reason for hiding this comment

xiangzhu70 Feb 2, 2023

Choose a reason for hiding this comment

apfitzge Feb 3, 2023

Choose a reason for hiding this comment

apfitzge Feb 1, 2023

Choose a reason for hiding this comment

xiangzhu70 Feb 2, 2023

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo commented Feb 15, 2023

xiangzhu70 commented Feb 15, 2023

xiangzhu70 commented Jan 3, 2023 •

edited