diff mbox series

[RFC,2/2] core.fsyncObjectFiles: make the docs less flippant

Message ID 20200917112830.26606-3-avarab@gmail.com
State New
Headers show
Series [RFC,1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles | expand

Commit Message

Ævar Arnfjörð Bjarmason Sept. 17, 2020, 11:28 a.m. UTC
As amusing as Linus's original prose[1] is here it doesn't really explain
in any detail to the uninitiated why you would or wouldn't enable
this, and the counter-intuitive reason for why git wouldn't fsync your
precious data.

So elaborate (a lot) on why this may or may not be needed. This is my
best-effort attempt to summarize the various points raised in the last
ML[2] discussion about this.

1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
    files", 2008-06-18)
2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Sept. 17, 2020, 2:12 p.m. UTC | #1
>  core.fsyncObjectFiles::
> +	This boolean will enable 'fsync()' when writing loose object
> +	files. Both the file itself and its containng directory will
> +	be fsynced.
> ++
> +When git writes data any required object writes will precede the
> +corresponding reference update(s). For example, a
> +linkgit:git-receive-pack[1] accepting a push might write a pack or
> +loose objects (depending on settings such as `transfer.unpackLimit`).
> ++
> +Therefore on a journaled file system which ensures that data is
> +flushed to disk in chronological order an fsync shouldn't be
> +needed. The loose objects might be lost with a crash, but so will the
> +ref update that would have referenced them. Git's own state in such a
> +crash will remain consistent.

While this is much better than what we had before I'm not sure it is
all that useful.  The only file system I know of that actually had the
above behavior was ext3, and the fact that it always wrote back that
way made it a complete performance desaster.  So even mentioning this
here will probably create a lot more confusion than actually clearing
things up.

> ++
> +This option exists because that assumption doesn't hold on filesystems
> +where the data ordering is not preserved, such as on ext3 and ext4
> +with "data=writeback". On such a filesystem the `rename()` that drops
> +the new reference in place might be preserved, but the contents or
> +directory entry for the loose object(s) might not have been synced to
> +disk.

As well as just about any other file system.  Which is another argument
on why it needs to be on by default.  Every time I install a new
development system (aka one that often crashes) and forget to enable
the option I keep corrupting my git repos.  And that is with at least
btrfs, ext4 and xfs as it is pretty much by design.

> +However, that's highly filesystem-dependent, on some filesystems
> +simply calling fsync() might force an unrelated bulk background write
> +to be serialized to disk. Such edge cases are the reason this option
> +is off by default. That default setting might change in future
> +versions.

Again the only "some file system" that was widely used that did this
was ext3.  And ext3 has long been removed from the Linux kernel..
Junio C Hamano Sept. 17, 2020, 3:43 p.m. UTC | #2
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As amusing as Linus's original prose[1] is here it doesn't really explain
> in any detail to the uninitiated why you would or wouldn't enable
> this, and the counter-intuitive reason for why git wouldn't fsync your
> precious data.
>
> So elaborate (a lot) on why this may or may not be needed. This is my
> best-effort attempt to summarize the various points raised in the last
> ML[2] discussion about this.
>
> 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
>     files", 2008-06-18)
> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 6 deletions(-)

When I saw the subject in my mailbox, I expected to see that you
would resurrect Christoph's updated text in [*1*], but you wrote a
whole lot more ;-) And they are quite informative to help readers to
understand what the option does.  I am not sure if the understanding
directly help readers to decide if it is appropriate for their own
repositories, though X-<.


Thanks.

[Reference]

*1* https://public-inbox.org/git/20180117193510.GA30657@lst.de/

>
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 74619a9c03..5b47670c16 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -548,12 +548,42 @@ core.whitespace::
>    errors. The default tab width is 8. Allowed values are 1 to 63.
>  
>  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").
> +	This boolean will enable 'fsync()' when writing loose object
> +	files. Both the file itself and its containng directory will
> +	be fsynced.
> ++
> +When git writes data any required object writes will precede the
> +corresponding reference update(s). For example, a
> +linkgit:git-receive-pack[1] accepting a push might write a pack or
> +loose objects (depending on settings such as `transfer.unpackLimit`).
> ++
> +Therefore on a journaled file system which ensures that data is
> +flushed to disk in chronological order an fsync shouldn't be
> +needed. The loose objects might be lost with a crash, but so will the
> +ref update that would have referenced them. Git's own state in such a
> +crash will remain consistent.
> ++
> +This option exists because that assumption doesn't hold on filesystems
> +where the data ordering is not preserved, such as on ext3 and ext4
> +with "data=writeback". On such a filesystem the `rename()` that drops
> +the new reference in place might be preserved, but the contents or
> +directory entry for the loose object(s) might not have been synced to
> +disk.
> ++
> +Enabling this option might slow git down by a lot in some
> +cases. E.g. in the case of a naïve bulk import tool which might create
> +a million loose objects before a final ref update and `gc`. In other
> +more common cases such as on a server being pushed to with default
> +`transfer.unpackLimit` settings the difference might not be noticable.
> ++
> +However, that's highly filesystem-dependent, on some filesystems
> +simply calling fsync() might force an unrelated bulk background write
> +to be serialized to disk. Such edge cases are the reason this option
> +is off by default. That default setting might change in future
> +versions.
> ++
> +In older versions of git only the descriptor for the file itself was
> +fsynced, not its directory entry.
>  
>  core.preloadIndex::
>  	Enable parallel index preload for operations like 'git diff'
Marc Branchaud Sept. 17, 2020, 7:21 p.m. UTC | #3
On 2020-09-17 7:28 a.m., Ævar Arnfjörð Bjarmason wrote:
> As amusing as Linus's original prose[1] is here it doesn't really explain
> in any detail to the uninitiated why you would or wouldn't enable
> this, and the counter-intuitive reason for why git wouldn't fsync your
> precious data.
> 
> So elaborate (a lot) on why this may or may not be needed. This is my
> best-effort attempt to summarize the various points raised in the last
> ML[2] discussion about this.
> 
> 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
>      files", 2008-06-18)
> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 74619a9c03..5b47670c16 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -548,12 +548,42 @@ core.whitespace::
>     errors. The default tab width is 8. Allowed values are 1 to 63.
>   
>   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").
> +	This boolean will enable 'fsync()' when writing loose object
> +	files. Both the file itself and its containng directory will

Typo: containng

		M.

> +	be fsynced.
> ++
> +When git writes data any required object writes will precede the
> +corresponding reference update(s). For example, a
> +linkgit:git-receive-pack[1] accepting a push might write a pack or
> +loose objects (depending on settings such as `transfer.unpackLimit`).
> ++
> +Therefore on a journaled file system which ensures that data is
> +flushed to disk in chronological order an fsync shouldn't be
> +needed. The loose objects might be lost with a crash, but so will the
> +ref update that would have referenced them. Git's own state in such a
> +crash will remain consistent.
> ++
> +This option exists because that assumption doesn't hold on filesystems
> +where the data ordering is not preserved, such as on ext3 and ext4
> +with "data=writeback". On such a filesystem the `rename()` that drops
> +the new reference in place might be preserved, but the contents or
> +directory entry for the loose object(s) might not have been synced to
> +disk.
> ++
> +Enabling this option might slow git down by a lot in some
> +cases. E.g. in the case of a naïve bulk import tool which might create
> +a million loose objects before a final ref update and `gc`. In other
> +more common cases such as on a server being pushed to with default
> +`transfer.unpackLimit` settings the difference might not be noticable.
> ++
> +However, that's highly filesystem-dependent, on some filesystems
> +simply calling fsync() might force an unrelated bulk background write
> +to be serialized to disk. Such edge cases are the reason this option
> +is off by default. That default setting might change in future
> +versions.
> ++
> +In older versions of git only the descriptor for the file itself was
> +fsynced, not its directory entry.
>   
>   core.preloadIndex::
>   	Enable parallel index preload for operations like 'git diff'
>
Johannes Sixt Sept. 17, 2020, 8:15 p.m. UTC | #4
Am 17.09.20 um 17:43 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
>> As amusing as Linus's original prose[1] is here it doesn't really explain
>> in any detail to the uninitiated why you would or wouldn't enable
>> this, and the counter-intuitive reason for why git wouldn't fsync your
>> precious data.
>>
>> So elaborate (a lot) on why this may or may not be needed. This is my
>> best-effort attempt to summarize the various points raised in the last
>> ML[2] discussion about this.
>>
>> 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
>>     files", 2008-06-18)
>> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> When I saw the subject in my mailbox, I expected to see that you
> would resurrect Christoph's updated text in [*1*], but you wrote a
> whole lot more ;-) And they are quite informative to help readers to
> understand what the option does.  I am not sure if the understanding
> directly help readers to decide if it is appropriate for their own
> repositories, though X-<.

Not only that; the new text also uses the term "fsync" in a manner that
I could be persuaded that it is actually an English word. Which, so far,
I doubt that it is ;) A little bit less 1337 wording would help the
users better.

> 
> 
> Thanks.
> 
> [Reference]
> 
> *1* https://public-inbox.org/git/20180117193510.GA30657@lst.de/
> 
>>
>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> index 74619a9c03..5b47670c16 100644
>> --- a/Documentation/config/core.txt
>> +++ b/Documentation/config/core.txt
>> @@ -548,12 +548,42 @@ core.whitespace::
>>    errors. The default tab width is 8. Allowed values are 1 to 63.
>>  
>>  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").
>> +	This boolean will enable 'fsync()' when writing loose object
>> +	files. Both the file itself and its containng directory will
>> +	be fsynced.
>> ++
>> +When git writes data any required object writes will precede the
>> +corresponding reference update(s). For example, a
>> +linkgit:git-receive-pack[1] accepting a push might write a pack or
>> +loose objects (depending on settings such as `transfer.unpackLimit`).
>> ++
>> +Therefore on a journaled file system which ensures that data is
>> +flushed to disk in chronological order an fsync shouldn't be
>> +needed. The loose objects might be lost with a crash, but so will the
>> +ref update that would have referenced them. Git's own state in such a
>> +crash will remain consistent.
>> ++
>> +This option exists because that assumption doesn't hold on filesystems
>> +where the data ordering is not preserved, such as on ext3 and ext4
>> +with "data=writeback". On such a filesystem the `rename()` that drops
>> +the new reference in place might be preserved, but the contents or
>> +directory entry for the loose object(s) might not have been synced to
>> +disk.
>> ++
>> +Enabling this option might slow git down by a lot in some
>> +cases. E.g. in the case of a naïve bulk import tool which might create
>> +a million loose objects before a final ref update and `gc`. In other
>> +more common cases such as on a server being pushed to with default
>> +`transfer.unpackLimit` settings the difference might not be noticable.
>> ++
>> +However, that's highly filesystem-dependent, on some filesystems
>> +simply calling fsync() might force an unrelated bulk background write
>> +to be serialized to disk. Such edge cases are the reason this option
>> +is off by default. That default setting might change in future
>> +versions.
>> ++
>> +In older versions of git only the descriptor for the file itself was
>> +fsynced, not its directory entry.
>>  
>>  core.preloadIndex::
>>  	Enable parallel index preload for operations like 'git diff'
>
Johannes Schindelin Oct. 8, 2020, 8:13 a.m. UTC | #5
Hi Junio and Ævar,

On Thu, 17 Sep 2020, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
> > As amusing as Linus's original prose[1] is here it doesn't really explain
> > in any detail to the uninitiated why you would or wouldn't enable
> > this, and the counter-intuitive reason for why git wouldn't fsync your
> > precious data.
> >
> > So elaborate (a lot) on why this may or may not be needed. This is my
> > best-effort attempt to summarize the various points raised in the last
> > ML[2] discussion about this.
> >
> > 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
> >     files", 2008-06-18)
> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > ---
> >  Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 6 deletions(-)
>
> When I saw the subject in my mailbox, I expected to see that you
> would resurrect Christoph's updated text in [*1*], but you wrote a
> whole lot more ;-) And they are quite informative to help readers to
> understand what the option does.  I am not sure if the understanding
> directly help readers to decide if it is appropriate for their own
> repositories, though X-<.

I agree that it is an improvement, and am therefore in favor of applying
the patch.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason Oct. 8, 2020, 3:57 p.m. UTC | #6
On Thu, Oct 08 2020, Johannes Schindelin wrote:

> Hi Junio and Ævar,
>
> On Thu, 17 Sep 2020, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>> > As amusing as Linus's original prose[1] is here it doesn't really explain
>> > in any detail to the uninitiated why you would or wouldn't enable
>> > this, and the counter-intuitive reason for why git wouldn't fsync your
>> > precious data.
>> >
>> > So elaborate (a lot) on why this may or may not be needed. This is my
>> > best-effort attempt to summarize the various points raised in the last
>> > ML[2] discussion about this.
>> >
>> > 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
>> >     files", 2008-06-18)
>> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
>> >
>> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> > ---
>> >  Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
>> >  1 file changed, 36 insertions(+), 6 deletions(-)
>>
>> When I saw the subject in my mailbox, I expected to see that you
>> would resurrect Christoph's updated text in [*1*], but you wrote a
>> whole lot more ;-) And they are quite informative to help readers to
>> understand what the option does.  I am not sure if the understanding
>> directly help readers to decide if it is appropriate for their own
>> repositories, though X-<.
>
> I agree that it is an improvement, and am therefore in favor of applying
> the patch.

Just the improved docs, or flipping the default of core.fsyncObjectFiles
to "true"?

I've been meaning to re-roll this. I won't have time anytime soon to fix
git's fsync() use, i.e. ensure that we run up & down modified
directories and fsync()/fdatasync() file/dir fd's as appropriate but I
think documenting it and changing the core.fsyncObjectFiles default
makes sense and is at least a step in the right direction.

I do think it makes more sense for a v2 to split most of this out into
some section that generally discusses data integrity in the .git
directory. I.e. that says that currently where we use fsync() (such as
pack/commit-graph writes) we don't fsync() the corresponding
director{y,ies), and ref updates don't fsync() at all.

Where to put that though? gitrepository-layout(5)? Or a new page like
gitrepository-integrity(5) (other suggestions welcome..).

Looking at the code again it seems easier than I thought to make this
right if we ignore .git/refs (which reftable can fix for us). Just:

1. Change fsync_or_die() and its callsites to also pass/sync the
   containing directory, which is always created already
   (e.g. .git/objects/pack/)...).

2. ..Or in the case where it's not created already such as
   .git/objects/??/ (or .git/objects/pack/) itself) it's not N-deep like
   the refs hierarchy, so "did we create it" state is pretty simple, or
   we can just always do it unconditionally.

3. Without reftable the .git/refs/ case shouldn't be too hard if we're
   OK with redundantly fsyncing all the way down, i.e. to make it
   simpler by not tracking the state of exactly what was changed.

4. Now that I'm writing this there's also .git/{config,rr-cache} and any
   number of other things we need to change for 100% coverage, but the
   above 1-3 should be good enough for repo integrity where repo = refs
   & objects.
Junio C Hamano Oct. 8, 2020, 6:53 p.m. UTC | #7
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> When I saw the subject in my mailbox, I expected to see that you
>>> would resurrect Christoph's updated text in [*1*], but you wrote a
>>> whole lot more ;-) And they are quite informative to help readers to
>>> understand what the option does.  I am not sure if the understanding
>>> directly help readers to decide if it is appropriate for their own
>>> repositories, though X-<.
>>
>> I agree that it is an improvement, and am therefore in favor of applying
>> the patch.
>
> Just the improved docs, or flipping the default of core.fsyncObjectFiles
> to "true"?

I am not Dscho, but "applying THE patch" meant, at least to me, the
patch [2/2] to the docs, which was the message we are responding to.

> I've been meaning to re-roll this. I won't have time anytime soon to fix
> git's fsync() use, i.e. ensure that we run up & down modified
> directories and fsync()/fdatasync() file/dir fd's as appropriate but I
> think documenting it and changing the core.fsyncObjectFiles default
> makes sense and is at least a step in the right direction.
>
> I do think it makes more sense for a v2 to split most of this out into
> some section that generally discusses data integrity in the .git
> directory. I.e. that says that currently where we use fsync() (such as
> pack/commit-graph writes) we don't fsync() the corresponding
> director{y,ies), and ref updates don't fsync() at all.

Yes, I think all of these are sensible things to do sometime in the
future.


> Where to put that though? gitrepository-layout(5)? Or a new page like
> gitrepository-integrity(5) (other suggestions welcome..).

I do not have a good suggestion at this moment on this.
Johannes Schindelin Oct. 9, 2020, 10:44 a.m. UTC | #8
Hi Ævar,

On Thu, 8 Oct 2020, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Oct 08 2020, Johannes Schindelin wrote:
>
> > On Thu, 17 Sep 2020, Junio C Hamano wrote:
> >
> >> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >>
> >> > As amusing as Linus's original prose[1] is here it doesn't really explain
> >> > in any detail to the uninitiated why you would or wouldn't enable
> >> > this, and the counter-intuitive reason for why git wouldn't fsync your
> >> > precious data.
> >> >
> >> > So elaborate (a lot) on why this may or may not be needed. This is my
> >> > best-effort attempt to summarize the various points raised in the last
> >> > ML[2] discussion about this.
> >> >
> >> > 1.  aafe9fbaf4 ("Add config option to enable 'fsync()' of object
> >> >     files", 2008-06-18)
> >> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/
> >> >
> >> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> > ---
> >> >  Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----
> >> >  1 file changed, 36 insertions(+), 6 deletions(-)
> >>
> >> When I saw the subject in my mailbox, I expected to see that you
> >> would resurrect Christoph's updated text in [*1*], but you wrote a
> >> whole lot more ;-) And they are quite informative to help readers to
> >> understand what the option does.  I am not sure if the understanding
> >> directly help readers to decide if it is appropriate for their own
> >> repositories, though X-<.
> >
> > I agree that it is an improvement, and am therefore in favor of applying
> > the patch.
>
> Just the improved docs, or flipping the default of core.fsyncObjectFiles
> to "true"?

I am actually also in favor of flipping the default. We carry
https://github.com/git-for-windows/git/commit/14dad078c28159b250be599c0890ece2d6f4d635
in Git for Windows for over three years. The commit message:

	mingw: change core.fsyncObjectFiles = 1 by default

	From the documentation of said setting:

		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").

	The most common file system on Windows (NTFS) does not guarantee that
	order, therefore a sudden loss of power (or any other event causing an
	unclean shutdown) would cause corrupt files (i.e. files filled with
	NULs). Therefore we need to change the default.

	Note that the documentation makes it sound as if this causes really bad
	performance. In reality, writing loose objects is something that is done
	only rarely, and only a handful of files at a time.

	Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The patch itself limits this change to Windows, but if this becomes a
platform-independent change, all the better for me!

> I've been meaning to re-roll this. I won't have time anytime soon to fix
> git's fsync() use, i.e. ensure that we run up & down modified
> directories and fsync()/fdatasync() file/dir fd's as appropriate but I
> think documenting it and changing the core.fsyncObjectFiles default
> makes sense and is at least a step in the right direction.

Agreed.

> I do think it makes more sense for a v2 to split most of this out into
> some section that generally discusses data integrity in the .git
> directory. I.e. that says that currently where we use fsync() (such as
> pack/commit-graph writes) we don't fsync() the corresponding
> director{y,ies), and ref updates don't fsync() at all.
>
> Where to put that though? gitrepository-layout(5)? Or a new page like
> gitrepository-integrity(5) (other suggestions welcome..).

I think `gitrepository-layout` is probably the best location for now.

> Looking at the code again it seems easier than I thought to make this
> right if we ignore .git/refs (which reftable can fix for us). Just:
>
> 1. Change fsync_or_die() and its callsites to also pass/sync the
>    containing directory, which is always created already
>    (e.g. .git/objects/pack/)...).
>
> 2. ..Or in the case where it's not created already such as
>    .git/objects/??/ (or .git/objects/pack/) itself) it's not N-deep like
>    the refs hierarchy, so "did we create it" state is pretty simple, or
>    we can just always do it unconditionally.
>
> 3. Without reftable the .git/refs/ case shouldn't be too hard if we're
>    OK with redundantly fsyncing all the way down, i.e. to make it
>    simpler by not tracking the state of exactly what was changed.
>
> 4. Now that I'm writing this there's also .git/{config,rr-cache} and any
>    number of other things we need to change for 100% coverage, but the
>    above 1-3 should be good enough for repo integrity where repo = refs
>    & objects.

I fear that the changes to `fsync` also the directory need to be guarded
behind a preprocessor flag, though: if you try to `open()` a directory on
Windows, it fails (our emulated `open()` sets `errno = EISDIR`).

Ciao,
Dscho
diff mbox series

Patch

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 74619a9c03..5b47670c16 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -548,12 +548,42 @@  core.whitespace::
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
 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").
+	This boolean will enable 'fsync()' when writing loose object
+	files. Both the file itself and its containng directory will
+	be fsynced.
++
+When git writes data any required object writes will precede the
+corresponding reference update(s). For example, a
+linkgit:git-receive-pack[1] accepting a push might write a pack or
+loose objects (depending on settings such as `transfer.unpackLimit`).
++
+Therefore on a journaled file system which ensures that data is
+flushed to disk in chronological order an fsync shouldn't be
+needed. The loose objects might be lost with a crash, but so will the
+ref update that would have referenced them. Git's own state in such a
+crash will remain consistent.
++
+This option exists because that assumption doesn't hold on filesystems
+where the data ordering is not preserved, such as on ext3 and ext4
+with "data=writeback". On such a filesystem the `rename()` that drops
+the new reference in place might be preserved, but the contents or
+directory entry for the loose object(s) might not have been synced to
+disk.
++
+Enabling this option might slow git down by a lot in some
+cases. E.g. in the case of a naïve bulk import tool which might create
+a million loose objects before a final ref update and `gc`. In other
+more common cases such as on a server being pushed to with default
+`transfer.unpackLimit` settings the difference might not be noticable.
++
+However, that's highly filesystem-dependent, on some filesystems
+simply calling fsync() might force an unrelated bulk background write
+to be serialized to disk. Such edge cases are the reason this option
+is off by default. That default setting might change in future
+versions.
++
+In older versions of git only the descriptor for the file itself was
+fsynced, not its directory entry.
 
 core.preloadIndex::
 	Enable parallel index preload for operations like 'git diff'