fix(pageserver): abort process if fsync fails

[skyzh]

Problem

close #8140

The original issue is rather vague on what we should do. After discussion w/ @problame we decided to narrow down the problems we want to solve in that issue.

• read path -- do not panic for now.
• write path -- panic only on write errors (i.e., device error, fsync error), but not on no-space for now.

The guideline is that if the pageserver behavior could lead to violation of persistent constraints (i.e., return an operation as successful but not actually persisting things), we should panic. Fsync is the place where both of us agree that we should panic, because if fsync fails, the kernel will mark dirty pages as clean, and the next fsync will not necessarily return false. This would make the storage client assume the operation is successful.

Summary of changes

Make fsync panic on fatal errors.

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

5065 tests run: 4907 passed, 0 failed, 158 skipped (full report)

---

Flaky tests (8)

Postgres 17

• test_pageserver_compaction_smoke: release-arm64
• test_subscriber_restart: debug-x86-64
• test_delete_timeline_client_hangup: debug-x86-64
• test_pageserver_lsn_wait_error_safekeeper_stop: release-x86-64

Postgres 15

• test_ondemand_wal_download_in_replication_slot_funcs: release-x86-64
• test_subscriber_restart: release-x86-64

Postgres 14

• test_replica_start_wait_subxids_finish: release-x86-64
• test_subscriber_restart: release-x86-64

Code coverage* (full report)

• functions: 32.0% (7490 of 23395 functions)
• lines: 50.0% (60481 of 120888 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
68a7b13 at 2024-09-27T18:35:22.810Z :recycle:}

[problame]

Quick search for regex \.sync_ in the codebase shows that this PR doesn't cover

• VirtualFile::crashsafe_overwrite
• some usage of tokio::fs

  • neon/pageserver/src/tenant/remote_timeline_client/download.rs

    Lines 185 to 186 in a132323

    	destination_file
    	.sync_all()

  • neon/pageserver/src/tenant/mgr.rs

    Line 222 in 1708743

    fs::File::open(parent).await?.sync_all().await?;

  • neon/pageserver/src/consumption_metrics/disk_cache.rs

    Line 104 in 25a3721

    tempfile.as_file().sync_all()?;

Also, there's the question how to avoid regressing the code base in the future. Can we use clippy to ban tokio::fs::File::sync_all ?

[skyzh]

Also, there's the question how to avoid regressing the code base in the future. Can we use clippy to ban tokio::fs::File::sync_all ?

We still have usage of fsync in safekeeper, and I don't plan to fix them in this pull request for now, so we cannot enforce the clippy lint until we fix all occurrences :(

[problame]

Also, there's the question how to avoid regressing the code base in the future. Can we use clippy to ban tokio::fs::File::sync_all ?

We still have usage of fsync in safekeeper, and I don't plan to fix them in this pull request for now, so we cannot enforce the clippy lint until we fix all occurrences :(

cc @arssher , please think about what safekeepers should do on sync failure / whether it would be better to just abort the process in those cases. Not in this PR though.

[skyzh]

created #9172 for safekeeper