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

scalar: set core.fsync=all for stability #567

Closed

Conversation

derrickstolee
Copy link

The core.fsync config option was introduced in b9f5d03 (core.fsync: documentation and user-friendly aggregate options, 2022-03-15), and first shipped in Git 2.36.0. This config option allows customizing which layers of Git data require fsync().

One issue is that 020406e (core.fsync: introduce granular fsync control infrastructure, 2022-03-10) changed the default behavior of fsync()ing index files (among others) such that users are starting to see issues where their .idx files are empty, halting their interactions with their repositories.

Setting core.fsync=all will use fsync() in all possible places, avoiding this issue in any data written by Git. Here, we are preferring stability over performance. This is left as an optional setting so users can override with their preferred setting, if needed. At that point, they accept the risk of not fsync()ing.

  • This is an early version of work already under review to consider for upstream contribution.

This is something that is affecting users right now, specifically when using the GVFS protocol. The precomputed prefetch packs are getting empty .idx files pretty frequently. This is very likely an fsync() issue, but the frequency is still alarming.

The core.fsync config option was introduced in b9f5d03 (core.fsync:
documentation and user-friendly aggregate options, 2022-03-15), and
first shipped in Git 2.36.0. This config option allows customizing which
layers of Git data require fsync().

One issue is that 020406e (core.fsync: introduce granular fsync
control infrastructure, 2022-03-10) changed the default behavior of
fsync()ing index files (among others) such that users are starting to
see issues where their .idx files are empty, halting their interactions
with their repositories.

Setting core.fsync=all will use fsync() in all possible places, avoiding
this issue in any data written by Git. Here, we are preferring stability
over performance. This is left as an optional setting so users can
override with their preferred setting, if needed. At that point, they
accept the risk of not fsync()ing.
@derrickstolee derrickstolee requested review from dscho and vdye March 15, 2023 19:43
@derrickstolee derrickstolee self-assigned this Mar 15, 2023
@dscho
Copy link
Member

dscho commented Mar 20, 2023

I wonder whether scalar is the best layer for this.

An alternative would be to include FSYNC_COMPONENT_PACK and FSYNC_COMPONENT_INDEX in FSYNC_COMPONENTS_DEFAULT.

@derrickstolee
Copy link
Author

e FSYNC_COMPONENT_PACK and FSYNC_COMPONENT_INDEX

FSYNC_COMPONENT_PACK is already enabled by FSYNC_COMPONENTS_OBJECTS, so maybe we should add FSYNC_COMPONENT_INDEX to FSYNC_COMPONENTS_OBJECTS.

@derrickstolee
Copy link
Author

FSYNC_COMPONENT_INDEX

Actually, FSYNC_COMPONENT_INDEX is for the .git/index file. Instead, FSYNC_COMPONENT_PACK_METADATA is used for pack-indexes. That bit seems to be on for FSYNC_COMPONENTS_DEFAULT.

I wonder if the gvfs-helper process doesn't load these bits at all. I'll start looking into that.

@derrickstolee
Copy link
Author

I wonder if the gvfs-helper process doesn't load these bits at all. I'll start looking into that.

This doesn't seem to be the problem. The bits are stored in the fsync_components global which is initialized to FSYNC_COMPONENTS_DEFAULT in environment.c. It's also set if core.fsync is set in the config by git_default_core_config and gvfs-helper.c does load the default config.

@derrickstolee
Copy link
Author

This is no longer required, as the real problem is identified in #571.

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