Skip to content

Disables tar's sparse file handling when archiving a snapshot#5445

Merged
brooksprumo merged 1 commit into
anza-xyz:masterfrom
brooksprumo:snap/no-sparse-tar
Mar 26, 2025
Merged

Disables tar's sparse file handling when archiving a snapshot#5445
brooksprumo merged 1 commit into
anza-xyz:masterfrom
brooksprumo:snap/no-sparse-tar

Conversation

@brooksprumo

@brooksprumo brooksprumo commented Mar 24, 2025

Copy link
Copy Markdown

Problem

Upgrading the tar crate causes intermittent snapshot archiving errors123.

For the recent string of snapshot archiving errors, I looked at the tar crate we use in v2.1 vs v2.0.

In v2.1 we use tar version 0.4.42. In v2.0 we use tar version 0.4.41. Looking through the commits between these versions of the tar crate revealed that this lseek error is new in v0.4.42! And here's the PR that added it: composefs/tar-rs#375

I opened an issue on the tar crate: composefs/tar-rs#403.

Summary of Changes

Disable tar's sparse file handing, since the errors only happen when sparse mode is enabled.

Note, I intend to backport this to v2.1.

Additional Testing

I ran this PR on a mnb node for 18+ hours without issue. I know this isn't a long time, as the full snapshot archiving was only exercised a few times, but it does show it is not immediately broken. I also restarted the node and had it load from its own snapshot archives, and that was successful as well. This means the snapshots created were not broken.

Footnotes

  1. https://discord.com/channels/428295358100013066/689412830075551748/1349323763270946826

  2. https://discord.com/channels/428295358100013066/689412830075551748/1352336706581303336

  3. https://github.com/anza-xyz/agave/issues/4211

@brooksprumo brooksprumo self-assigned this Mar 24, 2025
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.3%. Comparing base (a2af443) to head (45f5fed).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #5445     +/-   ##
=========================================
- Coverage    83.3%    83.3%   -0.1%     
=========================================
  Files         820      820             
  Lines      371809   371829     +20     
=========================================
- Hits       310054   310052      -2     
- Misses      61755    61777     +22     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brooksprumo
brooksprumo marked this pull request as ready for review March 24, 2025 18:56
@brooksprumo
brooksprumo requested a review from HaoranYi March 25, 2025 12:05
@mergify

mergify Bot commented Mar 25, 2025

Copy link
Copy Markdown

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

@mergify

mergify Bot commented Mar 25, 2025

Copy link
Copy Markdown

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@brooksprumo
brooksprumo requested a review from roryharr March 25, 2025 13:24

@HaoranYi HaoranYi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@t-nelson t-nelson left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept and backport ack

did we figure out if this is a bug tar-rs sparse search of nonsparse files or if appendvecs are in fact sparse? if the latter, is this expected?

@brooksprumo

brooksprumo commented Mar 25, 2025

Copy link
Copy Markdown
Author

did we figure out if this is a bug tar-rs sparse search of nonsparse files or if appendvecs are in fact sparse? if the latter, is this expected?

The error isn't when the append vecs are scanned, it's the bank snapshot files (serialized bank, status cache, etc).

And the account data in append vecs can be arbitrary. So could have 10 MiB of zeroes. Each account also has a hash that is all zeros today too. I'm not sure what the minimum size for a "hole" is though.

@brooksprumo

Copy link
Copy Markdown
Author

I got access to an XFS machine, so I'll be testing out there as well.

@brooksprumo

brooksprumo commented Mar 26, 2025

Copy link
Copy Markdown
Author

Original issues may be due to a file system defragmenter service that runs in the background, for example xfs_fsr. More info here: composefs/tar-rs#403 (comment). There may be a fix in the tar crate eventually.

In the mean time, I believe we can safely merge and backport this fix. If/when the tar crate is fixed, we can evaluate if we want to turn back on spare file handling in the tarball.

@roryharr

roryharr commented Mar 26, 2025

Copy link
Copy Markdown

Original issues may be due to a file system defragmenter service that runs in the background, for example xfs_fsr. More info here: alexcrichton/tar-rs#403 (comment). There may be a fix in the tar crate eventually.

In the mean time, I believe we can safely merge and backport this fix. If/when the tar crate is fixed, we can evaluate if we want to turn back on spare file handling in the tarball.

We should also mark them as no defrag for xfs:
https://linux.die.net/man/8/xfs_fsr

Files marked as no-defrag will be skipped. The xfs_io(8) chattr command with the f attribute can be used to set or clear this flag. Files and directories created in a directory with the no-defrag flag will inherit the attribute.

We don't want the file system changing the files underneath us.

brooksprumo added a commit that referenced this pull request Mar 27, 2025
…backport of #5445) (#5506)

Disables tar's sparse file handling when archiving a snapshot (#5445)

(cherry picked from commit b9ed4eb)

Co-authored-by: Brooks <brooks@anza.xyz>
@brooksprumo

Copy link
Copy Markdown
Author

I got access to an XFS machine, so I'll be testing out there as well.

I've been running ledger-tool create-snapshot in a loop (withOUT this patch) for two days, and haven't gotten any errors. It's possible this machine doesn't have any background defragmentation tasks (e.g. xfs_fsr) running, which may explain why I haven't seen any issues.

I've also been running this PR on a separate machine (non XFS) to ensure it takes snapshots properly and it's still running fine (this is the same machine as listed in the PR's description under the Additional Testing section) and metrics related to snapshots look normal.

brooksprumo added a commit that referenced this pull request Mar 27, 2025
…backport of #5445) (#5505)

Disables tar's sparse file handling when archiving a snapshot (#5445)

(cherry picked from commit b9ed4eb)

Co-authored-by: Brooks <brooks@anza.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants