[v4,2/4] core.fsync: introduce granular fsync control

Commit Message

Neeraj Singh (WINDOWS-SFS) Feb. 1, 2022, 3:33 a.m. UTC

From: Neeraj Singh <[e-mail]>

This commit introduces the `core.fsync` configuration
knob which can be used to control how components of the
repository are made durable on disk.

This setting allows future extensibility of the list of
syncable components:
* We issue a warning rather than an error for unrecognized
  components, so new configs can be used with old Git versions.
* We support negation, so users can choose one of the default
  aggregate options and then remove components that they don't
  want. The user would then harden any new components added in
  a Git version update.

This also supports the common request of doing absolutely no
fysncing with the `core.fsync=none` value, which is expected
to make the test suite faster.

Signed-off-by: Neeraj Singh <[e-mail]>
---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        | 24 +++++++----
 bulk-checkin.c                |  5 ++-
 cache.h                       | 41 ++++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  2 +-
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 177 insertions(+), 39 deletions(-)

Comments

Junio C Hamano Feb. 2, 2022, 12:51 a.m. UTC | #1

"Neeraj Singh via GitGitGadget" <[e-mail]> writes:

> +core.fsync::
> +	A comma-separated list of parts of the repository which should be
> +	hardened via the core.fsyncMethod when created or modified. You can
> +	disable hardening of any component by prefixing it with a '-'. Later
> +	items take precedence over earlier ones in the list. For example,
> +	`core.fsync=all,-pack-metadata` means "harden everything except pack
> +	metadata." Items that are not hardened may be lost in the event of an
> +	unclean system shutdown.
> ++
> +* `none` disables fsync completely. This must be specified alone.
> +* `loose-object` hardens objects added to the repo in loose-object form.
> +* `pack` hardens objects added to the repo in packfile form.
> +* `pack-metadata` hardens packfile bitmaps and indexes.
> +* `commit-graph` hardens the commit graph file.
> +* `objects` is an aggregate option that includes `loose-objects`, `pack`,
> +  `pack-metadata`, and `commit-graph`.
> +* `default` is an aggregate option that is equivalent to `objects,-loose-object`
> +* `all` is an aggregate option that syncs all individual components above.

I am not quite sure if this is way too complex (e.g. what does it
mean that we do not care much about loose-object safety while we do
care about commit-graph files?) and at the same time it is too
limited (e.g. if it makes sense to say a class of items deserve more
protection than another class of items, don't we want to be able to
say "class X is ultra-precious so use method A on them, while class
Y is mildly precious and use method B on them, everything else are
not that important and doing the default thing is just fine").

If we wanted to allow the "matrix" kind of flexibility, I think the
way to do so would be

	fsync.<class>.method = <value>

e.g.

	[fsync "default"] method = none
	[fsync "loose-object"] method = fsync
	[fsync "pack-metadata"] method = writeout-only

Where do we expect users to take the core.fsync settings from?  Per
repository?  If it is from per user (i.e. $HOME/.gitconfig), do
people tend to share it across systems (not necessarily over NFS)
with the same contents?  If so, I am not sure if fsync.method that
is way too close to the actual "implementation" is a good idea to
begin with.  From end-user's point of view, it may be easier to
express "class X is ultra-precious, and class Y and Z are mildly
so", with something like fsync.<class>.level = <how-precious> and
let the Git implementation on each platform choose the appropriate
fsync method to protect the stuff at that precious-ness.

Thanks.

Junio C Hamano Feb. 2, 2022, 1:42 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> I am not quite sure if this is way too complex (e.g. what does it
> mean that we do not care much about loose-object safety while we do
> care about commit-graph files?) and at the same time it is too
> limited (e.g. if it makes sense to say a class of items deserve more
> protection than another class of items, don't we want to be able to
> say "class X is ultra-precious so use method A on them, while class
> Y is mildly precious and use method B on them, everything else are
> not that important and doing the default thing is just fine").
>
> If we wanted to allow the "matrix" kind of flexibility,...

To continue with the thinking aloud...

Sometimes configuration flexibility is truly needed, but often it is
just a sign of designer being lazy and not thinking it through as an
end-user facing problem.  In other words, "I am giving enough knobs
to you, so it is up to you to express your policy in whatever way
you want with the knobs provided" is a very irresponsible thing to
tell end-users.

And this one smells like the case of a lazy design.

It may be that it makes sense in some workflows to protect
commit-graph files less than object files and pack.idx files can be
corrupted as long as pack.pack files are adequately protected
because the former can be recomputed from the latter, but in no
workflows, the reverse would be true.  Yet the design gives such
needless flexibility, which makes it hard for lay end-users to
choose the best combination and allows them to protect .idx files
more than .pack files by mistake, for example.

I am wondering if the classification itself introduced by this step
actually can form a natural and linear progression of safe-ness.  By
default, we'd want _all_ classes of things to be equally safe, but
at one level down, there is "protect things that are not
recomputable, but recomputable things can be left to the system"
level, and there would be even riskier "protect packs as it would
hurt a _lot_ to lose them, but losing loose ones will typically lose
only the most recent work, and they are less valuable" level.

If we, as the Git experts, spend extra brain cycles to come up with
an easy to understand spectrum of performance vs durability
trade-off, end-users won't have to learn the full flexibility and
easily take the advice from experts.  They just need to say what
level of durability they want (or how much durability they can risk
in exchange for an additional throughput), and leave the rest to us.

On the core.fsyncMethod side, the same suggestion applies.

Once we know the desired level of performance vs durability
trade-off from the user, we, as the impolementors, should know the
best method, for each class of items, to achieve that durability on
each platform when writing it to the storage, without exposing the
low level details of the implementation that only the Git folks need
to be aware of.

So, from the end-user UI perspective, I'd very much prefer if we can
just come up with a single scalar variable, (say "fsync.durability"
that ranges from "conservative" to "performance") that lets our
users express the level of durability desired.  The combination of
core.fsyncMethod and core.fsync are one variable too many, and the
latter being a variable that takes a list of things as its value
makes it even worse to sell to the end users.

Neeraj Singh Feb. 11, 2022, 8:38 p.m. UTC | #3

Apologies in advance for the delayed reply.  I've finally been able to
return to Git after an absence.

On Tue, Feb 1, 2022 at 4:51 PM Junio C Hamano <[e-mail]> wrote:
>
> "Neeraj Singh via GitGitGadget" <[e-mail]> writes:
>
> > +core.fsync::
> > +     A comma-separated list of parts of the repository which should be
> > +     hardened via the core.fsyncMethod when created or modified. You can
> > +     disable hardening of any component by prefixing it with a '-'. Later
> > +     items take precedence over earlier ones in the list. For example,
> > +     `core.fsync=all,-pack-metadata` means "harden everything except pack
> > +     metadata." Items that are not hardened may be lost in the event of an
> > +     unclean system shutdown.
> > ++
> > +* `none` disables fsync completely. This must be specified alone.
> > +* `loose-object` hardens objects added to the repo in loose-object form.
> > +* `pack` hardens objects added to the repo in packfile form.
> > +* `pack-metadata` hardens packfile bitmaps and indexes.
> > +* `commit-graph` hardens the commit graph file.
> > +* `objects` is an aggregate option that includes `loose-objects`, `pack`,
> > +  `pack-metadata`, and `commit-graph`.
> > +* `default` is an aggregate option that is equivalent to `objects,-loose-object`
> > +* `all` is an aggregate option that syncs all individual components above.
>
> I am not quite sure if this is way too complex (e.g. what does it
> mean that we do not care much about loose-object safety while we do
> care about commit-graph files?) and at the same time it is too
> limited (e.g. if it makes sense to say a class of items deserve more
> protection than another class of items, don't we want to be able to
> say "class X is ultra-precious so use method A on them, while class
> Y is mildly precious and use method B on them, everything else are
> not that important and doing the default thing is just fine").
>
> If we wanted to allow the "matrix" kind of flexibility, I think the
> way to do so would be
>
>         fsync.<class>.method = <value>
>
> e.g.
>
>         [fsync "default"] method = none
>         [fsync "loose-object"] method = fsync
>         [fsync "pack-metadata"] method = writeout-only
>

I don't believe it makes sense to offer a full matrix of what to fsync
and what method to use, since the method is a property of the
filesystem and OS the repo is running on, while the list of things to
fsync is more a selection of what the user values. So if I'm hosting
on APFS on macOS or NTFS on Windows, I'd want to set the fsyncMethod
to batch so that I can get good performance at the safety level I
choose.  If I'm working on my maintainer repo, I'd maybe not want to
fsync anything, but I'd want to fsync everything when working on my
developer repo.

> Where do we expect users to take the core.fsync settings from?  Per
> repository?  If it is from per user (i.e. $HOME/.gitconfig), do
> people tend to share it across systems (not necessarily over NFS)
> with the same contents?  If so, I am not sure if fsync.method that
> is way too close to the actual "implementation" is a good idea to
> begin with.  From end-user's point of view, it may be easier to
> express "class X is ultra-precious, and class Y and Z are mildly
> so", with something like fsync.<class>.level = <how-precious> and
> let the Git implementation on each platform choose the appropriate
> fsync method to protect the stuff at that precious-ness.
>

I expect the vast majority of users to have whatever setting is baked
into their build of Git.  For the users that want to do something
different, I expect them to have core.fsyncMethod and core.fsync
configured per-user for the majority of their repos. Some repos might
have custom settings that override the per-user settings: 1) Ephemeral
repos that don't contain unique data would probably want to set
core.fsync=none. 2) Repos hosting on NFS or on a different FS may have
a stricter core.fsyncmethod setting.

(More more text to follow in reply to your next email).

Neeraj Singh Feb. 11, 2022, 9:18 p.m. UTC | #4

On Tue, Feb 1, 2022 at 5:42 PM Junio C Hamano <[e-mail]> wrote:
>
> Junio C Hamano <[e-mail]> writes:
>
> > I am not quite sure if this is way too complex (e.g. what does it
> > mean that we do not care much about loose-object safety while we do
> > care about commit-graph files?) and at the same time it is too
> > limited (e.g. if it makes sense to say a class of items deserve more
> > protection than another class of items, don't we want to be able to
> > say "class X is ultra-precious so use method A on them, while class
> > Y is mildly precious and use method B on them, everything else are
> > not that important and doing the default thing is just fine").
> >
> > If we wanted to allow the "matrix" kind of flexibility,...
>
> To continue with the thinking aloud...
>
> Sometimes configuration flexibility is truly needed, but often it is
> just a sign of designer being lazy and not thinking it through as an
> end-user facing problem.  In other words, "I am giving enough knobs
> to you, so it is up to you to express your policy in whatever way
> you want with the knobs provided" is a very irresponsible thing to
> tell end-users.
>
> And this one smells like the case of a lazy design.
>
> It may be that it makes sense in some workflows to protect
> commit-graph files less than object files and pack.idx files can be
> corrupted as long as pack.pack files are adequately protected
> because the former can be recomputed from the latter, but in no
> workflows, the reverse would be true.  Yet the design gives such
> needless flexibility, which makes it hard for lay end-users to
> choose the best combination and allows them to protect .idx files
> more than .pack files by mistake, for example.
>
> I am wondering if the classification itself introduced by this step
> actually can form a natural and linear progression of safe-ness.  By
> default, we'd want _all_ classes of things to be equally safe, but
> at one level down, there is "protect things that are not
> recomputable, but recomputable things can be left to the system"
> level, and there would be even riskier "protect packs as it would
> hurt a _lot_ to lose them, but losing loose ones will typically lose
> only the most recent work, and they are less valuable" level.
>
> If we, as the Git experts, spend extra brain cycles to come up with
> an easy to understand spectrum of performance vs durability
> trade-off, end-users won't have to learn the full flexibility and
> easily take the advice from experts.  They just need to say what
> level of durability they want (or how much durability they can risk
> in exchange for an additional throughput), and leave the rest to us.
>
> On the core.fsyncMethod side, the same suggestion applies.
>
> Once we know the desired level of performance vs durability
> trade-off from the user, we, as the impolementors, should know the
> best method, for each class of items, to achieve that durability on
> each platform when writing it to the storage, without exposing the
> low level details of the implementation that only the Git folks need
> to be aware of.
>
> So, from the end-user UI perspective, I'd very much prefer if we can
> just come up with a single scalar variable, (say "fsync.durability"
> that ranges from "conservative" to "performance") that lets our
> users express the level of durability desired.  The combination of
> core.fsyncMethod and core.fsync are one variable too many, and the
> latter being a variable that takes a list of things as its value
> makes it even worse to sell to the end users.

I see the value in simplifying the core.fsync configuration to a
single scalar knob of preciousness. The main motivation for this more
granular scheme is that I didn't think the current configuration
follows a sensible principle. We should be fsyncing the loose objects,
index, refs, and config files in addition to what we're already
syncing today.  On macOS, we should be doing a full hardware flush if
we've said we want to fsync.  But you expressed the notion in [1] that
we don't want to degrade the performance of the vast majority of users
who are happy with the current "unprincipled but mostly works"
configuration.  I agree with that sentiment, but it leads to a design
where we can express the current configuration, which does not follow
a scalar hierarchy.

The aggregate core.fsync options are meant to provide a way for us to
recommend a sensible configuration to the user without having to get
into the intricacies of repo layout. Maybe we can define and document
aggregate options that make sense in terms of a scalar level of
preciousness.

One reason to keep core.fsyncMethod separate from the core.fsync knob
is that it's more a property of the system and FS the repo is on,
rather than the user's value of the repo.  We could try to auto-detect
some known filesystems that might support batch mode using 'statfs',
but having a table like that in Git would go out of date over time.

Please let me know what you think about these justifications for the
current design.  I'd be happy to make a change if the current
constraint of "keep the default config the same" can be relaxed in
some way.  I'd also be happy to go back to some variation of
expressing 'core.fsyncObjectFiles = batch' and leaving the rest of
fsync story alone.

Thanks,
Neeraj

[1] https://lore.kernel.org/git/xmqqtuilyfls.fsf@gitster.g/

Junio C Hamano Feb. 11, 2022, 10:19 p.m. UTC | #5

Neeraj Singh <[e-mail]> writes:

> The main motivation for this more granular scheme is that I didn't
> think the current configuration follows a sensible principle. We
> should be fsyncing the loose objects, index, refs, and config
> files in addition to what we're already syncing today.  On macOS,
> we should be doing a full hardware flush if we've said we want to
> fsync.

If the "robustness vs performance" trade-off is unevenly made in the
current code, then that is a very good problem to address first, and
such a change is very much justified on its own.

Perhaps "this is not a primary work repository but is used only to
follow external site to build, hence no fsync is fine" folks, who do
not have core.fsyncObjectFiles set to true, may appreciate if we
stopped doing fsync for packs and other things.  As the Boolean
core.fsyncObjectFiles is the only end-user visible knob to express
how the end-users express the trade-off, a good first step would be
to align other file accesses to the preference expressed by it, i.e.
others who say they want fsync in the current scheme would
appreciate if we start fsync in places like ref-files backend.

Making the choice more granular, from Boolean "yes/no", to linear
levels, would also be a good idea.  Doing both at the same time may
make it harder to explain and justify, but as long as at the end, if
"very performant" choice uniformly does not do any fsync while
"ultra durable" choice makes a uniform effort across subsystems to
make sure bits hit the platter, it would be a very good idea to do
them.

Thanks.

Neeraj Singh Feb. 11, 2022, 11:04 p.m. UTC | #6

On Fri, Feb 11, 2022 at 2:19 PM Junio C Hamano <[e-mail]> wrote:
>
> Neeraj Singh <[e-mail]> writes:
>
> > The main motivation for this more granular scheme is that I didn't
> > think the current configuration follows a sensible principle. We
> > should be fsyncing the loose objects, index, refs, and config
> > files in addition to what we're already syncing today.  On macOS,
> > we should be doing a full hardware flush if we've said we want to
> > fsync.
>
> If the "robustness vs performance" trade-off is unevenly made in the
> current code, then that is a very good problem to address first, and
> such a change is very much justified on its own.
>
> Perhaps "this is not a primary work repository but is used only to
> follow external site to build, hence no fsync is fine" folks, who do
> not have core.fsyncObjectFiles set to true, may appreciate if we
> stopped doing fsync for packs and other things.  As the Boolean
> core.fsyncObjectFiles is the only end-user visible knob to express
> how the end-users express the trade-off, a good first step would be
> to align other file accesses to the preference expressed by it, i.e.
> others who say they want fsync in the current scheme would
> appreciate if we start fsync in places like ref-files backend.
>

In practice, almost all users have core.fsyncObjectFiles set to the
platform default, which is 'false' everywhere besides Windows.  So at
minimum, we have to take default to mean that we maintain behavior no
weaker than the current version of Git, otherwise users will start
losing their data. The advantage of introducing a new knob is that we
don't have to try to divine why the user set a particular value of
core.fsyncObjectFiles.

> Making the choice more granular, from Boolean "yes/no", to linear
> levels, would also be a good idea.  Doing both at the same time may
> make it harder to explain and justify, but as long as at the end, if
> "very performant" choice uniformly does not do any fsync while
> "ultra durable" choice makes a uniform effort across subsystems to
> make sure bits hit the platter, it would be a very good idea to do
> them.
>

One path to get to your suggestion from the current patch series would
be to remove the component-specific options and only provide aggregate
options.  Alternatively, we could just not document the
component-specific options and leave them available to be people who
read source code. So if I rename the aggregate options in terms of
'levels of durability', and only document those, would that be
acceptable?

Thanks for the review!
-Neeraj

Junio C Hamano Feb. 11, 2022, 11:15 p.m. UTC | #7

Neeraj Singh <[e-mail]> writes:

> In practice, almost all users have core.fsyncObjectFiles set to the
> platform default, which is 'false' everywhere besides Windows.  So at
> minimum, we have to take default to mean that we maintain behavior no
> weaker than the current version of Git, otherwise users will start
> losing their data.

Would they?   If they think platform default is performant and safe
enough for their use, as long as our adjustment is out outrageously
more dangerous or less performant, I do not think "no weaker than"
is a strict requirement.  If we were overly conservative in some
areas than the "platform default", making it less conservative in
those areas to match the looseness of other areas should be OK and
vice versa.

> One path to get to your suggestion from the current patch series would
> be to remove the component-specific options and only provide aggregate
> options.  Alternatively, we could just not document the
> component-specific options and leave them available to be people who
> read source code. So if I rename the aggregate options in terms of
> 'levels of durability', and only document those, would that be
> acceptable?

In any case, if others who reviewed the series in the past are happy
with the "two knobs" approach and are willing to jump in to help new
users who will be confused with one knob too many, I actually am OK
with the series that I called "overly complex".  Let me let them
weigh in before I can answer that question.

Thanks.

Randall S. Becker Feb. 12, 2022, 12:39 a.m. UTC | #8

On February 11, 2022 6:15 PM, Junio C Hamano wrote:
> Neeraj Singh <[e-mail]> writes:
> 
> > In practice, almost all users have core.fsyncObjectFiles set to the
> > platform default, which is 'false' everywhere besides Windows.  So at
> > minimum, we have to take default to mean that we maintain behavior no
> > weaker than the current version of Git, otherwise users will start
> > losing their data.
> 
> Would they?   If they think platform default is performant and safe
> enough for their use, as long as our adjustment is out outrageously more
> dangerous or less performant, I do not think "no weaker than"
> is a strict requirement.  If we were overly conservative in some areas than the
> "platform default", making it less conservative in those areas to match the
> looseness of other areas should be OK and vice versa.
> 
> > One path to get to your suggestion from the current patch series would
> > be to remove the component-specific options and only provide aggregate
> > options.  Alternatively, we could just not document the
> > component-specific options and leave them available to be people who
> > read source code. So if I rename the aggregate options in terms of
> > 'levels of durability', and only document those, would that be
> > acceptable?
> 
> In any case, if others who reviewed the series in the past are happy with the
> "two knobs" approach and are willing to jump in to help new users who will be
> confused with one knob too many, I actually am OK with the series that I called
> "overly complex".  Let me let them weigh in before I can answer that question.

On behalf of those who are likely to set fsync to true in all cases, because a SIGSEGV or some other early abort will cause changes to be lost, I am not happy with excessive knobs, as we will have to ensure that the are all set to "write this out to disk as quickly as soon as possible or else". I end up having to teach large numbers of people about these settings and think that excessive controls in this area are overwhelmingly bad. This is not a "windows vs. everything else" situation. Not all platforms write buffered by default. fwrite and write behave different on NonStop (and other POSIXy things, and some variants are fully virtualized, so who knows what the hypervisor will do - it's bad enough that there is even an option to tell the hypervisor to keep things in memory until convenient to write even on RAID 0/1 emulation). Teaching people how to use the knobs correctly is going to be a challenge. For me, I'm always willing to sacrifice performance for reliability - 100% of the time. I'm sure this whole series is going to be problematic. I'm sorry but that's my position on it. The default position of all knobs must be to force the write and keeping it simple so that variations on knob settings give different results is not going to end well.

Sincerely,
Randall

Patrick Steinhardt Feb. 14, 2022, 7:04 a.m. UTC | #9

On Fri, Feb 11, 2022 at 03:15:20PM -0800, Junio C Hamano wrote:
> Neeraj Singh <[e-mail]> writes:
> 
> > In practice, almost all users have core.fsyncObjectFiles set to the
> > platform default, which is 'false' everywhere besides Windows.  So at
> > minimum, we have to take default to mean that we maintain behavior no
> > weaker than the current version of Git, otherwise users will start
> > losing their data.
> 
> Would they?   If they think platform default is performant and safe
> enough for their use, as long as our adjustment is out outrageously
> more dangerous or less performant, I do not think "no weaker than"
> is a strict requirement.  If we were overly conservative in some
> areas than the "platform default", making it less conservative in
> those areas to match the looseness of other areas should be OK and
> vice versa.
> 
> > One path to get to your suggestion from the current patch series would
> > be to remove the component-specific options and only provide aggregate
> > options.  Alternatively, we could just not document the
> > component-specific options and leave them available to be people who
> > read source code. So if I rename the aggregate options in terms of
> > 'levels of durability', and only document those, would that be
> > acceptable?
> 
> In any case, if others who reviewed the series in the past are happy
> with the "two knobs" approach and are willing to jump in to help new
> users who will be confused with one knob too many, I actually am OK
> with the series that I called "overly complex".  Let me let them
> weigh in before I can answer that question.
> 
> Thanks.

I wonder whether it makes sense to distinguish client- and server-side
requirements. While we probably want to "do the right thing" on the
client-side by default so that we don't have to teach users that "We may
use data in some cases on some systems, but not in other cases on other
systems by default." On the server-side folks who implement the Git
hosting are typically a lot more knowledgeable with regards to how Git
behaves, and it can be expected of them to dig a lot deeper than we can
and should reasonably expect from a user.

One point I really care about and that I think is extremely important in
the context of both client and server is data consistency. While it is
bad to lose data, the reality is that it can simply happen when systems
hit exceptional cases like a hard-reset. But what we really must not let
happen is that as a result, a repository gets corrupted because of this
hard reset. In the context of client-side repositories a user will be
left to wonder how to fix the repository, while on the server-side we
need to go in and somehow repair the repository fast or otherwise users
will come to us and complain their repository stopped working.

Our current defaults can end up with repository corruption though. We
don't sync object files before renaming them into place by default, and
furthermore we don't even give a knob to fsync loose references before
renaming them. On gitlab.com we enable "core.fsyncObjectFiles" and thus
to the best of my knowledge never hit a corrupted ODB until now. But
what we do regularly hit is corrupted references because there is no
knob to fsync them.

Furthermore, I really think that the advice in git-config(1) with
regards to loose syncing loose objects that "This is a total waste of
time and effort" is wrong. Filesystem developers have repeatedly stated
that for proper atomicity guarantees, a file must be synced to disk
before renaming it into place [1][2][3]. So from the perspective of the
people who write Linux-based filesystems our application is "broken"
right now, even though there are mechanisms in place which try to work
around our brokenness by using heuristics to detect what we're doing.

To summarize my take: while the degree of durability may be something
that's up for discussions, I think that the current defaults for
atomicity are bad for users because they can and do lead to repository
corruption.

Patrick

[1]: https://thunk.org/tytso/blog/2009/03/15/dont-fear-the-fsync/
[2]: https://btrfs.wiki.kernel.org/index.php/FAQ (What are the crash guarantees of overwrite-by-rename)
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/ext4.rst (see auto_da_alloc)

Junio C Hamano Feb. 14, 2022, 5:17 p.m. UTC | #10

Patrick Steinhardt <[e-mail]> writes:

> To summarize my take: while the degree of durability may be something
> that's up for discussions, I think that the current defaults for
> atomicity are bad for users because they can and do lead to repository
> corruption.

Good summary.

If the user cares about fsynching loose object files in the right
way, we shouldn't leave loose ref files not following the safe
safety level, regardless of how this new core.fsync knobs would look
like.

I think we three are in agreement on that.

Patrick Steinhardt March 9, 2022, 1:42 p.m. UTC | #11

On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> > To summarize my take: while the degree of durability may be something
> > that's up for discussions, I think that the current defaults for
> > atomicity are bad for users because they can and do lead to repository
> > corruption.
> 
> Good summary.
> 
> If the user cares about fsynching loose object files in the right
> way, we shouldn't leave loose ref files not following the safe
> safety level, regardless of how this new core.fsync knobs would look
> like.
> 
> I think we three are in agreement on that.

Is there anything I can specifically do to help out with this topic? We
have again hit data loss in production because we don't sync loose refs
to disk before renaming them into place, so I'd really love to sort out
this issue somehow so that I can revive my patch series which fixes the
known repository corruption [1].

Alternatively, can we maybe find a way forward with applying a version
of my patch series without first settling the bigger question of how we
want the overall design to look like? In my opinion repository
corruption is a severe bug that needs to be fixed, and it doesn't feel
sensible to block such a fix over a discussion that potentially will
take a long time to settle.

Patrick

[1]: http://public-inbox.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason March 9, 2022, 6:50 p.m. UTC | #12

On Wed, Mar 09 2022, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <[e-mail]> writes:
>> 
>> > To summarize my take: while the degree of durability may be something
>> > that's up for discussions, I think that the current defaults for
>> > atomicity are bad for users because they can and do lead to repository
>> > corruption.
>> 
>> Good summary.
>> 
>> If the user cares about fsynching loose object files in the right
>> way, we shouldn't leave loose ref files not following the safe
>> safety level, regardless of how this new core.fsync knobs would look
>> like.
>> 
>> I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].
>
> Alternatively, can we maybe find a way forward with applying a version
> of my patch series without first settling the bigger question of how we
> want the overall design to look like? In my opinion repository
> corruption is a severe bug that needs to be fixed, and it doesn't feel
> sensible to block such a fix over a discussion that potentially will
> take a long time to settle.
>
> Patrick
>
> [1]: http://public-inbox.org/git/[e-mail]/

I share that view. I was wondering how this topic fizzled out the other
day, but then promptly forgot about it.

I think the best thing at this point (hint hint!) would be for someone
in the know to (re-)submit the various patches appropriate to move this
forward. Whether that's just this series, part of it, or some/both of
those + patches from you and Eric and this point I don't know/remember.

But just to be explicitly clear, as probably the person most responsible
for pushing this towards the "bigger question of [...] overall
design".

I just wanted to facilitate a discussion that would result in the
various stakeholders who wanted to add some fsync-related config coming
up with something that's mutually compatible, and I think the design
from Neeraj in this series fits that purpose, is Good Enough etc.

I.e. the actually important and IMO blockers were all resolved, e.g. not
having an fsync configuration that older git versions would needlessly
die on, and not painting ourselves into a corner where
e.g. core.fsync=false or something was squatted on by something other
than a "no fsync, whatsoever" etc.

(But I haven't looked at it again just now, so...)

Anyway, just trying to be explicit that to whatever extent this was held
up by questions/comments of mine I'm very happy to see this go forward.
As you (basically) say we shouldn't lose sight of ongoing data loss in
this area because of some config bikeshedding :)

Junio C Hamano March 9, 2022, 8:03 p.m. UTC | #13

Patrick Steinhardt <[e-mail]> writes:

>> If the user cares about fsynching loose object files in the right
>> way, we shouldn't leave loose ref files not following the safe
>> safety level, regardless of how this new core.fsync knobs would look
>> like.
>> 
>> I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].

How about doing a series to unconditionally sync loose ref creation
and modification?

Alternatively, we could link it to the existing configuration to
control synching of object files.

I do not think core.fsyncObjectFiles having "object" in its name is
a good reason not to think those who set it to true only care about
the loose object files and nothing else.  It is more sensible to
consider that those who set it to true cares about the repository
integrity more than those who set it to false, I would think.

But that (i.e. doing it conditionally and choose which knob to use)
is one extra thing that needs justification, so starting from
unconditional fsync_or_die() may be the best way to ease it in.

Neeraj Singh March 9, 2022, 8:05 p.m. UTC | #14

On Wed, Mar 9, 2022 at 5:42 AM Patrick Steinhardt <[e-mail]> wrote:
>
> On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <[e-mail]> writes:
> >
> > > To summarize my take: while the degree of durability may be something
> > > that's up for discussions, I think that the current defaults for
> > > atomicity are bad for users because they can and do lead to repository
> > > corruption.
> >
> > Good summary.
> >
> > If the user cares about fsynching loose object files in the right
> > way, we shouldn't leave loose ref files not following the safe
> > safety level, regardless of how this new core.fsync knobs would look
> > like.
> >
> > I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].
>
> Alternatively, can we maybe find a way forward with applying a version
> of my patch series without first settling the bigger question of how we
> want the overall design to look like? In my opinion repository
> corruption is a severe bug that needs to be fixed, and it doesn't feel
> sensible to block such a fix over a discussion that potentially will
> take a long time to settle.
>
> Patrick
>
> [1]: http://public-inbox.org/git/[e-mail]/

Hi Patrick,
Thanks for reviving this discussion.  I've updated the PR on
GitGitGadget with a rebase
onto the current 'main' branch and some minor build fixes. I've also
revamped the aggregate options
and documentation to be more inline with Junio's suggestion of having
'levels of safety' that we steer
the user towards. I'm still keeping the detailed options, but
hopefully the guidance is clear enough to
avoid confusion.

I'd be happy to make any point fixes as necessary to get that branch
into proper shape for
upstream, if we've gotten to the point where we don't want to change
the fundamental design.

I agree with Patrick that the detailed knobs are primarily for use by
hosters like GitLab and GitHub.

Please expect a v5 today.

Thanks,
Neeraj

Patrick Steinhardt March 10, 2022, 12:33 p.m. UTC | #15

On Wed, Mar 09, 2022 at 12:03:11PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> >> If the user cares about fsynching loose object files in the right
> >> way, we shouldn't leave loose ref files not following the safe
> >> safety level, regardless of how this new core.fsync knobs would look
> >> like.
> >> 
> >> I think we three are in agreement on that.
> >
> > Is there anything I can specifically do to help out with this topic? We
> > have again hit data loss in production because we don't sync loose refs
> > to disk before renaming them into place, so I'd really love to sort out
> > this issue somehow so that I can revive my patch series which fixes the
> > known repository corruption [1].
> 
> How about doing a series to unconditionally sync loose ref creation
> and modification?
> 
> Alternatively, we could link it to the existing configuration to
> control synching of object files.
> 
> I do not think core.fsyncObjectFiles having "object" in its name is
> a good reason not to think those who set it to true only care about
> the loose object files and nothing else.  It is more sensible to
> consider that those who set it to true cares about the repository
> integrity more than those who set it to false, I would think.
> 
> But that (i.e. doing it conditionally and choose which knob to use)
> is one extra thing that needs justification, so starting from
> unconditional fsync_or_die() may be the best way to ease it in.

I'd be happy to revive my old patch series, but this kind of depends on
where we're standing with the other patch series by Neeraj. If you say
that we'll likely not land his patch series for the upcoming release,
but a small patch series which only starts to sync loose refs may have a
chance, then I'd like to go down that path as a stop-gap solution.
Otherwise it probably wouldn't make a lot of sense.

Patrick

Junio C Hamano March 10, 2022, 5:15 p.m. UTC | #16

Patrick Steinhardt <[e-mail]> writes:

>> How about doing a series to unconditionally sync loose ref creation
>> and modification?
>> 
>> Alternatively, we could link it to the existing configuration to
>> control synching of object files.
>> 
>> I do not think core.fsyncObjectFiles having "object" in its name is
>> a good reason not to think those who set it to true only care about
>> the loose object files and nothing else.  It is more sensible to
>> consider that those who set it to true cares about the repository
>> integrity more than those who set it to false, I would think.
>> 
>> But that (i.e. doing it conditionally and choose which knob to use)
>> is one extra thing that needs justification, so starting from
>> unconditional fsync_or_die() may be the best way to ease it in.
>
> I'd be happy to revive my old patch series, but this kind of depends on
> where we're standing with the other patch series by Neeraj. If you say
> that we'll likely not land his patch series for the upcoming release,
> but a small patch series which only starts to sync loose refs may have a
> chance, then I'd like to go down that path as a stop-gap solution.
> Otherwise it probably wouldn't make a lot of sense.

The above was what I wrote before the revived series from Neeraj.
Now I've seen it, and more importantly, the most recent one from you
on top of that to add ref hardening as a new "component" or two, I
like the overall shape of the end result (except for the semantics
implemented by the configuration parser, which can be fixed without
affecting how the hardening components are implemented).  Hopefully
the base series of Neeraj can be solidified soon enough to make it
unnecessary for a stop-gap measure.  We'll see.

Thanks.

Patch

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index dbb134f7136..4f1747ec871 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -547,6 +547,25 @@  core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+	A comma-separated list of parts of the repository which should be
+	hardened via the core.fsyncMethod when created or modified. You can
+	disable hardening of any component by prefixing it with a '-'. Later
+	items take precedence over earlier ones in the list. For example,
+	`core.fsync=all,-pack-metadata` means "harden everything except pack
+	metadata." Items that are not hardened may be lost in the event of an
+	unclean system shutdown.
++
+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+
 core.fsyncMethod::
 	A value indicating the strategy Git will use to harden repository data
 	using fsync and related primitives.
@@ -556,14 +575,6 @@  core.fsyncMethod::
   filesystem and storage hardware, data added to the repository may not be
   durable in the event of a system crash. This is the default mode on macOS.
 
-core.fsyncObjectFiles::
-	This boolean will enable 'fsync()' when writing object files.
-+
-This is a total waste of time and effort on a filesystem that orders
-data writes properly, but can be useful for filesystems that do not use
-journalling (traditional UNIX filesystems) or that only journal metadata
-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
-
 core.preloadIndex::
 	Enable parallel index preload for operations like 'git diff'
 +
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 2b2e28bad79..ff70aeb1a0e 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -858,7 +858,7 @@  static void end_packfile(void)
 		struct tag *t;
 
 		close_pack_windows(pack_data);
-		finalize_hashfile(pack_file, cur_pack_oid.hash, 0);
+		finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0);
 		fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash,
 					 pack_data->pack_name, object_count,
 					 cur_pack_oid.hash, pack_size);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..b871d721e95 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1286,7 +1286,7 @@  static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 			    nr_objects - nr_objects_initial);
 		stop_progress_msg(&progress, msg.buf);
 		strbuf_release(&msg);
-		finalize_hashfile(f, tail_hash, 0);
+		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
 		hashcpy(read_hash, pack_hash);
 		fixup_pack_header_footer(output_fd, pack_hash,
 					 curr_pack, nr_objects,
@@ -1508,7 +1508,7 @@  static void final(const char *final_pack_name, const char *curr_pack_name,
 	if (!from_stdin) {
 		close(input_fd);
 	} else {
-		fsync_or_die(output_fd, curr_pack_name);
+		fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
 		err = close(output_fd);
 		if (err)
 			die_errno(_("error while closing pack file"));
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba2006f2212..b483ba65adb 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1199,16 +1199,26 @@  static void write_pack_file(void)
 			display_progress(progress_state, written);
 		}
 
-		/*
-		 * Did we write the wrong # entries in the header?
-		 * If so, rewrite it like in fast-import
-		 */
 		if (pack_to_stdout) {
-			finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+			/*
+			 * We never fsync when writing to stdout since we may
+			 * not be writing to an actual pack file. For instance,
+			 * the upload-pack code passes a pipe here. Calling
+			 * fsync on a pipe results in unnecessary
+			 * synchronization with the reader on some platforms.
+			 */
+			finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+					  CSUM_HASH_IN_STREAM | CSUM_CLOSE);
 		} else if (nr_written == nr_remaining) {
-			finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
+			finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK,
+					  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 		} else {
-			int fd = finalize_hashfile(f, hash, 0);
+			/*
+			 * If we wrote the wrong number of entries in the
+			 * header, rewrite it like in fast-import.
+			 */
+
+			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
 			fixup_pack_header_footer(fd, hash, pack_tmp_name,
 						 nr_written, hash, offset);
 			close(fd);
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 8785b2ac806..a2cf9dcbc8d 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -53,9 +53,10 @@  static void finish_bulk_checkin(struct bulk_checkin_state *state)
 		unlink(state->pack_tmp_name);
 		goto clear_exit;
 	} else if (state->nr_written == 1) {
-		finalize_hashfile(state->f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
+		finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,
+				  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 	} else {
-		int fd = finalize_hashfile(state->f, hash, 0);
+		int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);
 		fixup_pack_header_footer(fd, hash, state->pack_tmp_name,
 					 state->nr_written, hash,
 					 state->offset);
diff --git a/cache.h b/cache.h
index 37a32034b2f..b3cd7d928de 100644
--- a/cache.h
+++ b/cache.h
@@ -993,8 +993,38 @@  void reset_shared_repository(void);
 extern int read_replace_refs;
 extern char *git_replace_ref_base;
 
-extern int fsync_object_files;
-extern int use_fsync;
+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+	FSYNC_COMPONENT_NONE,
+	FSYNC_COMPONENT_LOOSE_OBJECT		= 1 << 0,
+	FSYNC_COMPONENT_PACK			= 1 << 1,
+	FSYNC_COMPONENT_PACK_METADATA		= 1 << 2,
+	FSYNC_COMPONENT_COMMIT_GRAPH		= 1 << 3,
+};
+
+#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
+				  FSYNC_COMPONENT_PACK_METADATA | \
+				  FSYNC_COMPONENT_COMMIT_GRAPH)
+
+#define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \
+				  FSYNC_COMPONENT_PACK | \
+				  FSYNC_COMPONENT_PACK_METADATA | \
+				  FSYNC_COMPONENT_COMMIT_GRAPH)
+
+#define FSYNC_COMPONENTS_ALL (FSYNC_COMPONENT_LOOSE_OBJECT | \
+			      FSYNC_COMPONENT_PACK | \
+			      FSYNC_COMPONENT_PACK_METADATA | \
+			      FSYNC_COMPONENT_COMMIT_GRAPH)
+
+
+/*
+ * A bitmask indicating which components of the repo should be fsynced.
+ */
+extern enum fsync_component fsync_components;
 
 enum fsync_method {
 	FSYNC_METHOD_FSYNC,
@@ -1002,6 +1032,7 @@  enum fsync_method {
 };
 
 extern enum fsync_method fsync_method;
+extern int use_fsync;
 extern int core_preload_index;
 extern int precomposed_unicode;
 extern int protect_hfs;
@@ -1757,6 +1788,12 @@  int copy_file_with_time(const char *dst, const char *src, int mode);
 void write_or_die(int fd, const void *buf, size_t count);
 void fsync_or_die(int fd, const char *);
 
+inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
+{
+	if (fsync_components & component)
+		fsync_or_die(fd, msg);
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
diff --git a/commit-graph.c b/commit-graph.c
index 265c010122e..64897f57d9f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1942,7 +1942,8 @@  static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	}
 
 	close_commit_graph(ctx->r->objects);
-	finalize_hashfile(f, file_hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
+	finalize_hashfile(f, file_hash, FSYNC_COMPONENT_COMMIT_GRAPH,
+			  CSUM_HASH_IN_STREAM | CSUM_FSYNC);
 	free_chunkfile(cf);
 
 	if (ctx->split) {
diff --git a/config.c b/config.c
index f67f545f839..224563c7b3e 100644
--- a/config.c
+++ b/config.c
@@ -1213,6 +1213,73 @@  static int git_parse_maybe_bool_text(const char *value)
 	return -1;
 }
 
+static const struct fsync_component_entry {
+	const char *name;
+	enum fsync_component component_bits;
+} fsync_component_table[] = {
+	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+	{ "pack", FSYNC_COMPONENT_PACK },
+	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+	{ "objects", FSYNC_COMPONENTS_OBJECTS },
+	{ "default", FSYNC_COMPONENTS_DEFAULT },
+	{ "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+	enum fsync_component output = 0;
+
+	if (!strcmp(string, "none"))
+		return output;
+
+	while (string) {
+		int i;
+		size_t len;
+		const char *ep;
+		int negated = 0;
+		int found = 0;
+
+		string = string + strspn(string, ", \t\n\r");
+		ep = strchrnul(string, ',');
+		len = ep - string;
+
+		if (*string == '-') {
+			negated = 1;
+			string++;
+			len--;
+			if (!len)
+				warning(_("invalid value for variable %s"), var);
+		}
+
+		if (!len)
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
+			const struct fsync_component_entry *entry = &fsync_component_table[i];
+
+			if (strncmp(entry->name, string, len))
+				continue;
+
+			found = 1;
+			if (negated)
+				output &= ~entry->component_bits;
+			else
+				output |= entry->component_bits;
+		}
+
+		if (!found) {
+			char *component = xstrndup(string, len);
+			warning(_("ignoring unknown core.fsync component '%s'"), component);
+			free(component);
+		}
+
+		string = ep;
+	}
+
+	return output;
+}
+
 int git_parse_maybe_bool(const char *value)
 {
 	int v = git_parse_maybe_bool_text(value);
@@ -1490,6 +1557,13 @@  static int git_default_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.fsync")) {
+		if (!value)
+			return config_error_nonbool(var);
+		fsync_components = parse_fsync_components(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.fsyncmethod")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -1503,7 +1577,7 @@  static int git_default_core_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "core.fsyncobjectfiles")) {
-		fsync_object_files = git_config_bool(var, value);
+		warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
 		return 0;
 	}
 
diff --git a/csum-file.c b/csum-file.c
index 26e8a6df44e..59ef3398ca2 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -58,7 +58,8 @@  static void free_hashfile(struct hashfile *f)
 	free(f);
 }
 
-int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags)
+int finalize_hashfile(struct hashfile *f, unsigned char *result,
+		      enum fsync_component component, unsigned int flags)
 {
 	int fd;
 
@@ -69,7 +70,7 @@  int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl
 	if (flags & CSUM_HASH_IN_STREAM)
 		flush(f, f->buffer, the_hash_algo->rawsz);
 	if (flags & CSUM_FSYNC)
-		fsync_or_die(f->fd, f->name);
+		fsync_component_or_die(component, f->fd, f->name);
 	if (flags & CSUM_CLOSE) {
 		if (close(f->fd))
 			die_errno("%s: sha1 file error on close", f->name);
diff --git a/csum-file.h b/csum-file.h
index 291215b34eb..0d29f528fbc 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -1,6 +1,7 @@ 
 #ifndef CSUM_FILE_H
 #define CSUM_FILE_H
 
+#include "cache.h"
 #include "hash.h"
 
 struct progress;
@@ -38,7 +39,7 @@  int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);
 struct hashfile *hashfd(int fd, const char *name);
 struct hashfile *hashfd_check(const char *name);
 struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp);
-int finalize_hashfile(struct hashfile *, unsigned char *, unsigned int);
+int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);
 void hashwrite(struct hashfile *, const void *, unsigned int);
 void hashflush(struct hashfile *f);
 void crc32_begin(struct hashfile *);
diff --git a/environment.c b/environment.c
index 3e3620d759f..378424b9af5 100644
--- a/environment.c
+++ b/environment.c
@@ -42,9 +42,9 @@  const char *git_attributes_file;
 const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
 int pack_compression_level = Z_DEFAULT_COMPRESSION;
-int fsync_object_files;
 int use_fsync = -1;
 enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
+enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 96 * 1024 * 1024;
diff --git a/midx.c b/midx.c
index 837b46b2af5..882f91f7d57 100644
--- a/midx.c
+++ b/midx.c
@@ -1406,7 +1406,8 @@  static int write_midx_internal(const char *object_dir,
 	write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);
 	write_chunkfile(cf, &ctx);
 
-	finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);
+	finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_FSYNC | CSUM_HASH_IN_STREAM);
 	free_chunkfile(cf);
 
 	if (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))
diff --git a/object-file.c b/object-file.c
index 8be57f48de7..ff29e678204 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1850,8 +1850,7 @@  int hash_object_file(const struct git_hash_algo *algo, const void *buf,
 static void close_loose_object(int fd)
 {
 	if (!the_repository->objects->odb->will_destroy) {
-		if (fsync_object_files)
-			fsync_or_die(fd, "loose object file");
+		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd, "loose object file");
 	}
 
 	if (close(fd) != 0)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e1..c16e43d1669 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -719,7 +719,8 @@  void bitmap_writer_finish(struct pack_idx_entry **index,
 	if (options & BITMAP_OPT_HASH_CACHE)
 		write_hash_cache(f, index, index_nr);
 
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 
 	if (adjust_shared_perm(tmp_file.buf))
 		die_errno("unable to make temporary bitmap file readable");
diff --git a/pack-write.c b/pack-write.c
index a5846f3a346..51812cb1299 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -159,9 +159,9 @@  const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 	}
 
 	hashwrite(f, sha1, the_hash_algo->rawsz);
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
-				    ((opts->flags & WRITE_IDX_VERIFY)
-				    ? 0 : CSUM_FSYNC));
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
+			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 	return index_name;
 }
 
@@ -281,8 +281,9 @@  const char *write_rev_file_order(const char *rev_name,
 	if (rev_name && adjust_shared_perm(rev_name) < 0)
 		die(_("failed to make %s readable"), rev_name);
 
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
-				    ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
+			  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 
 	return rev_name;
 }
@@ -390,7 +391,7 @@  void fixup_pack_header_footer(int pack_fd,
 		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
 	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
 	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
-	fsync_or_die(pack_fd, pack_name);
+	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
 char *index_pack_lockfile(int ip_out, int *is_well_formed)
diff --git a/read-cache.c b/read-cache.c
index cbe73f14e5e..a0de70195c8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3081,7 +3081,7 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			return -1;
 	}
 
-	finalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);
+	finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_NONE, CSUM_HASH_IN_STREAM);
 	if (close_tempfile_gently(tempfile)) {
 		error(_("could not close '%s'"), get_tempfile_path(tempfile));
 		return -1;