Skip to content
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

layer file creation: remove redundant fsync()s #6983

Conversation

problame
Copy link
Contributor

@problame problame commented Mar 1, 2024

The writer.finish() methods already fsync the inode, using VirtualFile::sync_all().

All that the callers need to do is fsync their directory, i.e., the timeline directory.

Note that there's a call in the new compaction code that is apparently dead-at-runtime, so, I couldn't fix up any fsyncs there Link.

Note that layer durability still matters somewhat, even after #5198 which made remote storage authoritative.
We do have the layer file length as an indicator, but no checksums on the layer file contents.
So, a series of overwrites without fsyncs in the middle, plus a subsequent crash, could cause us to end up in a state where the file length matches but the contents are garbage.

part of #6663

The `writer.finish()` methods already fsync the inode,
using `VirtualFile::sync_all()`.

All that the callers need to do is fsync their directory,
i.e., the timeline directory.

Note that there's a call in the new compaction code that
is apparently dead-at-runtime, so, I couldn't fix up
any fsyncs there [Link](https://github.com/neondatabase/neon/blob/502b69b33bbd4ad1b0647e921a9c665249a2cd62/pageserver/src/tenant/timeline/compaction.rs#L204-L211).

In the grand scheme of things, layer durability probably doesn't
matter anymore because the remote storage is authoritative at all times
as of #5198. But, let's not break the discipline in htis commit.

part of #6663
@problame problame requested a review from koivunej March 1, 2024 11:58
@problame problame requested a review from a team as a code owner March 1, 2024 11:58
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Thanks

@koivunej
Copy link
Member

koivunej commented Mar 1, 2024

In the grand scheme of things, layer durability probably doesn't matter anymore because the remote storage is authoritative at all times as of #5198. But, let's not break the discipline in this commit.

We currently use the length as the only measure of "layer file is intact" so we would have to come with something for this if we were to drop fsyncs.

@problame
Copy link
Contributor Author

problame commented Mar 1, 2024

We currently use the length as the only measure of "layer file is intact" so we would have to come with something for this if we were to drop fsyncs.

Hm, ok, will adjust the PR description.

@problame
Copy link
Contributor Author

problame commented Mar 1, 2024

Erm, wait, no. The layer length mechanism would be sufficient to detect if we crashed after writing out a layer into the kernel page cache but before fsyncing.

Then gain, better to be safe here.

Copy link

github-actions bot commented Mar 1, 2024

2484 tests run: 2362 passed, 0 failed, 122 skipped (full report)


Flaky tests (1)

Postgres 15

  • test_compute_pageserver_connection_stress: debug

Code coverage* (full report)

  • functions: 28.7% (6937 of 24179 functions)
  • lines: 47.2% (42542 of 90143 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
d46f31e at 2024-03-04T11:29:25.829Z :recycle:

…kio-epoll-uring/layer-write-path-fsync-cleanups
@problame problame merged commit 3fd77eb into main Mar 4, 2024
53 checks passed
@problame problame deleted the problame/integrate-tokio-epoll-uring/layer-write-path-fsync-cleanups branch March 4, 2024 11:33
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.

2 participants