{"thread":{"id":"47626","subject":"[PATCH] enable core.fsyncObjectFiles by default","startedAt":"2018-01-17T18:48:37Z","lastAt":"2020-11-19T11:38:41Z","messageCount":52,"participants":["Christoph Hellwig","Junio C Hamano","Matthew Wilcox","Andreas Schwab","Jeff King","Ævar Arnfjörð Bjarmason","Linus Torvalds","Theodore Ts'o","Chris Mason","Johannes Sixt","Taylor Blau","Marc Branchaud","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336710","messageId":"20180117184828.31816-1-hch@lst.de","threadId":"47626","inReplyTo":null,"subject":"[PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2018-01-17T18:48:28Z","receivedAt":"2018-01-17T18:48:37Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"fsync is required for data integrity as there is no gurantee that\ndata makes it to disk at any specified time without it. Even for\next3 with data=ordered mode the file system will only commit all\ndata at some point in time that is not guaranteed.\n\nI've lost data on development machines with various times countless\ntimes due to the lack of this option, and now lost trees on a\ngit server with ext4 as well yesterday. It's time to make git\nsafe by default.\n\nSigned-off-by: Christoph Hellwig \n---\n Documentation/config.txt | 6 ++----\n environment.c | 2 +-\n 2 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0e25b2c92..9a1cec5c8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -866,10 +866,8 @@ core.whitespace::\n core.fsyncObjectFiles::\n \tThis boolean will enable 'fsync()' when writing object files.\n +\n-This is a total waste of time and effort on a filesystem that orders\n-data writes properly, but can be useful for filesystems that do not use\n-journalling (traditional UNIX filesystems) or that only journal metadata\n-and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n+This option is enabled by default and ensures actual data integrity\n+by calling fsync after writing object files.\n \n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\ndiff --git a/environment.c b/environment.c\nindex 63ac38a46..c74375b5e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -36,7 +36,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n-int fsync_object_files;\n+int fsync_object_files = 1;\n size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 96 * 1024 * 1024;\n-- \n2.14.2\n\n"},{"id":"336711","messageId":"xmqqd128s3wf.fsf@gitster.mtv.corp.google.com","threadId":"47626","inReplyTo":"20180117184828.31816-1-hch@lst.de","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-17T19:04:32Z","receivedAt":"2018-01-17T19:04:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christoph Hellwig writes:\n\n> fsync is required for data integrity as there is no gurantee that\n> data makes it to disk at any specified time without it. Even for\n> ext3 with data=ordered mode the file system will only commit all\n> data at some point in time that is not guaranteed.\n\nIt comes from this one:\n\ncommit aafe9fbaf4f1d1f27a6f6e3eb3e246fff81240ef\nAuthor: Linus Torvalds \nDate: Wed Jun 18 15:18:44 2008 -0700\n\n Add config option to enable 'fsync()' of object files\n \n As explained in the documentation[*] this is totally useless on\n filesystems that do ordered/journalled data writes, but it can be a\n useful safety feature on filesystems like HFS+ that only journal the\n metadata, not the actual file contents.\n \n It defaults to off, although we could presumably in theory some day\n auto-enable it on a per-filesystem basis.\n \n [*] Yes, I updated the docs for the thing. Hell really _has_ frozen\n over, and the four horsemen are probably just beyond the horizon.\n EVERYBODY PANIC!\n \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0e25b2c92..9a1cec5c8 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -866,10 +866,8 @@ core.whitespace::\n> core.fsyncObjectFiles::\n> \tThis boolean will enable 'fsync()' when writing object files.\n> +\n> -This is a total waste of time and effort on a filesystem that orders\n> -data writes properly, but can be useful for filesystems that do not use\n> -journalling (traditional UNIX filesystems) or that only journal metadata\n> -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n> +This option is enabled by default and ensures actual data integrity\n> +by calling fsync after writing object files.\n\nI am somewhat sympathetic to the desire to flip the default to\n\"safe\" and allow those who know they are already safe to tweak the\nknob for performance, and it also makes sense to document that the\ndefault is \"true\" here. But I do not see the point of removing the\nfour lines from this paragraph; the sole effect of the removal is to\nrob information from readers that they can use to decide if they\nwant to disable the configuration, no?\n"},{"id":"336714","messageId":"20180117193510.GA30657@lst.de","threadId":"47626","inReplyTo":"xmqqd128s3wf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2018-01-17T19:35:10Z","receivedAt":"2018-01-17T19:35:19Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Wed, Jan 17, 2018 at 11:04:32AM -0800, Junio C Hamano wrote:\n> I am somewhat sympathetic to the desire to flip the default to\n> \"safe\" and allow those who know they are already safe to tweak the\n> knob for performance, and it also makes sense to document that the\n> default is \"true\" here. But I do not see the point of removing the\n> four lines from this paragraph; the sole effect of the removal is to\n> rob information from readers that they can use to decide if they\n> want to disable the configuration, no?\n\nDoes this sound any better? It is a little to technical for my\ntaste, but I couldn't come up with anything better:\n\n---\nFrom ab8f2d38dfe40e74de5399af0d069427c7473b76 Mon Sep 17 00:00:00 2001\nFrom: Christoph Hellwig \nDate: Wed, 17 Jan 2018 19:42:46 +0100\nSubject: enable core.fsyncObjectFiles by default\n\nfsync is required for data integrity as there is no gurantee that\ndata makes it to disk at any specified time without it. Even for\next3 with data=ordered mode the file system will only commit all\ndata at some point in time that is not guaranteed.\n\nI've lost data on development machines with various times countless\ntimes due to the lack of this option, and now lost trees on a\ngit server with ext4 as well yesterday. It's time to make git\nsafe by default.\n\nSigned-off-by: Christoph Hellwig \n---\n Documentation/config.txt | 11 +++++++----\n environment.c | 2 +-\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0e25b2c92..8b99f1389 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -866,10 +866,13 @@ core.whitespace::\n core.fsyncObjectFiles::\n \tThis boolean will enable 'fsync()' when writing object files.\n +\n-This is a total waste of time and effort on a filesystem that orders\n-data writes properly, but can be useful for filesystems that do not use\n-journalling (traditional UNIX filesystems) or that only journal metadata\n-and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n+This option is enabled by default and ensures actual data integrity\n+by calling fsync after writing object files.\n+\n+Note that this might be really slow on ext3 in the traditional\n+data=ordered mode, in which case you might want to disable this option.\n+ext3 in data=ordered mode will order the actual data writeout with\n+metadata operation, but not actually guarantee data integrity either.\n \n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\ndiff --git a/environment.c b/environment.c\nindex 63ac38a46..c74375b5e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -36,7 +36,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n-int fsync_object_files;\n+int fsync_object_files = 1;\n size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 96 * 1024 * 1024;\n-- \n2.14.2\n\n\n\n"},{"id":"336715","messageId":"20180117193731.GC25862@bombadil.infradead.org","threadId":"47626","inReplyTo":"xmqqd128s3wf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Matthew Wilcox","fromEmail":"willy@infradead.org","sentAt":"2018-01-17T19:37:31Z","receivedAt":"2018-01-17T19:37:58Z","isPatch":true,"sender":{"key":"willy@infradead.org","avatar":null},"body":"On Wed, Jan 17, 2018 at 11:04:32AM -0800, Junio C Hamano wrote:\n> Christoph Hellwig writes:\n> > @@ -866,10 +866,8 @@ core.whitespace::\n> > core.fsyncObjectFiles::\n> > \tThis boolean will enable 'fsync()' when writing object files.\n> > +\n> > -This is a total waste of time and effort on a filesystem that orders\n> > -data writes properly, but can be useful for filesystems that do not use\n> > -journalling (traditional UNIX filesystems) or that only journal metadata\n> > -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n> > +This option is enabled by default and ensures actual data integrity\n> > +by calling fsync after writing object files.\n> \n> I am somewhat sympathetic to the desire to flip the default to\n> \"safe\" and allow those who know they are already safe to tweak the\n> knob for performance, and it also makes sense to document that the\n> default is \"true\" here. But I do not see the point of removing the\n> four lines from this paragraph; the sole effect of the removal is to\n> rob information from readers that they can use to decide if they\n> want to disable the configuration, no?\n\nHow about this instead?\n\nThis option is enabled by default and ensures data integrity by calling\nfsync after writing object files. It is not necessary on filesystems\nwhich journal data writes, but is still necessary on filesystems which\ndo not use journalling (ext2), or that only journal metadata writes\n(OS X's HFS+, or Linux's ext4 with \"data=writeback\"). Turning this\noption off will increase performance at the possible risk of data loss.\n\n"},{"id":"336716","messageId":"20180117194225.GA30940@lst.de","threadId":"47626","inReplyTo":"20180117193731.GC25862@bombadil.infradead.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2018-01-17T19:42:25Z","receivedAt":"2018-01-17T19:42:33Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Wed, Jan 17, 2018 at 11:37:31AM -0800, Matthew Wilcox wrote:\n> How about this instead?\n> \n> This option is enabled by default and ensures data integrity by calling\n> fsync after writing object files. It is not necessary on filesystems\n> which journal data writes, but is still necessary on filesystems which\n> do not use journalling (ext2), or that only journal metadata writes\n> (OS X's HFS+, or Linux's ext4 with \"data=writeback\"). Turning this\n> option off will increase performance at the possible risk of data loss.\n\nI think this goes entirely into the wrong direction. The point is\nfsync is always the right thing to do. But on ext3 (and ext3 only)\nthe right thing is way too painful, and conveniently ext3 happens\nto be almost ok without it. So if anything should get a special\nmention it is ext3.\n"},{"id":"336717","messageId":"87a7xcw8sa.fsf@linux-m68k.org","threadId":"47626","inReplyTo":"20180117193510.GA30657@lst.de","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2018-01-17T20:05:25Z","receivedAt":"2018-01-17T20:05:35Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jan 17 2018, Christoph Hellwig wrote:\n\n> I've lost data on development machines with various times countless\n> times due to the lack of this option, and now lost trees on a\n\nToo many times. :-)\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"336721","messageId":"20180117205509.GA14828@sigill.intra.peff.net","threadId":"47626","inReplyTo":"20180117184828.31816-1-hch@lst.de","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-17T20:55:09Z","receivedAt":"2018-01-17T20:55:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 17, 2018 at 07:48:28PM +0100, Christoph Hellwig wrote:\n\n> fsync is required for data integrity as there is no gurantee that\n> data makes it to disk at any specified time without it. Even for\n> ext3 with data=ordered mode the file system will only commit all\n> data at some point in time that is not guaranteed.\n> \n> I've lost data on development machines with various times countless\n> times due to the lack of this option, and now lost trees on a\n> git server with ext4 as well yesterday. It's time to make git\n> safe by default.\n\nI'm definitely sympathetic, and I've contemplated a patch like this a\nfew times. But I'm not sure we're \"safe by default\" here after this\npatch. In particular:\n\n 1. This covers only loose objects. We generally sync pack writes\n already, so we're covered there. But we do not sync ref updates at\n all, which we'd probably want to in a default-safe setup (a common\n post-crash symptom I've seen is zero-length ref files).\n\n 2. Is it sufficient to fsync() the individual file's descriptors?\n We often do other filesystem operations (like hardlinking or\n renaming) that also need to be committed to disk before an\n operation can be considered saved.\n\n 3. Related to (2), we often care about the order of metadata commits.\n E.g., a common sequence is:\n\n a. Write object contents to tempfile.\n\n b. rename() or hardlink tempfile to final name.\n\n c. Write object name into ref.lock file.\n\n d. rename() ref.lock to ref\n\n If we see (d) but not (b), then the result is a corrupted\n repository. Is this guaranteed by ext4 journaling with\n data=ordered?\n\nIt may be that data=ordered gets us what we need for (2) and (3). But I\nthink at the very least we should consider fsyncing ref updates based on\na config option, too.\n\n-Peff\n"},{"id":"336723","messageId":"20180117211011.GA355@lst.de","threadId":"47626","inReplyTo":"20180117205509.GA14828@sigill.intra.peff.net","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2018-01-17T21:10:11Z","receivedAt":"2018-01-17T21:10:20Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Wed, Jan 17, 2018 at 03:55:09PM -0500, Jeff King wrote:\n> I'm definitely sympathetic, and I've contemplated a patch like this a\n> few times. But I'm not sure we're \"safe by default\" here after this\n> patch. In particular:\n> \n> 1. This covers only loose objects. We generally sync pack writes\n> already, so we're covered there. But we do not sync ref updates at\n> all, which we'd probably want to in a default-safe setup (a common\n> post-crash symptom I've seen is zero-length ref files).\n\nI've not seen them myself yet, but yes, they need an fsync.\n\n> 2. Is it sufficient to fsync() the individual file's descriptors?\n> We often do other filesystem operations (like hardlinking or\n> renaming) that also need to be committed to disk before an\n> operation can be considered saved.\n\nNo, for metadata operations we need to fsync the directory as well.\n\n> 3. Related to (2), we often care about the order of metadata commits.\n> E.g., a common sequence is:\n> \n> a. Write object contents to tempfile.\n> \n> b. rename() or hardlink tempfile to final name.\n> \n> c. Write object name into ref.lock file.\n> \n> d. rename() ref.lock to ref\n> \n> If we see (d) but not (b), then the result is a corrupted\n> repository. Is this guaranteed by ext4 journaling with\n> data=ordered?\n\nIt is not generally guranteed by Linux file system semantics. Various\nfile system will actually start writeback of file data before rename,\nbut not actually wait on it.\n"},{"id":"336726","messageId":"87h8rki2iu.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"xmqqd128s3wf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-17T21:44:25Z","receivedAt":"2018-01-17T21:44:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 17 2018, Junio C. Hamano jotted:\n\n> Christoph Hellwig writes:\n>\n>> fsync is required for data integrity as there is no gurantee that\n>> data makes it to disk at any specified time without it. Even for\n>> ext3 with data=ordered mode the file system will only commit all\n>> data at some point in time that is not guaranteed.\n>\n> It comes from this one:\n>\n> commit aafe9fbaf4f1d1f27a6f6e3eb3e246fff81240ef\n> Author: Linus Torvalds \n> Date: Wed Jun 18 15:18:44 2008 -0700\n>\n> Add config option to enable 'fsync()' of object files\n>\n> As explained in the documentation[*] this is totally useless on\n> filesystems that do ordered/journalled data writes, but it can be a\n> useful safety feature on filesystems like HFS+ that only journal the\n> metadata, not the actual file contents.\n>\n> It defaults to off, although we could presumably in theory some day\n> auto-enable it on a per-filesystem basis.\n>\n> [*] Yes, I updated the docs for the thing. Hell really _has_ frozen\n> over, and the four horsemen are probably just beyond the horizon.\n> EVERYBODY PANIC!\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 0e25b2c92..9a1cec5c8 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -866,10 +866,8 @@ core.whitespace::\n>> core.fsyncObjectFiles::\n>> \tThis boolean will enable 'fsync()' when writing object files.\n>> +\n>> -This is a total waste of time and effort on a filesystem that orders\n>> -data writes properly, but can be useful for filesystems that do not use\n>> -journalling (traditional UNIX filesystems) or that only journal metadata\n>> -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n>> +This option is enabled by default and ensures actual data integrity\n>> +by calling fsync after writing object files.\n>\n> I am somewhat sympathetic to the desire to flip the default to\n> \"safe\" and allow those who know they are already safe to tweak the\n> knob for performance, and it also makes sense to document that the\n> default is \"true\" here. But I do not see the point of removing the\n> four lines from this paragraph; the sole effect of the removal is to\n> rob information from readers that they can use to decide if they\n> want to disable the configuration, no?\n\n[CC'd the author of the current behavior]\n\nSome points/questions:\n\n a) Is there some reliable way to test whether this is needed from\n userspace? I'm thinking something like `git update-index\n --test-untracked-cache` but for fsync().\n\n b) On the filesystems that don't need this, what's the performance\n impact?\n\n I ran a small test myself on CentOS 7 (3.10) with ext4 data=ordered\n on the tests I thought might do a lot of loose object writes:\n\n $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS=\"NO_OPENSSL=Y CFLAGS=-O3 -j56\" ./run origin/master fsync-on~ fsync-on p3400-rebase.sh p0007-write-cache.sh\n [...]\n Test fsync-on~ fsync-on\n -------------------------------------------------------------------------------------------------------\n 3400.2: rebase on top of a lot of unrelated changes 1.45(1.30+0.17) 1.45(1.28+0.20) +0.0%\n 3400.4: rebase a lot of unrelated changes without split-index 4.34(3.71+0.66) 4.33(3.69+0.66) -0.2%\n 3400.6: rebase a lot of unrelated changes with split-index 3.38(2.94+0.47) 3.38(2.93+0.47) +0.0%\n 0007.2: write_locked_index 3 times (3214 files) 0.01(0.00+0.00) 0.01(0.00+0.00) +0.0%\n\n No impact. However I did my own test of running the test suite 10%\n times with/without this patch, and it runs 9% slower:\n\n fsync-off: avg:21.59 21.50 21.50 21.52 21.53 21.54 21.57 21.59 21.61 21.63 21.95\n fsync-on: avg:23.43 23.21 23.25 23.26 23.26 23.27 23.32 23.49 23.51 23.83 23.88\n\n Test script at the end of this E-Mail.\n\n c) What sort of guarantees in this regard do NFS-mounted filesystems\n commonly make?\n\nTest script:\n\nuse v5.10.0;\nuse strict;\nuse warnings;\nuse Time::HiRes qw(time);\nuse List::Util qw(sum);\nuse Data::Dumper;\n\nmy %time;\nfor my $ref (@ARGV) {\n system \"git checkout $ref\";\n system qq[make -j56 CFLAGS=\"-O3 -g\" NO_OPENSSL=Y all];\n for (1..10) {\n my $t0 = -time();\n system \"(cd t && NO_SVN_TESTS=1 GIT_TEST_HTTPD=0 prove -j56 --state=slow,save t[0-9]*.sh)\";\n $t0 += time();\n push @{$time{$ref}} => $t0;\n }\n}\nfor my $ref (sort keys %time) {\n printf \"%20s: avg:%.2f %s\\n\",\n $ref,\n sum(@{$time{$ref}})/@{$time{$ref}},\n join(\" \", map { sprintf \"%.02f\", $_ } sort { $a <=> $b } @{$time{$ref}});\n}\n"},{"id":"336731","messageId":"CA+55aFzJ2QO0MH3vgbUd8X-dzg_65A-jKmEBMSVt8ST2bpmzSQ@mail.gmail.com","threadId":"47626","inReplyTo":"87h8rki2iu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-01-17T22:07:22Z","receivedAt":"2018-01-17T22:07:30Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 17, 2018 at 1:44 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> I ran a small test myself on CentOS 7 (3.10) with ext4 data=ordered\n> on the tests I thought might do a lot of loose object writes:\n>\n> $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS=\"NO_OPENSSL=Y CFLAGS=-O3 -j56\" ./run origin/master fsync-on~ fsync-on p3400-rebase.sh p0007-write-cache.sh\n> [...]\n> Test fsync-on~ fsync-on\n> -------------------------------------------------------------------------------------------------------\n> 3400.2: rebase on top of a lot of unrelated changes 1.45(1.30+0.17) 1.45(1.28+0.20) +0.0%\n> 3400.4: rebase a lot of unrelated changes without split-index 4.34(3.71+0.66) 4.33(3.69+0.66) -0.2%\n> 3400.6: rebase a lot of unrelated changes with split-index 3.38(2.94+0.47) 3.38(2.93+0.47) +0.0%\n> 0007.2: write_locked_index 3 times (3214 files) 0.01(0.00+0.00) 0.01(0.00+0.00) +0.0%\n>\n> No impact. However I did my own test of running the test suite 10%\n> times with/without this patch, and it runs 9% slower:\n>\n> fsync-off: avg:21.59 21.50 21.50 21.52 21.53 21.54 21.57 21.59 21.61 21.63 21.95\n> fsync-on: avg:23.43 23.21 23.25 23.26 23.26 23.27 23.32 23.49 23.51 23.83 23.88\n\nThat's not the thing you should check.\n\nNow re-do the test while another process writes to a totally unrelated\na huge file (say, do a ISO file copy or something).\n\nThat was the thing that several filesystems get completely and\nhorribly wrong. Generally _particularly_ the logging filesystems that\ndon't even need the fsync, because they use a single log for\neverything (so fsync serializes all the writes, not just the writes to\nthe one file it's fsync'ing).\n\nThe original git design was very much to write each object file\nwithout any syncing, because they don't matter since a new object file\n- by definition - isn't really reachable. Then sync before writing the\nindex file or a new ref.\n\nBut things have changed, I'm not arguing that the code shouldn't be\nmade safe by default. I personally refuse to use rotating media on my\nmachines anyway, largely exactly because of the fsync() issue (things\nlike \"firefox\" started doing fsync on the mysql database for stupid\nthings, and you'd get huge pauses).\n\nBut I do think your benchmark is wrong. The case where only git does\nsomething is not interesting or relevant. It really is \"git does\nsomething _and_ somebody else writes something entirely unrelated at\nthe same time\" that matters.\n\n Linus\n"},{"id":"336734","messageId":"CA+55aFxSEd4azxVTEDNnUJaFY0Lp7VMQ2OGTYmVOFF7cr_HcqA@mail.gmail.com","threadId":"47626","inReplyTo":"CA+55aFzJ2QO0MH3vgbUd8X-dzg_65A-jKmEBMSVt8ST2bpmzSQ@mail.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-01-17T22:25:41Z","receivedAt":"2018-01-17T22:25:48Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 17, 2018 at 2:07 PM, Linus Torvalds\n wrote:\n>\n> The original git design was very much to write each object file\n> without any syncing, because they don't matter since a new object file\n> - by definition - isn't really reachable. Then sync before writing the\n> index file or a new ref.\n\n.. actually, I think it originally sync'ed only before starting to\nactually remove objects due to repacking.\n\nThe theory was that you can lose the last commit series or whatever,\nand have to git-fsck, and have to re-do it, but you could never lose\nlong-term work. If your machine crashes, you still remember what you\ndid just before the crash.\n\nThat theory may have been more correct back in the days than it is\nnow. People who use git might be less willing to treat it like a\nfilesystem that you can fsck than I was back 10+ ago.\n\nIt's worth noting that the commit that Junio pointed to (from 2008)\ndidn't actually change any behavior. It just allowed people who cared\nto change behavior. The original \"let's not waste time on fsync every\nobject write, because we can just re-create the state anyway\" behavior\ngoes back to 2005.\n\n Linus\n"},{"id":"336740","messageId":"87efmohy8s.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"CA+55aFzJ2QO0MH3vgbUd8X-dzg_65A-jKmEBMSVt8ST2bpmzSQ@mail.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-17T23:16:51Z","receivedAt":"2018-01-17T23:17:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 17 2018, Linus Torvalds jotted:\n\n> On Wed, Jan 17, 2018 at 1:44 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> I ran a small test myself on CentOS 7 (3.10) with ext4 data=ordered\n>> on the tests I thought might do a lot of loose object writes:\n>>\n>> $ GIT_PERF_REPEAT_COUNT=10 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS=\"NO_OPENSSL=Y CFLAGS=-O3 -j56\" ./run origin/master fsync-on~ fsync-on p3400-rebase.sh p0007-write-cache.sh\n>> [...]\n>> Test fsync-on~ fsync-on\n>> -------------------------------------------------------------------------------------------------------\n>> 3400.2: rebase on top of a lot of unrelated changes 1.45(1.30+0.17) 1.45(1.28+0.20) +0.0%\n>> 3400.4: rebase a lot of unrelated changes without split-index 4.34(3.71+0.66) 4.33(3.69+0.66) -0.2%\n>> 3400.6: rebase a lot of unrelated changes with split-index 3.38(2.94+0.47) 3.38(2.93+0.47) +0.0%\n>> 0007.2: write_locked_index 3 times (3214 files) 0.01(0.00+0.00) 0.01(0.00+0.00) +0.0%\n>>\n>> No impact. However I did my own test of running the test suite 10%\n>> times with/without this patch, and it runs 9% slower:\n\nThat should be \"10 times\" b.t.w., not \"10% times\"\n\n>>\n>> fsync-off: avg:21.59 21.50 21.50 21.52 21.53 21.54 21.57 21.59 21.61 21.63 21.95\n>> fsync-on: avg:23.43 23.21 23.25 23.26 23.26 23.27 23.32 23.49 23.51 23.83 23.88\n>\n> That's not the thing you should check.\n>\n> Now re-do the test while another process writes to a totally unrelated\n> a huge file (say, do a ISO file copy or something).\n>\n> That was the thing that several filesystems get completely and\n> horribly wrong. Generally _particularly_ the logging filesystems that\n> don't even need the fsync, because they use a single log for\n> everything (so fsync serializes all the writes, not just the writes to\n> the one file it's fsync'ing).\n>\n> The original git design was very much to write each object file\n> without any syncing, because they don't matter since a new object file\n> - by definition - isn't really reachable. Then sync before writing the\n> index file or a new ref.\n>>\n> But things have changed, I'm not arguing that the code shouldn't be\n> made safe by default. I personally refuse to use rotating media on my\n> machines anyway, largely exactly because of the fsync() issue (things\n> like \"firefox\" started doing fsync on the mysql database for stupid\n> things, and you'd get huge pauses).\n>\n> But I do think your benchmark is wrong. The case where only git does\n> something is not interesting or relevant. It really is \"git does\n> something _and_ somebody else writes something entirely unrelated at\n> the same time\" that matters.\n\nYeah it's shitty, just a quick hack to get some since there was a\ndiscussion of performance, but neither your original patch or this\nthread had quoted any.\n\nOne thing you may have missed is that this is a parallel (56 tests at a\ntime) run of the full test suite. So there's plenty of other git\nprocesses (and test setup/teardown) racing with any given git\nprocess. Running the test suite in a loop like this gives me ~100K IO\nops/s & ~50% disk utilization.\n\nOr does overall FS activity and raw throughput (e.g. with an ISO copy)\nmatter more than general FS contention?\n\nTweaking it to emulate this iso copy case, running another test with one\nof these running concurrently:\n\n # setup\n dd if=/dev/urandom of=/tmp/fake.iso bs=1024 count=$((1000*1024))\n # run in a loop (shuf to not always write the same thing)\n while sleep 0.1; do shuf /tmp/fake.iso | pv >/tmp/fake.shuf.iso; done\n\nGives throughput that spikes to 100% (not consistently) and:\n\n fsync-off: avg:36.37 31.74 33.83 35.12 36.19 36.32 37.04 37.34 37.71 37.93 40.43\n fsync-on: avg:38.09 34.56 35.14 35.69 36.41 36.41 37.96 38.25 40.45 41.44 44.59\n\n~4.7% slower, v.s. ~8.5% in my earlier\n87h8rki2iu.fsf@evledraar.gmail.com without that running.\n\nWhich is not an argument for / against this patch, but those numbers\nseem significant, and generally if the entire test suite slows down by\nthat much there's going to be sub-parts of it that are much worse.\n\nWhich might be a reason to tread more carefully and if it *does* slow\nthings down perhaps do it with more granularity, e.g. turning it on in\ngit-receive-pack might be more sensible than in git-filter-branch.\n\nI remember Roberto Tyley's BFG writing an amazing amount of loose\nobjects, but it doesn't seem to have an fsync() option, I wonder if\nadding one would be a representative pathological test case.\n"},{"id":"336741","messageId":"CA+55aFyXn-DMhkTFNmHa8QgLYx6Fb0Jr+kkr5v4mRhyT7Pvx=A@mail.gmail.com","threadId":"47626","inReplyTo":"87efmohy8s.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-01-17T23:42:10Z","receivedAt":"2018-01-17T23:42:19Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 17, 2018 at 3:16 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Or does overall FS activity and raw throughput (e.g. with an ISO copy)\n> matter more than general FS contention?\n\nTraditionally, yes.\n\nAlso note that none of this is about \"throughput\". It's about waiting\nfor a second or two when you do a simple \"git commit\" or similar.\n\nAlso, hardware has changed. I haven't used a rotational disk in about\n10 years now, and I don't worry about latencies so much more.\n\nThe fact that you get ~100k iops indicates that you probably aren't\nusing those stupid platters of rust either. So I doubt you can even\ntrigger the bad cases.\n\n Linus\n"},{"id":"336742","messageId":"20180117235220.GD6948@thunk.org","threadId":"47626","inReplyTo":"CA+55aFzJ2QO0MH3vgbUd8X-dzg_65A-jKmEBMSVt8ST2bpmzSQ@mail.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2018-01-17T23:52:20Z","receivedAt":"2018-01-17T23:52:31Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Wed, Jan 17, 2018 at 02:07:22PM -0800, Linus Torvalds wrote:\n> \n> Now re-do the test while another process writes to a totally unrelated\n> a huge file (say, do a ISO file copy or something).\n> \n> That was the thing that several filesystems get completely and\n> horribly wrong. Generally _particularly_ the logging filesystems that\n> don't even need the fsync, because they use a single log for\n> everything (so fsync serializes all the writes, not just the writes to\n> the one file it's fsync'ing).\n\nWell, let's be fair; this is something *ext3* got wrong, and it was\nthe default file system back them. All of the modern file systems now\ndo delayed allocation, which means that an fsync of one file doesn't\nactually imply an fsync of another file. Hence...\n\n> The original git design was very much to write each object file\n> without any syncing, because they don't matter since a new object file\n> - by definition - isn't really reachable. Then sync before writing the\n> index file or a new ref.\n\nThis isn't really safe any more. Yes, there's a single log. But\nfiles which are subject to delayed allocation are in the page cache,\nand just because you fsync the index file doesn't mean that the object\nfile is now written to disk. It was true for ext3, but it's not true\nfor ext4, xfs, btrfs, etc.\n\nThe good news is that if you have another process downloading a huge\nISO image, the fsync of the index file won't force the ISO file to be\nwritten out. The bad news is that it won't force out the other git\nobject files, either.\n\nNow, there is a potential downside of fsync'ing each object file, and\nthat is the cost of doing a CACHE FLUSH on a HDD is non-trivial, and\neven on a SSD, it's not optimal to call CACHE FLUSH thousands of times\nin a second. So if you are creating thousands of tiny files, and you\nfsync each one, each fsync(2) call is a serializing instruction, which\nmeans it won't return until that one file is written to disk. If you\nare writing lots of small files, and you are using a HDD, you'll be\nbottlenecked to around 30 files per second on a 5400 RPM HDD, and this\nis true regardless of what file system you use, because the bottle\nneck is the CACHE FLUSH operation, and how you organize the metadata\nand how you do the block allocation, is largely lost in the noise\ncompared to the CACHE FLUSH command, which serializes everything.\n\nThere are solutions to this; you could simply not call fsync(2) a\nthousand times, and instead write a pack file, and call fsync once on\nthe pack file. That's probably the smartest approach.\n\nYou could also create a thousand threads, and call fsync(2) on those\nthousand threads at roughly the same time. Or you could use a\nbleeding edge kernel with the latest AIO patch, and use the newly\nadded IOCB_CMD_FSYNC support.\n\nBut I'd simply recommend writing a pack and fsync'ing the pack,\ninstead of trying to write a gazillion object files. (git-repack -A,\nI'm looking at you....)\n\n\t\t\t\t\t- Ted\n"},{"id":"336743","messageId":"CA+55aFxgg6MT5Z+Jox2xyG28g9jNJ4cL3jNZ5AgTOmUODuiBsA@mail.gmail.com","threadId":"47626","inReplyTo":"20180117235220.GD6948@thunk.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-01-17T23:57:53Z","receivedAt":"2018-01-17T23:58:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 17, 2018 at 3:52 PM, Theodore Ts'o wrote:\n>\n> Well, let's be fair; this is something *ext3* got wrong, and it was\n> the default file system back them.\n\nI'm pretty sure reiserfs and btrfs did too..\n\n Linus\n"},{"id":"336803","messageId":"20180118162721.GA26078@lst.de","threadId":"47626","inReplyTo":"CA+55aFxgg6MT5Z+Jox2xyG28g9jNJ4cL3jNZ5AgTOmUODuiBsA@mail.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2018-01-18T16:27:21Z","receivedAt":"2018-01-18T16:27:28Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"[adding Chris to the Cc list - this is about the awful ext3 data=ordered\nbehavior of syncing the whole file system data and metadata on each\nfsync]\n\nOn Wed, Jan 17, 2018 at 03:57:53PM -0800, Linus Torvalds wrote:\n> On Wed, Jan 17, 2018 at 3:52 PM, Theodore Ts'o wrote:\n> >\n> > Well, let's be fair; this is something *ext3* got wrong, and it was\n> > the default file system back them.\n> \n> I'm pretty sure reiserfs and btrfs did too..\n\nI'm pretty sure btrfs never did, and reiserfs at least looks like\nit currently doesn't but I'd have to dig into history to check if\nit ever did.\n"},{"id":"336924","messageId":"xmqqzi59psxt.fsf@gitster.mtv.corp.google.com","threadId":"47626","inReplyTo":"20180118162721.GA26078@lst.de","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-19T19:08:46Z","receivedAt":"2018-01-19T19:08:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christoph Hellwig writes:\n\n> [adding Chris to the Cc list - this is about the awful ext3 data=ordered\n> behavior of syncing the whole file system data and metadata on each\n> fsync]\n>\n> On Wed, Jan 17, 2018 at 03:57:53PM -0800, Linus Torvalds wrote:\n>> On Wed, Jan 17, 2018 at 3:52 PM, Theodore Ts'o wrote:\n>> >\n>> > Well, let's be fair; this is something *ext3* got wrong, and it was\n>> > the default file system back them.\n>> \n>> I'm pretty sure reiserfs and btrfs did too..\n>\n> I'm pretty sure btrfs never did, and reiserfs at least looks like\n> it currently doesn't but I'd have to dig into history to check if\n> it ever did.\n\nSo..., is it fair to say that the one you sent in\n\n https://public-inbox.org/git/20180117193510.GA30657@lst.de/\n\nis the best variant we have seen in this thread so far? I'll keep\nthat in my inbox so that I do not forget, but I think we would want\nto deal with a hotfix for 2.16 on case insensitive platforms before\nthis topic.\n\nThanks.\n\n"},{"id":"336996","messageId":"20180120221445.GA4451@thunk.org","threadId":"47626","inReplyTo":"xmqqzi59psxt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2018-01-20T22:14:45Z","receivedAt":"2018-01-20T22:14:59Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Fri, Jan 19, 2018 at 11:08:46AM -0800, Junio C Hamano wrote:\n> So..., is it fair to say that the one you sent in\n> \n> https://public-inbox.org/git/20180117193510.GA30657@lst.de/\n> \n> is the best variant we have seen in this thread so far? I'll keep\n> that in my inbox so that I do not forget, but I think we would want\n> to deal with a hotfix for 2.16 on case insensitive platforms before\n> this topic.\n\nIt's a simplistic fix, but it will work. There may very well be\ncertain workloads which generate a large number of loose objects\n(e.g., git repack -A) which will make things go significantly more\nslowly as a result. It might very well be the case that if nothing\nelse is going on, something like \"write all the files without\nfsync(2), then use syncfs(2)\" would be much faster. The downside with\nthat approach is if indeed you were downloading a multi-gigabyte DVD\nimage at the same time, the syncfs(2) will force a writeback of the\npartially writte DVD image, or some other unrelated files.\n\nBut if the goal is to just change the default, and then see what\nshakes out, and then apply other optimizations later, that's certainly\na valid result. I've never been fond of the \"git repack -A\" behavior\nwhere it can generate huge numbers of loose files. I'd much prefer it\nif the other objects ended up in a separate pack file, and then some\nother provision made for nuking that pack file some time later. But\nthat's expanding the scope significantly over what's currently being\ndiscussed.\n\n\t\t\t\t\t\t- Ted\n"},{"id":"336999","messageId":"xmqqefmknp3f.fsf@gitster.mtv.corp.google.com","threadId":"47626","inReplyTo":"20180120221445.GA4451@thunk.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-20T22:27:00Z","receivedAt":"2018-01-20T22:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Theodore Ts'o writes:\n\n> .... I've never been fond of the \"git repack -A\" behavior\n> where it can generate huge numbers of loose files. I'd much prefer it\n> if the other objects ended up in a separate pack file, and then some\n> other provision made for nuking that pack file some time later....\n\nYes, a \"cruft pack\" that holds unreachable object has been discussed\na few times recently on list, and I do agree that it is a desirable\nthing to have in the longer run.\n\nWhat's tricky is to devise a way to allow us to salvage objects that\nare placed in a cruft pack because they are accessed recently,\nproving themselves to be no longer crufts. It could be that a good\nway to resurrect them is to explode them to loose form when they are\naccessed out of a cruft pack. We need to worry about interactions\nwith read-only users if we go that route, but with the current\n\"explode unreachable to loose, touch their mtime when they are\naccessed\" scheme ends up ignoring accesses from read-only users that\ncannot update mtime, so it might not be too bad.\n\n"},{"id":"337036","messageId":"f797983e-1890-d84c-c3c4-87904a0a8135@fb.com","threadId":"47626","inReplyTo":"20180118162721.GA26078@lst.de","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Chris Mason","fromEmail":"clm@fb.com","sentAt":"2018-01-21T21:32:50Z","receivedAt":"2018-01-21T21:34:19Z","isPatch":true,"sender":{"key":"clm@fb.com","avatar":null},"body":"On 01/18/2018 11:27 AM, Christoph Hellwig wrote:\n> [adding Chris to the Cc list - this is about the awful ext3 data=ordered\n> behavior of syncing the whole file system data and metadata on each\n> fsync]\n> \n> On Wed, Jan 17, 2018 at 03:57:53PM -0800, Linus Torvalds wrote:\n>> On Wed, Jan 17, 2018 at 3:52 PM, Theodore Ts'o wrote:\n>>>\n>>> Well, let's be fair; this is something *ext3* got wrong, and it was\n>>> the default file system back them.\n>>\n>> I'm pretty sure reiserfs and btrfs did too..\n> \n> I'm pretty sure btrfs never did, and reiserfs at least looks like\n> it currently doesn't but I'd have to dig into history to check if\n> it ever did.\n> \n\nChristoph has this right, btrfs only fsyncs the one file that you've \nasked for, and unrelated data/metadata won't be included.\n\nWe've seen big fsync stalls on ext4 caused by data=ordered, so it's \nstill possible to trigger on ext4, but much better than ext3.\n\nI do share Ted's concern about the perf impact of the fsyncs on tons of \nloose files, but the defaults should be safety first.\n\n-chris\n"},{"id":"337091","messageId":"871siihqvw.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"xmqqefmknp3f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-22T15:09:23Z","receivedAt":"2018-01-22T15:09:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 20 2018, Junio C. Hamano jotted:\n\n> Theodore Ts'o writes:\n>\n>> .... I've never been fond of the \"git repack -A\" behavior\n>> where it can generate huge numbers of loose files. I'd much prefer it\n>> if the other objects ended up in a separate pack file, and then some\n>> other provision made for nuking that pack file some time later....\n>\n> Yes, a \"cruft pack\" that holds unreachable object has been discussed\n> a few times recently on list, and I do agree that it is a desirable\n> thing to have in the longer run.\n>\n> What's tricky is to devise a way to allow us to salvage objects that\n> are placed in a cruft pack because they are accessed recently,\n> proving themselves to be no longer crufts. It could be that a good\n> way to resurrect them is to explode them to loose form when they are\n> accessed out of a cruft pack. We need to worry about interactions\n> with read-only users if we go that route, but with the current\n> \"explode unreachable to loose, touch their mtime when they are\n> accessed\" scheme ends up ignoring accesses from read-only users that\n> cannot update mtime, so it might not be too bad.\n\nWouldn't it also make gc pruning more expensive? Now you can repack\nregularly and loose objects will be left out of the pack, and then just\nrm'd, whereas now it would entail creating new packs (unless the whole\npack was objects meant for removal).\n\nProbably still worth it, but something to keep in mind.\n"},{"id":"337097","messageId":"20180122180903.GB3513@thunk.org","threadId":"47626","inReplyTo":"871siihqvw.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2018-01-22T18:09:03Z","receivedAt":"2018-01-22T18:09:14Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Mon, Jan 22, 2018 at 04:09:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > What's tricky is to devise a way to allow us to salvage objects that\n> > are placed in a cruft pack because they are accessed recently,\n> > proving themselves to be no longer crufts. It could be that a good\n> > way to resurrect them is to explode them to loose form when they are\n> > accessed out of a cruft pack. We need to worry about interactions\n> > with read-only users if we go that route, but with the current\n> > \"explode unreachable to loose, touch their mtime when they are\n> > accessed\" scheme ends up ignoring accesses from read-only users that\n> > cannot update mtime, so it might not be too bad.\n> \n> Wouldn't it also make gc pruning more expensive? Now you can repack\n> regularly and loose objects will be left out of the pack, and then just\n> rm'd, whereas now it would entail creating new packs (unless the whole\n> pack was objects meant for removal).\n\nThe idea is that the cruft pack would be all objects that were no\nlonger referenced. Hence the proposal that if they ever *are*\naccessed, they would be exploded to a loose object at that point. So\nin the common case, the GC would go quickly since the entire pack\ncould just be rm'ed once it hit the designated expiry time.\n\nAnother way of doing things would be to use the mtime of the cruft\npack for the expiry time, and if the curft pack is ever referenced,\nits mtime would get updated. Yet a third way would be to simply clear\nthe \"cruft\" bit if it ever *is* referenced. In the common case, it\nwould never be referenced, so it could just get deleted, but in the\ncase where the user has manually \"rescued\" a set of commits (perhaps\nby explicitly setting a branch head to commit id found from a reflog),\nthe objects would be saved.\n\nSo there are many ways it could be managed.\n\n\t\t\t\t\t\t\t- Ted\n"},{"id":"337136","messageId":"20180123002513.GE26357@sigill.intra.peff.net","threadId":"47626","inReplyTo":"871siihqvw.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-23T00:25:13Z","receivedAt":"2018-01-23T00:25:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 22, 2018 at 04:09:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Yes, a \"cruft pack\" that holds unreachable object has been discussed\n> > a few times recently on list, and I do agree that it is a desirable\n> > thing to have in the longer run.\n> >\n> > What's tricky is to devise a way to allow us to salvage objects that\n> > are placed in a cruft pack because they are accessed recently,\n> > proving themselves to be no longer crufts. It could be that a good\n> > way to resurrect them is to explode them to loose form when they are\n> > accessed out of a cruft pack. We need to worry about interactions\n> > with read-only users if we go that route, but with the current\n> > \"explode unreachable to loose, touch their mtime when they are\n> > accessed\" scheme ends up ignoring accesses from read-only users that\n> > cannot update mtime, so it might not be too bad.\n> \n> Wouldn't it also make gc pruning more expensive? Now you can repack\n> regularly and loose objects will be left out of the pack, and then just\n> rm'd, whereas now it would entail creating new packs (unless the whole\n> pack was objects meant for removal).\n> \n> Probably still worth it, but something to keep in mind.\n\nThat's a good point. I think it would be OK in practice, though, since\nnormal operations don't tend to create a huge number of unreachable\nloose objects (at least compared to the _reachable_ loose objects, which\nwe're already dealing with). We tend to get unbalanced explosions of\nloose objects only because a huge chunk of packed history expired.\n\nIt is something to keep in mind when implementing the scheme, though.\nLuckily we already have the right behavior implemented via\n--pack-loose-unreachable (which is used for \"repack -k\" currently), so I\nthink it would just be a matter of passing the right flags from\ngit-repack.\n\n-Peff\n"},{"id":"337137","messageId":"20180123004710.GF26357@sigill.intra.peff.net","threadId":"47626","inReplyTo":"20180122180903.GB3513@thunk.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-23T00:47:10Z","receivedAt":"2018-01-23T00:47:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 22, 2018 at 01:09:03PM -0500, Theodore Ts'o wrote:\n\n> > Wouldn't it also make gc pruning more expensive? Now you can repack\n> > regularly and loose objects will be left out of the pack, and then just\n> > rm'd, whereas now it would entail creating new packs (unless the whole\n> > pack was objects meant for removal).\n> \n> The idea is that the cruft pack would be all objects that were no\n> longer referenced. Hence the proposal that if they ever *are*\n> accessed, they would be exploded to a loose object at that point. So\n> in the common case, the GC would go quickly since the entire pack\n> could just be rm'ed once it hit the designated expiry time.\n\nI think Ævar is talking about the case of:\n\n 1. You make 100 objects that aren't referenced. They're loose.\n\n 2. You run git-gc. They're still too recent to be deleted.\n\nRight now those recent loose objects sit loose, and have zero cost at\nthe time of gc. In a \"cruft pack\" world, you'd pay some I/O to copy\nthem into the cruft pack, and some CPU to zlib and delta-compress them.\nI think that's probably fine, though.\n\nThat said, some of what you wrote left me confused, and whether we're\nall talking about the same idea. ;) Let me describe the idea I had\nmentioned in another thread. Right now the behavior is basically this:\n\nIf an unreachable object becomes referenced, it doesn't immediately get\nexploded. During the next gc, whatever new object referenced them would\nbe one of:\n\n 1. Reachable from refs, in which case it carries along the\n formerly-cruft object into the new pack, since it is now also\n reachable.\n\n 2. Unreachable but still recent by mtime; we keep such objects, and\n anything they reference (now as unreachable, in this proposal in\n the cruft pack). Now these get either left loose, or exploded loose\n if they were previously packed.\n\n 3. Unreachable and old. Both objects can be dropped totally.\n\nThe current strategy is to use the mtimes for \"recent\", and we use the\npack's mtime for every object in the pack.\n\nSo if we pack all the loose objects into a cruft pack, the mtime of the\ncruft pack becomes the new gauge for \"recent\". And if we migrate objects\nfrom old cruft pack to new cruft pack at each gc, then they'll keep\ngetting their mtimes refreshed, and we'll never drop them.\n\nSo we need to either:\n\n - keep per-object mtimes, so that old ones can age out (i.e., they'd\n hit case 3 and just not get migrated to either the new \"real\" pack\n or the new cruft pack).\n\n - keep multiple cruft packs, and let whole packs age out. But then\n cruft objects which get referenced again by other cruft have to get\n copied (not moved!) to new packs. That _probably_ doesn't happen all\n that often, so it might be OK.\n\n> Another way of doing things would be to use the mtime of the cruft\n> pack for the expiry time, and if the curft pack is ever referenced,\n> its mtime would get updated. Yet a third way would be to simply clear\n> the \"cruft\" bit if it ever *is* referenced. In the common case, it\n> would never be referenced, so it could just get deleted, but in the\n> case where the user has manually \"rescued\" a set of commits (perhaps\n> by explicitly setting a branch head to commit id found from a reflog),\n> the objects would be saved.\n\nI don't think we have to worry about \"rescued\" objects. Those are\nreachable, so they'd get copied into the new \"real\" pack (and then their\ncruft pack eventually deleted).\n\n-Peff\n"},{"id":"337145","messageId":"20180123054553.GA21015@thunk.org","threadId":"47626","inReplyTo":"20180123004710.GF26357@sigill.intra.peff.net","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2018-01-23T05:45:53Z","receivedAt":"2018-01-23T05:46:06Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Mon, Jan 22, 2018 at 07:47:10PM -0500, Jeff King wrote:\n> \n> I think Ævar is talking about the case of:\n> \n> 1. You make 100 objects that aren't referenced. They're loose.\n> \n> 2. You run git-gc. They're still too recent to be deleted.\n> \n> Right now those recent loose objects sit loose, and have zero cost at\n> the time of gc. In a \"cruft pack\" world, you'd pay some I/O to copy\n> them into the cruft pack, and some CPU to zlib and delta-compress them.\n> I think that's probably fine, though.\n\nI wasn't assuming that git-gc would create a cruft pack --- although I\nguess it could. As you say, recent loose objects have relatively zero\ncost at the time of gc. To the extent that the gc has to read lots of\nloose files, there may be more seeks in the cold cache case, so there\nis actually *some* cost to having the loose objects, but it's not\ngreat.\n\nWhat I was thinking about instead is that in cases where we know we\nare likely to be creating a large number of loose objects (whether\nthey referenced or not), in a world where we will be calling fsync(2)\nafter every single loose object being created, pack files start\nlooking *way* more efficient. So in general, if you know you will be\ncreating N loose objects, where N is probably around 50 or so, you'll\nwant to create a pack instead.\n\nOne of those cases is \"repack -A\", and in that case the loose objects\nare all going tobe not referenced, so it would be a \"cruft pack\". But\nin many other cases where we might be importing from another DCVS,\nwhich will be another case where doing an fsync(2) after every loose\nobject creation (and where I have sometimes seen it create them *all*\nloose, and not use a pack at all), is going to get extremely slow and\npainful.\n\n> So if we pack all the loose objects into a cruft pack, the mtime of the\n> cruft pack becomes the new gauge for \"recent\". And if we migrate objects\n> from old cruft pack to new cruft pack at each gc, then they'll keep\n> getting their mtimes refreshed, and we'll never drop them.\n\nWell, I was assuming that gc would be a special case which doesn't the\nmtime of the old cruft pack. (Or more generally, any time an object\nis gets copied out of the cruft pack, either to a loose object, or to\nanother pack, the mtime on the source pack should not be touched.)\n\n\t \t \t \t \t \t - Ted\n"},{"id":"337156","messageId":"20180123161738.GC13068@sigill.intra.peff.net","threadId":"47626","inReplyTo":"20180123054553.GA21015@thunk.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-23T16:17:38Z","receivedAt":"2018-01-23T16:17:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 23, 2018 at 12:45:53AM -0500, Theodore Ts'o wrote:\n\n> What I was thinking about instead is that in cases where we know we\n> are likely to be creating a large number of loose objects (whether\n> they referenced or not), in a world where we will be calling fsync(2)\n> after every single loose object being created, pack files start\n> looking *way* more efficient. So in general, if you know you will be\n> creating N loose objects, where N is probably around 50 or so, you'll\n> want to create a pack instead.\n> \n> One of those cases is \"repack -A\", and in that case the loose objects\n> are all going tobe not referenced, so it would be a \"cruft pack\". But\n> in many other cases where we might be importing from another DCVS,\n> which will be another case where doing an fsync(2) after every loose\n> object creation (and where I have sometimes seen it create them *all*\n> loose, and not use a pack at all), is going to get extremely slow and\n> painful.\n\nAh, I see. I think in the general case of git operations this is hard\n(because most object writes don't realize the larger operation that\nthey're a part of). But I agree that those two are the low-hanging fruit\n(imports should already be using fast-import, and \"cruft packs\" are not\ntoo hard an idea to implement).\n\nI agree that a cruft-pack implementation could just be for \"repack -A\",\nand does not have to collect otherwise loose objects. I think part of my\nconfusion was that you and I are coming to the idea from different\nangles: you care about minimizing fsyncs, and I'm interested in stopping\nthe problem where you have too many loose objects after running auto-gc.\nSo I care more about collecting those loose objects for that case.\n\n> > So if we pack all the loose objects into a cruft pack, the mtime of the\n> > cruft pack becomes the new gauge for \"recent\". And if we migrate objects\n> > from old cruft pack to new cruft pack at each gc, then they'll keep\n> > getting their mtimes refreshed, and we'll never drop them.\n> \n> Well, I was assuming that gc would be a special case which doesn't the\n> mtime of the old cruft pack. (Or more generally, any time an object\n> is gets copied out of the cruft pack, either to a loose object, or to\n> another pack, the mtime on the source pack should not be touched.)\n\nRight, that's the \"you have multiple cruft packs\" idea which has been\ndiscussed[1] (each one just hangs around until its mtime expires, and\nmay duplicate objects found elsewhere).\n\nThat does end up with one pack per gc, which just punts the \"too many\nloose objects\" to \"too many packs\". But unless the number of gc runs you\ndo is very high compared to the expiration time, we can probably ignore\nthat.\n\n-Peff\n\n[1] https://public-inbox.org/git/20170610080626.sjujpmgkli4muh7h@sigill.intra.peff.net/\n"},{"id":"405737","messageId":"87sgbghdbp.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"20180117235220.GD6948@thunk.org","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-17T11:06:50Z","receivedAt":"2020-09-17T11:07:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 18 2018, Theodore Ts'o wrote:\n\n> On Wed, Jan 17, 2018 at 02:07:22PM -0800, Linus Torvalds wrote:\n>> \n>> Now re-do the test while another process writes to a totally unrelated\n>> a huge file (say, do a ISO file copy or something).\n>> \n>> That was the thing that several filesystems get completely and\n>> horribly wrong. Generally _particularly_ the logging filesystems that\n>> don't even need the fsync, because they use a single log for\n>> everything (so fsync serializes all the writes, not just the writes to\n>> the one file it's fsync'ing).\n>\n> Well, let's be fair; this is something *ext3* got wrong, and it was\n> the default file system back them. All of the modern file systems now\n> do delayed allocation, which means that an fsync of one file doesn't\n> actually imply an fsync of another file. Hence...\n>\n>> The original git design was very much to write each object file\n>> without any syncing, because they don't matter since a new object file\n>> - by definition - isn't really reachable. Then sync before writing the\n>> index file or a new ref.\n>\n> This isn't really safe any more. Yes, there's a single log. But\n> files which are subject to delayed allocation are in the page cache,\n> and just because you fsync the index file doesn't mean that the object\n> file is now written to disk. It was true for ext3, but it's not true\n> for ext4, xfs, btrfs, etc.\n>\n> The good news is that if you have another process downloading a huge\n> ISO image, the fsync of the index file won't force the ISO file to be\n> written out. The bad news is that it won't force out the other git\n> object files, either.\n>\n> Now, there is a potential downside of fsync'ing each object file, and\n> that is the cost of doing a CACHE FLUSH on a HDD is non-trivial, and\n> even on a SSD, it's not optimal to call CACHE FLUSH thousands of times\n> in a second. So if you are creating thousands of tiny files, and you\n> fsync each one, each fsync(2) call is a serializing instruction, which\n> means it won't return until that one file is written to disk. If you\n> are writing lots of small files, and you are using a HDD, you'll be\n> bottlenecked to around 30 files per second on a 5400 RPM HDD, and this\n> is true regardless of what file system you use, because the bottle\n> neck is the CACHE FLUSH operation, and how you organize the metadata\n> and how you do the block allocation, is largely lost in the noise\n> compared to the CACHE FLUSH command, which serializes everything.\n>\n> There are solutions to this; you could simply not call fsync(2) a\n> thousand times, and instead write a pack file, and call fsync once on\n> the pack file. That's probably the smartest approach.\n>\n> You could also create a thousand threads, and call fsync(2) on those\n> thousand threads at roughly the same time. Or you could use a\n> bleeding edge kernel with the latest AIO patch, and use the newly\n> added IOCB_CMD_FSYNC support.\n>\n> But I'd simply recommend writing a pack and fsync'ing the pack,\n> instead of trying to write a gazillion object files. (git-repack -A,\n> I'm looking at you....)\n>\n> \t\t\t\t\t- Ted\n\n[I didn't find an ideal message to reply to in this thread, but this\nseemed to probably be the best]\n\nJust an update on this since I went back and looked at this thread,\nGitLab about ~1yr ago turned on core.fsyncObjectFiles=true by\ndefault.\n\nThe reason is detailed in [1], tl;dr: empty loose object file issue on\next4 allegedly caused by a lack of core.fsyncObjectFiles=true, but I\ndidn't do any root cause analysis. Just noting it here for for future\nreference.\n\n1. https://gitlab.com/gitlab-org/gitlab-foss/-/issues/51680#note_180508774\n"},{"id":"405738","messageId":"20200917112830.26606-3-avarab@gmail.com","threadId":"47626","inReplyTo":"87sgbghdbp.fsf@evledraar.gmail.com","subject":"[RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-17T11:28:30Z","receivedAt":"2020-09-17T11:29:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As amusing as Linus's original prose[1] is here it doesn't really explain\nin any detail to the uninitiated why you would or wouldn't enable\nthis, and the counter-intuitive reason for why git wouldn't fsync your\nprecious data.\n\nSo elaborate (a lot) on why this may or may not be needed. This is my\nbest-effort attempt to summarize the various points raised in the last\nML[2] discussion about this.\n\n1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n files\", 2008-06-18)\n2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n 1 file changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 74619a9c03..5b47670c16 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -548,12 +548,42 @@ core.whitespace::\n errors. The default tab width is 8. Allowed values are 1 to 63.\n \n core.fsyncObjectFiles::\n-\tThis boolean will enable 'fsync()' when writing object files.\n-+\n-This is a total waste of time and effort on a filesystem that orders\n-data writes properly, but can be useful for filesystems that do not use\n-journalling (traditional UNIX filesystems) or that only journal metadata\n-and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n+\tThis boolean will enable 'fsync()' when writing loose object\n+\tfiles. Both the file itself and its containng directory will\n+\tbe fsynced.\n++\n+When git writes data any required object writes will precede the\n+corresponding reference update(s). For example, a\n+linkgit:git-receive-pack[1] accepting a push might write a pack or\n+loose objects (depending on settings such as `transfer.unpackLimit`).\n++\n+Therefore on a journaled file system which ensures that data is\n+flushed to disk in chronological order an fsync shouldn't be\n+needed. The loose objects might be lost with a crash, but so will the\n+ref update that would have referenced them. Git's own state in such a\n+crash will remain consistent.\n++\n+This option exists because that assumption doesn't hold on filesystems\n+where the data ordering is not preserved, such as on ext3 and ext4\n+with \"data=writeback\". On such a filesystem the `rename()` that drops\n+the new reference in place might be preserved, but the contents or\n+directory entry for the loose object(s) might not have been synced to\n+disk.\n++\n+Enabling this option might slow git down by a lot in some\n+cases. E.g. in the case of a naïve bulk import tool which might create\n+a million loose objects before a final ref update and `gc`. In other\n+more common cases such as on a server being pushed to with default\n+`transfer.unpackLimit` settings the difference might not be noticable.\n++\n+However, that's highly filesystem-dependent, on some filesystems\n+simply calling fsync() might force an unrelated bulk background write\n+to be serialized to disk. Such edge cases are the reason this option\n+is off by default. That default setting might change in future\n+versions.\n++\n+In older versions of git only the descriptor for the file itself was\n+fsynced, not its directory entry.\n \n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\n-- \n2.28.0.297.g1956fa8f8d\n\n"},{"id":"405739","messageId":"20200917112830.26606-1-avarab@gmail.com","threadId":"47626","inReplyTo":"87sgbghdbp.fsf@evledraar.gmail.com","subject":"[RFC PATCH 0/2] should core.fsyncObjectFiles fsync the dir entry + docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-17T11:28:28Z","receivedAt":"2020-09-17T11:30:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I was re-reading the\nhttps://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/ thread\ntoday and thought we should at least update the docs, and per my\nearlier E-Mail in\nhttps://lore.kernel.org/git/87sgbghdbp.fsf@evledraar.gmail.com/\nperhaps the directory entry should also be synced.\n\nI kept linux-fsdevel@vger.kernel.org in the CC, it was in the original\nthread, but more importantly it would be really nice to have people\nwho know more about the state of filesystems on Linux and other OS's\nto give 2/2 a read to see how accurate what I put together is.\n\nÆvar Arnfjörð Bjarmason (2):\n sha1-file: fsync() loose dir entry when core.fsyncObjectFiles\n core.fsyncObjectFiles: make the docs less flippant\n\n Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n sha1-file.c | 19 +++++++++++-----\n 2 files changed, 50 insertions(+), 11 deletions(-)\n\n-- \n2.28.0.297.g1956fa8f8d\n\n"},{"id":"405740","messageId":"20200917112830.26606-2-avarab@gmail.com","threadId":"47626","inReplyTo":"87sgbghdbp.fsf@evledraar.gmail.com","subject":"[RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-17T11:28:29Z","receivedAt":"2020-09-17T11:35:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the behavior of core.fsyncObjectFiles to also sync the\ndirectory entry. I don't have a case where this broke, just going by\nparanoia and the fsync(2) manual page's guarantees about its behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sha1-file.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex dd65bd5c68..d286346921 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n }\n \n /* Finalize a file on disk, and close it. */\n-static void close_loose_object(int fd)\n+static void close_loose_object(int fd, const struct strbuf *dirname)\n {\n-\tif (fsync_object_files)\n+\tint dirfd;\n+\tif (fsync_object_files) {\n \t\tfsync_or_die(fd, \"loose object file\");\n+\t\tdirfd = xopen(dirname->buf, O_RDONLY);\n+\t\tfsync_or_die(dirfd, \"loose object directory\");\n+\t}\n \tif (close(fd) != 0)\n \t\tdie_errno(_(\"error when closing loose object file\"));\n }\n@@ -1808,12 +1812,15 @@ static inline int directory_size(const char *filename)\n * We want to avoid cross-directory filename renames, because those\n * can have problems on various filesystems (FAT, NFS, Coda).\n */\n-static int create_tmpfile(struct strbuf *tmp, const char *filename)\n+static int create_tmpfile(struct strbuf *tmp,\n+\t\t\t const char *filename,\n+\t\t\t struct strbuf *dirname)\n {\n \tint fd, dirlen = directory_size(filename);\n \n \tstrbuf_reset(tmp);\n \tstrbuf_add(tmp, filename, dirlen);\n+\tstrbuf_add(dirname, filename, dirlen);\n \tstrbuf_addstr(tmp, \"tmp_obj_XXXXXX\");\n \tfd = git_mkstemp_mode(tmp->buf, 0444);\n \tif (fd < 0 && dirlen && errno == ENOENT) {\n@@ -1848,10 +1855,11 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \tstruct object_id parano_oid;\n \tstatic struct strbuf tmp_file = STRBUF_INIT;\n \tstatic struct strbuf filename = STRBUF_INIT;\n+\tstatic struct strbuf dirname = STRBUF_INIT;\n \n \tloose_object_path(the_repository, &filename, oid);\n \n-\tfd = create_tmpfile(&tmp_file, filename.buf);\n+\tfd = create_tmpfile(&tmp_file, filename.buf, &dirname);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n \t\t\treturn error(_(\"insufficient permission for adding an object to repository database %s\"), get_object_directory());\n@@ -1897,7 +1905,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \t\tdie(_(\"confused by unstable object source data for %s\"),\n \t\t oid_to_hex(oid));\n \n-\tclose_loose_object(fd);\n+\tclose_loose_object(fd, &dirname);\n+\tstrbuf_release(&dirname);\n \n \tif (mtime) {\n \t\tstruct utimbuf utb;\n-- \n2.28.0.297.g1956fa8f8d\n\n"},{"id":"405745","messageId":"20200917131605.GC3024501@coredump.intra.peff.net","threadId":"47626","inReplyTo":"20200917112830.26606-2-avarab@gmail.com","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T13:16:05Z","receivedAt":"2020-09-17T13:20:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the behavior of core.fsyncObjectFiles to also sync the\n> directory entry. I don't have a case where this broke, just going by\n> paranoia and the fsync(2) manual page's guarantees about its behavior.\n\nI've also often wondered whether this is necessary. Given the symptom of\n\"oops, this object is there but with 0 bytes\" after a hard crash (power\noff, etc), my assumption is that the metadata is being journaled but the\nactual data is not. Which would imply this isn't needed, but may just be\nrevealing my naive view of how filesystems work.\n\nAnd of course all of my experience is on ext4 (which doubly confuses me,\nbecause my systems typically have data=ordered, which I thought would\nsolve this). Non-journalling filesystems or other modes likely behave\ndifferently, but if this extra fsync carries a cost, we may want to make\nit optional.\n\n> sha1-file.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n\nWe already fsync pack files, but we don't fsync their directories. If\nthis is important to do, we should be doing it there, too.\n\nWe also don't fsync ref files (nor packed-refs) at all. If fsyncing\nfiles is important for reliability, we should be including those, too.\nIt may be tempting to say that the important stuff is in objects and the\nrefs can be salvaged from the commit graph, but my experience says\notherwise. Missing, broken, or mysteriously-rewound refs cause confusing\nuser-visible behavior, and when compounded with pruning operations like\n\"git gc\" they _do_ result in losing objects.\n\n-Peff\n"},{"id":"405750","messageId":"20200917140912.GA27653@lst.de","threadId":"47626","inReplyTo":"20200917112830.26606-2-avarab@gmail.com","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2020-09-17T14:09:12Z","receivedAt":"2020-09-17T14:20:52Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change the behavior of core.fsyncObjectFiles to also sync the\n> directory entry. I don't have a case where this broke, just going by\n> paranoia and the fsync(2) manual page's guarantees about its behavior.\n\nIt is not just paranoia, but indeed what is required from the standards\nPOV. At least for many Linux file systems your second fsync will be\nvery cheap (basically a NULL syscall) as the log has alredy been forced\nall the way by the first one, but you can't rely on that.\n\nAcked-by: Christoph Hellwig \n"},{"id":"405751","messageId":"20200917141439.GC27653@lst.de","threadId":"47626","inReplyTo":"87sgbghdbp.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2020-09-17T14:14:39Z","receivedAt":"2020-09-17T14:25:34Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Thu, Sep 17, 2020 at 01:06:50PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> The reason is detailed in [1], tl;dr: empty loose object file issue on\n> ext4 allegedly caused by a lack of core.fsyncObjectFiles=true, but I\n> didn't do any root cause analysis. Just noting it here for for future\n> reference.\n\nAll the modern Linux file systems first write the data, and then only\nwrite the metadata after the data write has finished. So your data might\nhave been partially or fully on disk, but until the transaction to commit\nthe size change and/or extent state change you're not going to be able\nto read it back.\n"},{"id":"405757","messageId":"20200917145523.GB3076467@coredump.intra.peff.net","threadId":"47626","inReplyTo":"20200917140912.GA27653@lst.de","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T14:55:23Z","receivedAt":"2020-09-17T14:55:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 04:09:12PM +0200, Christoph Hellwig wrote:\n\n> On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > Change the behavior of core.fsyncObjectFiles to also sync the\n> > directory entry. I don't have a case where this broke, just going by\n> > paranoia and the fsync(2) manual page's guarantees about its behavior.\n> \n> It is not just paranoia, but indeed what is required from the standards\n> POV. At least for many Linux file systems your second fsync will be\n> very cheap (basically a NULL syscall) as the log has alredy been forced\n> all the way by the first one, but you can't rely on that.\n\nIs it sufficient to fsync() just the surrounding directory? I.e., if I\ndo:\n\n mkdir(\"a\");\n mkdir(\"a/b\");\n open(\"a/b/c\", O_WRONLY);\n\nis it enough to fsync() a descriptor pointing to \"a/b\", or should I\nalso do \"a\"?\n\n-Peff\n"},{"id":"405758","messageId":"20200917145653.GA30972@lst.de","threadId":"47626","inReplyTo":"20200917145523.GB3076467@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2020-09-17T14:56:53Z","receivedAt":"2020-09-17T14:57:31Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Thu, Sep 17, 2020 at 10:55:23AM -0400, Jeff King wrote:\n> On Thu, Sep 17, 2020 at 04:09:12PM +0200, Christoph Hellwig wrote:\n> \n> > On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > > Change the behavior of core.fsyncObjectFiles to also sync the\n> > > directory entry. I don't have a case where this broke, just going by\n> > > paranoia and the fsync(2) manual page's guarantees about its behavior.\n> > \n> > It is not just paranoia, but indeed what is required from the standards\n> > POV. At least for many Linux file systems your second fsync will be\n> > very cheap (basically a NULL syscall) as the log has alredy been forced\n> > all the way by the first one, but you can't rely on that.\n> \n> Is it sufficient to fsync() just the surrounding directory? I.e., if I\n> do:\n> \n> mkdir(\"a\");\n> mkdir(\"a/b\");\n> open(\"a/b/c\", O_WRONLY);\n> \n> is it enough to fsync() a descriptor pointing to \"a/b\", or should I\n> also do \"a\"?\n\nYou need to fsync both to be fully compliant, even if just fsyncing b\nwill work for most but not all file systems. The good news is that\nfor those common file systems the extra fsync of a is almost free.\n"},{"id":"405761","messageId":"xmqq4knwto7o.fsf@gitster.c.googlers.com","threadId":"47626","inReplyTo":"87sgbghdbp.fsf@evledraar.gmail.com","subject":"Re: [PATCH] enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T15:30:51Z","receivedAt":"2020-09-17T15:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> [I didn't find an ideal message to reply to in this thread, but this\n> seemed to probably be the best]\n>\n> Just an update on this since I went back and looked at this thread,\n> GitLab about ~1yr ago turned on core.fsyncObjectFiles=true by\n> default.\n>\n> The reason is detailed in [1], tl;dr: empty loose object file issue on\n> ext4 allegedly caused by a lack of core.fsyncObjectFiles=true, but I\n> didn't do any root cause analysis. Just noting it here for for future\n> reference.\n>\n> 1. https://gitlab.com/gitlab-org/gitlab-foss/-/issues/51680#note_180508774\n\nThanks for bringing the original discussion back. I do recall the\ndiscussion and the list of issues to think about raised by Peff back\nthen in [2] is still relevant.\n\n\n2. https://public-inbox.org/git/20180117205509.GA14828@sigill.intra.peff.net/\n\n\n\n"},{"id":"405764","messageId":"xmqqzh5os9cg.fsf@gitster.c.googlers.com","threadId":"47626","inReplyTo":"20200917145653.GA30972@lst.de","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T15:37:19Z","receivedAt":"2020-09-17T15:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christoph Hellwig writes:\n\n> On Thu, Sep 17, 2020 at 10:55:23AM -0400, Jeff King wrote:\n>> On Thu, Sep 17, 2020 at 04:09:12PM +0200, Christoph Hellwig wrote:\n>> \n>> > On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> > > Change the behavior of core.fsyncObjectFiles to also sync the\n>> > > directory entry. I don't have a case where this broke, just going by\n>> > > paranoia and the fsync(2) manual page's guarantees about its behavior.\n>> > \n>> > It is not just paranoia, but indeed what is required from the standards\n>> > POV. At least for many Linux file systems your second fsync will be\n>> > very cheap (basically a NULL syscall) as the log has alredy been forced\n>> > all the way by the first one, but you can't rely on that.\n>> \n>> Is it sufficient to fsync() just the surrounding directory? I.e., if I\n>> do:\n>> \n>> mkdir(\"a\");\n>> mkdir(\"a/b\");\n>> open(\"a/b/c\", O_WRONLY);\n>> \n>> is it enough to fsync() a descriptor pointing to \"a/b\", or should I\n>> also do \"a\"?\n>\n> You need to fsync both to be fully compliant, even if just fsyncing b\n> will work for most but not all file systems. The good news is that\n> for those common file systems the extra fsync of a is almost free.\n\nBack to Ævar's patch, when creating a new loose object, we do these\nthings:\n\n 1. create temporary file and write the compressed contents to it\n while computing its object name\n\n 2. create the fan-out directory under .git/objects/ if needed\n\n 3. mv temporary file to its final name\n\nand the patch adds open+fsync+close on the fan-out directory. In\nthe above exchange with Peff, we learned that open+fsync+close needs\nto be done on .git/objects if we created the fan-out directory, too.\n\nAm I reading the above correctly?\n\nThanks.\n"},{"id":"405766","messageId":"xmqqv9gcs91k.fsf@gitster.c.googlers.com","threadId":"47626","inReplyTo":"20200917112830.26606-3-avarab@gmail.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T15:43:51Z","receivedAt":"2020-09-17T15:44:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> As amusing as Linus's original prose[1] is here it doesn't really explain\n> in any detail to the uninitiated why you would or wouldn't enable\n> this, and the counter-intuitive reason for why git wouldn't fsync your\n> precious data.\n>\n> So elaborate (a lot) on why this may or may not be needed. This is my\n> best-effort attempt to summarize the various points raised in the last\n> ML[2] discussion about this.\n>\n> 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n> files\", 2008-06-18)\n> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n> 1 file changed, 36 insertions(+), 6 deletions(-)\n\nWhen I saw the subject in my mailbox, I expected to see that you\nwould resurrect Christoph's updated text in [*1*], but you wrote a\nwhole lot more ;-) And they are quite informative to help readers to\nunderstand what the option does. I am not sure if the understanding\ndirectly help readers to decide if it is appropriate for their own\nrepositories, though X-<.\n\n\nThanks.\n\n[Reference]\n\n*1* https://public-inbox.org/git/20180117193510.GA30657@lst.de/\n\n>\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index 74619a9c03..5b47670c16 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -548,12 +548,42 @@ core.whitespace::\n> errors. The default tab width is 8. Allowed values are 1 to 63.\n> \n> core.fsyncObjectFiles::\n> -\tThis boolean will enable 'fsync()' when writing object files.\n> -+\n> -This is a total waste of time and effort on a filesystem that orders\n> -data writes properly, but can be useful for filesystems that do not use\n> -journalling (traditional UNIX filesystems) or that only journal metadata\n> -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n> +\tThis boolean will enable 'fsync()' when writing loose object\n> +\tfiles. Both the file itself and its containng directory will\n> +\tbe fsynced.\n> ++\n> +When git writes data any required object writes will precede the\n> +corresponding reference update(s). For example, a\n> +linkgit:git-receive-pack[1] accepting a push might write a pack or\n> +loose objects (depending on settings such as `transfer.unpackLimit`).\n> ++\n> +Therefore on a journaled file system which ensures that data is\n> +flushed to disk in chronological order an fsync shouldn't be\n> +needed. The loose objects might be lost with a crash, but so will the\n> +ref update that would have referenced them. Git's own state in such a\n> +crash will remain consistent.\n> ++\n> +This option exists because that assumption doesn't hold on filesystems\n> +where the data ordering is not preserved, such as on ext3 and ext4\n> +with \"data=writeback\". On such a filesystem the `rename()` that drops\n> +the new reference in place might be preserved, but the contents or\n> +directory entry for the loose object(s) might not have been synced to\n> +disk.\n> ++\n> +Enabling this option might slow git down by a lot in some\n> +cases. E.g. in the case of a naïve bulk import tool which might create\n> +a million loose objects before a final ref update and `gc`. In other\n> +more common cases such as on a server being pushed to with default\n> +`transfer.unpackLimit` settings the difference might not be noticable.\n> ++\n> +However, that's highly filesystem-dependent, on some filesystems\n> +simply calling fsync() might force an unrelated bulk background write\n> +to be serialized to disk. Such edge cases are the reason this option\n> +is off by default. That default setting might change in future\n> +versions.\n> ++\n> +In older versions of git only the descriptor for the file itself was\n> +fsynced, not its directory entry.\n> \n> core.preloadIndex::\n> \tEnable parallel index preload for operations like 'git diff'\n"},{"id":"405772","messageId":"20200917171212.GA3732163@coredump.intra.peff.net","threadId":"47626","inReplyTo":"xmqqzh5os9cg.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T17:12:12Z","receivedAt":"2020-09-17T17:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 08:37:19AM -0700, Junio C Hamano wrote:\n\n> > You need to fsync both to be fully compliant, even if just fsyncing b\n> > will work for most but not all file systems. The good news is that\n> > for those common file systems the extra fsync of a is almost free.\n> \n> Back to Ævar's patch, when creating a new loose object, we do these\n> things:\n> \n> 1. create temporary file and write the compressed contents to it\n> while computing its object name\n> \n> 2. create the fan-out directory under .git/objects/ if needed\n> \n> 3. mv temporary file to its final name\n> \n> and the patch adds open+fsync+close on the fan-out directory. In\n> the above exchange with Peff, we learned that open+fsync+close needs\n> to be done on .git/objects if we created the fan-out directory, too.\n> \n> Am I reading the above correctly?\n\nThat's my understanding. It gets trickier with refs (which I think we\nalso ought to consider fsyncing), as we may create arbitrarily deep\nhierarchies (so we'd have to keep track of which parts got created, or\njust conservatively fsync up the whole hierarchy).\n\nIt gets a lot easier if we move to reftables that have a more\npredictable file/disk structure.\n\n-Peff\n"},{"id":"405793","messageId":"20200917150958.GA31693@lst.de","threadId":"47626","inReplyTo":"20200917131605.GC3024501@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2020-09-17T15:09:58Z","receivedAt":"2020-09-17T19:54:58Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"On Thu, Sep 17, 2020 at 09:16:05AM -0400, Jeff King wrote:\n> I've also often wondered whether this is necessary. Given the symptom of\n> \"oops, this object is there but with 0 bytes\" after a hard crash (power\n> off, etc), my assumption is that the metadata is being journaled but the\n> actual data is not. Which would imply this isn't needed, but may just be\n> revealing my naive view of how filesystems work.\n> \n> And of course all of my experience is on ext4 (which doubly confuses me,\n> because my systems typically have data=ordered, which I thought would\n> solve this). Non-journalling filesystems or other modes likely behave\n> differently, but if this extra fsync carries a cost, we may want to make\n> it optional.\n\nI hope my other mail clarified how this works at a high level, if not\nfeel free to ask more questions.\n\n> > sha1-file.c | 19 ++++++++++++++-----\n> > 1 file changed, 14 insertions(+), 5 deletions(-)\n> \n> We already fsync pack files, but we don't fsync their directories. If\n> this is important to do, we should be doing it there, too.\n> \n> We also don't fsync ref files (nor packed-refs) at all. If fsyncing\n> files is important for reliability, we should be including those, too.\n> It may be tempting to say that the important stuff is in objects and the\n> refs can be salvaged from the commit graph, but my experience says\n> otherwise. Missing, broken, or mysteriously-rewound refs cause confusing\n> user-visible behavior, and when compounded with pruning operations like\n> \"git gc\" they _do_ result in losing objects.\n\nTrue, this probably needs to do for the directories of other files\nas well.\n\nOne interesting optimization under linux is the syncfs syscall, that\nsyncs all files on a file system - if you need to do a large number\nof fsyncs that do not depend on each other for transaction semantics\nit can provide a huge speedup.\n"},{"id":"405795","messageId":"20200917141257.GB27653@lst.de","threadId":"47626","inReplyTo":"20200917112830.26606-3-avarab@gmail.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Christoph Hellwig","fromEmail":"hch@lst.de","sentAt":"2020-09-17T14:12:57Z","receivedAt":"2020-09-17T19:56:32Z","isPatch":true,"sender":{"key":"hch@lst.de","avatar":null},"body":"> core.fsyncObjectFiles::\n> +\tThis boolean will enable 'fsync()' when writing loose object\n> +\tfiles. Both the file itself and its containng directory will\n> +\tbe fsynced.\n> ++\n> +When git writes data any required object writes will precede the\n> +corresponding reference update(s). For example, a\n> +linkgit:git-receive-pack[1] accepting a push might write a pack or\n> +loose objects (depending on settings such as `transfer.unpackLimit`).\n> ++\n> +Therefore on a journaled file system which ensures that data is\n> +flushed to disk in chronological order an fsync shouldn't be\n> +needed. The loose objects might be lost with a crash, but so will the\n> +ref update that would have referenced them. Git's own state in such a\n> +crash will remain consistent.\n\nWhile this is much better than what we had before I'm not sure it is\nall that useful. The only file system I know of that actually had the\nabove behavior was ext3, and the fact that it always wrote back that\nway made it a complete performance desaster. So even mentioning this\nhere will probably create a lot more confusion than actually clearing\nthings up.\n\n> ++\n> +This option exists because that assumption doesn't hold on filesystems\n> +where the data ordering is not preserved, such as on ext3 and ext4\n> +with \"data=writeback\". On such a filesystem the `rename()` that drops\n> +the new reference in place might be preserved, but the contents or\n> +directory entry for the loose object(s) might not have been synced to\n> +disk.\n\nAs well as just about any other file system. Which is another argument\non why it needs to be on by default. Every time I install a new\ndevelopment system (aka one that often crashes) and forget to enable\nthe option I keep corrupting my git repos. And that is with at least\nbtrfs, ext4 and xfs as it is pretty much by design.\n\n> +However, that's highly filesystem-dependent, on some filesystems\n> +simply calling fsync() might force an unrelated bulk background write\n> +to be serialized to disk. Such edge cases are the reason this option\n> +is off by default. That default setting might change in future\n> +versions.\n\nAgain the only \"some file system\" that was widely used that did this\nwas ext3. And ext3 has long been removed from the Linux kernel..\n"},{"id":"405798","messageId":"64358b70-4fff-5dc8-6e63-2fc916bea6af@kdbg.org","threadId":"47626","inReplyTo":"20200917112830.26606-2-avarab@gmail.com","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-09-17T20:21:57Z","receivedAt":"2020-09-17T20:22:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.09.20 um 13:28 schrieb Ævar Arnfjörð Bjarmason:\n> Change the behavior of core.fsyncObjectFiles to also sync the\n> directory entry. I don't have a case where this broke, just going by\n> paranoia and the fsync(2) manual page's guarantees about its behavior.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> sha1-file.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n> \n> diff --git a/sha1-file.c b/sha1-file.c\n> index dd65bd5c68..d286346921 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n> }\n> \n> /* Finalize a file on disk, and close it. */\n> -static void close_loose_object(int fd)\n> +static void close_loose_object(int fd, const struct strbuf *dirname)\n> {\n> -\tif (fsync_object_files)\n> +\tint dirfd;\n> +\tif (fsync_object_files) {\n> \t\tfsync_or_die(fd, \"loose object file\");\n> +\t\tdirfd = xopen(dirname->buf, O_RDONLY);\n> +\t\tfsync_or_die(dirfd, \"loose object directory\");\n\nDid you have the opportunity to verify that this works on Windows?\nOpening a directory with open(2), I mean: It's disallowed according to\nthe docs:\nhttps://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019#return-value\n\n> +\t}\n> \tif (close(fd) != 0)\n> \t\tdie_errno(_(\"error when closing loose object file\"));\n> }\n\n-- Hannes\n"},{"id":"405801","messageId":"20200917203749.GA1589021@nand.local","threadId":"47626","inReplyTo":"20200917171212.GA3732163@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-17T20:37:49Z","receivedAt":"2020-09-17T20:37:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 17, 2020 at 01:12:12PM -0400, Jeff King wrote:\n> On Thu, Sep 17, 2020 at 08:37:19AM -0700, Junio C Hamano wrote:\n> > Am I reading the above correctly?\n>\n> That's my understanding. It gets trickier with refs (which I think we\n> also ought to consider fsyncing), as we may create arbitrarily deep\n> hierarchies (so we'd have to keep track of which parts got created, or\n> just conservatively fsync up the whole hierarchy).\n\nYeah, it definitely gets trickier, but hopefully not by much. I\nappreciate Christoph's explanation, and certainly buy into it. I can't\nthink of any reason why we wouldn't want to apply the same reasoning to\nstoring refs, too.\n\nIt shouldn't be a hold-up for this series, though.\n\n> It gets a lot easier if we move to reftables that have a more\n> predictable file/disk structure.\n\nIn the sense that we don't have to worry about arbitrary-depth loose\nreferences, yes, but I think we'll still have to deal with both cases.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"405802","messageId":"5b969f59-0006-8632-d040-6a816416f51a@kdbg.org","threadId":"47626","inReplyTo":"xmqqv9gcs91k.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-09-17T20:15:10Z","receivedAt":"2020-09-17T20:59:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.09.20 um 17:43 schrieb Junio C Hamano:\n> Ævar Arnfjörð Bjarmason writes:\n> \n>> As amusing as Linus's original prose[1] is here it doesn't really explain\n>> in any detail to the uninitiated why you would or wouldn't enable\n>> this, and the counter-intuitive reason for why git wouldn't fsync your\n>> precious data.\n>>\n>> So elaborate (a lot) on why this may or may not be needed. This is my\n>> best-effort attempt to summarize the various points raised in the last\n>> ML[2] discussion about this.\n>>\n>> 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n>> files\", 2008-06-18)\n>> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n>> 1 file changed, 36 insertions(+), 6 deletions(-)\n> \n> When I saw the subject in my mailbox, I expected to see that you\n> would resurrect Christoph's updated text in [*1*], but you wrote a\n> whole lot more ;-) And they are quite informative to help readers to\n> understand what the option does. I am not sure if the understanding\n> directly help readers to decide if it is appropriate for their own\n> repositories, though X-<.\n\nNot only that; the new text also uses the term \"fsync\" in a manner that\nI could be persuaded that it is actually an English word. Which, so far,\nI doubt that it is ;) A little bit less 1337 wording would help the\nusers better.\n\n> \n> \n> Thanks.\n> \n> [Reference]\n> \n> *1* https://public-inbox.org/git/20180117193510.GA30657@lst.de/\n> \n>>\n>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n>> index 74619a9c03..5b47670c16 100644\n>> --- a/Documentation/config/core.txt\n>> +++ b/Documentation/config/core.txt\n>> @@ -548,12 +548,42 @@ core.whitespace::\n>> errors. The default tab width is 8. Allowed values are 1 to 63.\n>> \n>> core.fsyncObjectFiles::\n>> -\tThis boolean will enable 'fsync()' when writing object files.\n>> -+\n>> -This is a total waste of time and effort on a filesystem that orders\n>> -data writes properly, but can be useful for filesystems that do not use\n>> -journalling (traditional UNIX filesystems) or that only journal metadata\n>> -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n>> +\tThis boolean will enable 'fsync()' when writing loose object\n>> +\tfiles. Both the file itself and its containng directory will\n>> +\tbe fsynced.\n>> ++\n>> +When git writes data any required object writes will precede the\n>> +corresponding reference update(s). For example, a\n>> +linkgit:git-receive-pack[1] accepting a push might write a pack or\n>> +loose objects (depending on settings such as `transfer.unpackLimit`).\n>> ++\n>> +Therefore on a journaled file system which ensures that data is\n>> +flushed to disk in chronological order an fsync shouldn't be\n>> +needed. The loose objects might be lost with a crash, but so will the\n>> +ref update that would have referenced them. Git's own state in such a\n>> +crash will remain consistent.\n>> ++\n>> +This option exists because that assumption doesn't hold on filesystems\n>> +where the data ordering is not preserved, such as on ext3 and ext4\n>> +with \"data=writeback\". On such a filesystem the `rename()` that drops\n>> +the new reference in place might be preserved, but the contents or\n>> +directory entry for the loose object(s) might not have been synced to\n>> +disk.\n>> ++\n>> +Enabling this option might slow git down by a lot in some\n>> +cases. E.g. in the case of a naïve bulk import tool which might create\n>> +a million loose objects before a final ref update and `gc`. In other\n>> +more common cases such as on a server being pushed to with default\n>> +`transfer.unpackLimit` settings the difference might not be noticable.\n>> ++\n>> +However, that's highly filesystem-dependent, on some filesystems\n>> +simply calling fsync() might force an unrelated bulk background write\n>> +to be serialized to disk. Such edge cases are the reason this option\n>> +is off by default. That default setting might change in future\n>> +versions.\n>> ++\n>> +In older versions of git only the descriptor for the file itself was\n>> +fsynced, not its directory entry.\n>> \n>> core.preloadIndex::\n>> \tEnable parallel index preload for operations like 'git diff'\n> \n\n"},{"id":"405803","messageId":"1edd9eb1-365b-b4f7-87f8-3ad35bd7d5be@xiplink.com","threadId":"47626","inReplyTo":"20200917112830.26606-3-avarab@gmail.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2020-09-17T19:21:29Z","receivedAt":"2020-09-17T21:16:51Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2020-09-17 7:28 a.m., Ævar Arnfjörð Bjarmason wrote:\n> As amusing as Linus's original prose[1] is here it doesn't really explain\n> in any detail to the uninitiated why you would or wouldn't enable\n> this, and the counter-intuitive reason for why git wouldn't fsync your\n> precious data.\n> \n> So elaborate (a lot) on why this may or may not be needed. This is my\n> best-effort attempt to summarize the various points raised in the last\n> ML[2] discussion about this.\n> \n> 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n> files\", 2008-06-18)\n> 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n> 1 file changed, 36 insertions(+), 6 deletions(-)\n> \n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index 74619a9c03..5b47670c16 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -548,12 +548,42 @@ core.whitespace::\n> errors. The default tab width is 8. Allowed values are 1 to 63.\n> \n> core.fsyncObjectFiles::\n> -\tThis boolean will enable 'fsync()' when writing object files.\n> -+\n> -This is a total waste of time and effort on a filesystem that orders\n> -data writes properly, but can be useful for filesystems that do not use\n> -journalling (traditional UNIX filesystems) or that only journal metadata\n> -and not file contents (OS X's HFS+, or Linux ext3 with \"data=writeback\").\n> +\tThis boolean will enable 'fsync()' when writing loose object\n> +\tfiles. Both the file itself and its containng directory will\n\nTypo: containng\n\n\t\tM.\n\n> +\tbe fsynced.\n> ++\n> +When git writes data any required object writes will precede the\n> +corresponding reference update(s). For example, a\n> +linkgit:git-receive-pack[1] accepting a push might write a pack or\n> +loose objects (depending on settings such as `transfer.unpackLimit`).\n> ++\n> +Therefore on a journaled file system which ensures that data is\n> +flushed to disk in chronological order an fsync shouldn't be\n> +needed. The loose objects might be lost with a crash, but so will the\n> +ref update that would have referenced them. Git's own state in such a\n> +crash will remain consistent.\n> ++\n> +This option exists because that assumption doesn't hold on filesystems\n> +where the data ordering is not preserved, such as on ext3 and ext4\n> +with \"data=writeback\". On such a filesystem the `rename()` that drops\n> +the new reference in place might be preserved, but the contents or\n> +directory entry for the loose object(s) might not have been synced to\n> +disk.\n> ++\n> +Enabling this option might slow git down by a lot in some\n> +cases. E.g. in the case of a naïve bulk import tool which might create\n> +a million loose objects before a final ref update and `gc`. In other\n> +more common cases such as on a server being pushed to with default\n> +`transfer.unpackLimit` settings the difference might not be noticable.\n> ++\n> +However, that's highly filesystem-dependent, on some filesystems\n> +simply calling fsync() might force an unrelated bulk background write\n> +to be serialized to disk. Such edge cases are the reason this option\n> +is off by default. That default setting might change in future\n> +versions.\n> ++\n> +In older versions of git only the descriptor for the file itself was\n> +fsynced, not its directory entry.\n> \n> core.preloadIndex::\n> \tEnable parallel index preload for operations like 'git diff'\n> \n"},{"id":"406116","messageId":"87a6xii5gs.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"64358b70-4fff-5dc8-6e63-2fc916bea6af@kdbg.org","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-22T08:24:51Z","receivedAt":"2020-09-22T08:33:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 17 2020, Johannes Sixt wrote:\n\n> Am 17.09.20 um 13:28 schrieb Ævar Arnfjörð Bjarmason:\n>> Change the behavior of core.fsyncObjectFiles to also sync the\n>> directory entry. I don't have a case where this broke, just going by\n>> paranoia and the fsync(2) manual page's guarantees about its behavior.\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> sha1-file.c | 19 ++++++++++++++-----\n>> 1 file changed, 14 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/sha1-file.c b/sha1-file.c\n>> index dd65bd5c68..d286346921 100644\n>> --- a/sha1-file.c\n>> +++ b/sha1-file.c\n>> @@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n>> }\n>> \n>> /* Finalize a file on disk, and close it. */\n>> -static void close_loose_object(int fd)\n>> +static void close_loose_object(int fd, const struct strbuf *dirname)\n>> {\n>> -\tif (fsync_object_files)\n>> +\tint dirfd;\n>> +\tif (fsync_object_files) {\n>> \t\tfsync_or_die(fd, \"loose object file\");\n>> +\t\tdirfd = xopen(dirname->buf, O_RDONLY);\n>> +\t\tfsync_or_die(dirfd, \"loose object directory\");\n>\n> Did you have the opportunity to verify that this works on Windows?\n> Opening a directory with open(2), I mean: It's disallowed according to\n> the docs:\n> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019#return-value\n\nI did not, just did a quick hack for an RFC discussion (didn't even\nclose() that fd), but if I pursue this I'll do it properly.\n\nDoing some research on it now reveals that we should probably have some\nWindows-specific code here, e.g. browsing GNUlib's source code reveals\nthat it uses FlushFileBuffers(), and that code itself is taken from\nsqlite. SQLite also has special-case code for some Unix warts,\ne.g. OSX's and AIX's special fsync behaviors in its src/os_unix.c\n\n>> +\t}\n>> \tif (close(fd) != 0)\n>> \t\tdie_errno(_(\"error when closing loose object file\"));\n>> }\n>\n> -- Hannes\n\n"},{"id":"406120","messageId":"877dsmhz36.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"20200917140912.GA27653@lst.de","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-22T10:42:37Z","receivedAt":"2020-09-22T10:42:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 17 2020, Christoph Hellwig wrote:\n\n> On Thu, Sep 17, 2020 at 01:28:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Change the behavior of core.fsyncObjectFiles to also sync the\n>> directory entry. I don't have a case where this broke, just going by\n>> paranoia and the fsync(2) manual page's guarantees about its behavior.\n>\n> It is not just paranoia, but indeed what is required from the standards\n> POV. At least for many Linux file systems your second fsync will be\n> very cheap (basically a NULL syscall) as the log has alredy been forced\n> all the way by the first one, but you can't rely on that.\n>\n> Acked-by: Christoph Hellwig \n\nThanks a lot for your advice in this thread.\n\nCan you (or someone else) suggest a Linux fs setup that's as unforgiving\nas possible vis-a-vis fsync() for testing? I'd like to hack on making\ngit better at this, but one of the problems of testing it is that modern\nfilesystems generally do a pretty good job of not losing your data.\n\nSo something like ext4's commit=N is an obvious start, but for git's own\ntest suite it would be ideal to have process A write file X, and then\nhave process B try to read it and just not see it if X hadn't been\nfsynced (or not see its directory if that hadn't been synced).\n\nIt would turn our test suite into pretty much a 100% failure, but one\nthat could then be fixed by fixing the relevant file writing code.\n"},{"id":"407127","messageId":"nycvar.QRO.7.76.6.2010081012490.50@tvgsbejvaqbjf.bet","threadId":"47626","inReplyTo":"xmqqv9gcs91k.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-08T08:13:29Z","receivedAt":"2020-10-08T08:13:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio and Ævar,\n\nOn Thu, 17 Sep 2020, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n> > As amusing as Linus's original prose[1] is here it doesn't really explain\n> > in any detail to the uninitiated why you would or wouldn't enable\n> > this, and the counter-intuitive reason for why git wouldn't fsync your\n> > precious data.\n> >\n> > So elaborate (a lot) on why this may or may not be needed. This is my\n> > best-effort attempt to summarize the various points raised in the last\n> > ML[2] discussion about this.\n> >\n> > 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n> > files\", 2008-06-18)\n> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n> >\n> > Signed-off-by: Ævar Arnfjörð Bjarmason \n> > ---\n> > Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n> > 1 file changed, 36 insertions(+), 6 deletions(-)\n>\n> When I saw the subject in my mailbox, I expected to see that you\n> would resurrect Christoph's updated text in [*1*], but you wrote a\n> whole lot more ;-) And they are quite informative to help readers to\n> understand what the option does. I am not sure if the understanding\n> directly help readers to decide if it is appropriate for their own\n> repositories, though X-<.\n\nI agree that it is an improvement, and am therefore in favor of applying\nthe patch.\n\nCiao,\nDscho\n"},{"id":"407160","messageId":"87eem8hfrp.fsf@evledraar.gmail.com","threadId":"47626","inReplyTo":"nycvar.QRO.7.76.6.2010081012490.50@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-10-08T15:57:30Z","receivedAt":"2020-10-08T15:58:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 08 2020, Johannes Schindelin wrote:\n\n> Hi Junio and Ævar,\n>\n> On Thu, 17 Sep 2020, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>> > As amusing as Linus's original prose[1] is here it doesn't really explain\n>> > in any detail to the uninitiated why you would or wouldn't enable\n>> > this, and the counter-intuitive reason for why git wouldn't fsync your\n>> > precious data.\n>> >\n>> > So elaborate (a lot) on why this may or may not be needed. This is my\n>> > best-effort attempt to summarize the various points raised in the last\n>> > ML[2] discussion about this.\n>> >\n>> > 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n>> > files\", 2008-06-18)\n>> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n>> >\n>> > Signed-off-by: Ævar Arnfjörð Bjarmason \n>> > ---\n>> > Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n>> > 1 file changed, 36 insertions(+), 6 deletions(-)\n>>\n>> When I saw the subject in my mailbox, I expected to see that you\n>> would resurrect Christoph's updated text in [*1*], but you wrote a\n>> whole lot more ;-) And they are quite informative to help readers to\n>> understand what the option does. I am not sure if the understanding\n>> directly help readers to decide if it is appropriate for their own\n>> repositories, though X-<.\n>\n> I agree that it is an improvement, and am therefore in favor of applying\n> the patch.\n\nJust the improved docs, or flipping the default of core.fsyncObjectFiles\nto \"true\"?\n\nI've been meaning to re-roll this. I won't have time anytime soon to fix\ngit's fsync() use, i.e. ensure that we run up & down modified\ndirectories and fsync()/fdatasync() file/dir fd's as appropriate but I\nthink documenting it and changing the core.fsyncObjectFiles default\nmakes sense and is at least a step in the right direction.\n\nI do think it makes more sense for a v2 to split most of this out into\nsome section that generally discusses data integrity in the .git\ndirectory. I.e. that says that currently where we use fsync() (such as\npack/commit-graph writes) we don't fsync() the corresponding\ndirector{y,ies), and ref updates don't fsync() at all.\n\nWhere to put that though? gitrepository-layout(5)? Or a new page like\ngitrepository-integrity(5) (other suggestions welcome..).\n\nLooking at the code again it seems easier than I thought to make this\nright if we ignore .git/refs (which reftable can fix for us). Just:\n\n1. Change fsync_or_die() and its callsites to also pass/sync the\n containing directory, which is always created already\n (e.g. .git/objects/pack/)...).\n\n2. ..Or in the case where it's not created already such as\n .git/objects/??/ (or .git/objects/pack/) itself) it's not N-deep like\n the refs hierarchy, so \"did we create it\" state is pretty simple, or\n we can just always do it unconditionally.\n\n3. Without reftable the .git/refs/ case shouldn't be too hard if we're\n OK with redundantly fsyncing all the way down, i.e. to make it\n simpler by not tracking the state of exactly what was changed.\n\n4. Now that I'm writing this there's also .git/{config,rr-cache} and any\n number of other things we need to change for 100% coverage, but the\n above 1-3 should be good enough for repo integrity where repo = refs\n & objects.\n"},{"id":"407177","messageId":"xmqqo8lcwnuz.fsf@gitster.c.googlers.com","threadId":"47626","inReplyTo":"87eem8hfrp.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-08T18:53:40Z","receivedAt":"2020-10-08T18:53:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> When I saw the subject in my mailbox, I expected to see that you\n>>> would resurrect Christoph's updated text in [*1*], but you wrote a\n>>> whole lot more ;-) And they are quite informative to help readers to\n>>> understand what the option does. I am not sure if the understanding\n>>> directly help readers to decide if it is appropriate for their own\n>>> repositories, though X-<.\n>>\n>> I agree that it is an improvement, and am therefore in favor of applying\n>> the patch.\n>\n> Just the improved docs, or flipping the default of core.fsyncObjectFiles\n> to \"true\"?\n\nI am not Dscho, but \"applying THE patch\" meant, at least to me, the\npatch [2/2] to the docs, which was the message we are responding to.\n\n> I've been meaning to re-roll this. I won't have time anytime soon to fix\n> git's fsync() use, i.e. ensure that we run up & down modified\n> directories and fsync()/fdatasync() file/dir fd's as appropriate but I\n> think documenting it and changing the core.fsyncObjectFiles default\n> makes sense and is at least a step in the right direction.\n>\n> I do think it makes more sense for a v2 to split most of this out into\n> some section that generally discusses data integrity in the .git\n> directory. I.e. that says that currently where we use fsync() (such as\n> pack/commit-graph writes) we don't fsync() the corresponding\n> director{y,ies), and ref updates don't fsync() at all.\n\nYes, I think all of these are sensible things to do sometime in the\nfuture.\n\n\n> Where to put that though? gitrepository-layout(5)? Or a new page like\n> gitrepository-integrity(5) (other suggestions welcome..).\n\nI do not have a good suggestion at this moment on this.\n\n\n\n\n"},{"id":"407207","messageId":"nycvar.QRO.7.76.6.2010091219480.50@tvgsbejvaqbjf.bet","threadId":"47626","inReplyTo":"87eem8hfrp.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] core.fsyncObjectFiles: make the docs less flippant","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-09T10:44:36Z","receivedAt":"2020-10-09T13:02:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 8 Oct 2020, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, Oct 08 2020, Johannes Schindelin wrote:\n>\n> > On Thu, 17 Sep 2020, Junio C Hamano wrote:\n> >\n> >> Ævar Arnfjörð Bjarmason writes:\n> >>\n> >> > As amusing as Linus's original prose[1] is here it doesn't really explain\n> >> > in any detail to the uninitiated why you would or wouldn't enable\n> >> > this, and the counter-intuitive reason for why git wouldn't fsync your\n> >> > precious data.\n> >> >\n> >> > So elaborate (a lot) on why this may or may not be needed. This is my\n> >> > best-effort attempt to summarize the various points raised in the last\n> >> > ML[2] discussion about this.\n> >> >\n> >> > 1. aafe9fbaf4 (\"Add config option to enable 'fsync()' of object\n> >> > files\", 2008-06-18)\n> >> > 2. https://lore.kernel.org/git/20180117184828.31816-1-hch@lst.de/\n> >> >\n> >> > Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> > ---\n> >> > Documentation/config/core.txt | 42 ++++++++++++++++++++++++++++++-----\n> >> > 1 file changed, 36 insertions(+), 6 deletions(-)\n> >>\n> >> When I saw the subject in my mailbox, I expected to see that you\n> >> would resurrect Christoph's updated text in [*1*], but you wrote a\n> >> whole lot more ;-) And they are quite informative to help readers to\n> >> understand what the option does. I am not sure if the understanding\n> >> directly help readers to decide if it is appropriate for their own\n> >> repositories, though X-<.\n> >\n> > I agree that it is an improvement, and am therefore in favor of applying\n> > the patch.\n>\n> Just the improved docs, or flipping the default of core.fsyncObjectFiles\n> to \"true\"?\n\nI am actually also in favor of flipping the default. We carry\nhttps://github.com/git-for-windows/git/commit/14dad078c28159b250be599c0890ece2d6f4d635\nin Git for Windows for over three years. The commit message:\n\n\tmingw: change core.fsyncObjectFiles = 1 by default\n\n\tFrom the documentation of said setting:\n\n\t\tThis boolean will enable fsync() when writing object files.\n\n\t\tThis is a total waste of time and effort on a filesystem that\n\t\torders data writes properly, but can be useful for filesystems\n\t\tthat do not use journalling (traditional UNIX filesystems) or\n\t\tthat only journal metadata and not file contents (OS X’s HFS+,\n\t\tor Linux ext3 with \"data=writeback\").\n\n\tThe most common file system on Windows (NTFS) does not guarantee that\n\torder, therefore a sudden loss of power (or any other event causing an\n\tunclean shutdown) would cause corrupt files (i.e. files filled with\n\tNULs). Therefore we need to change the default.\n\n\tNote that the documentation makes it sound as if this causes really bad\n\tperformance. In reality, writing loose objects is something that is done\n\tonly rarely, and only a handful of files at a time.\n\n\tSigned-off-by: Johannes Schindelin \n\nThe patch itself limits this change to Windows, but if this becomes a\nplatform-independent change, all the better for me!\n\n> I've been meaning to re-roll this. I won't have time anytime soon to fix\n> git's fsync() use, i.e. ensure that we run up & down modified\n> directories and fsync()/fdatasync() file/dir fd's as appropriate but I\n> think documenting it and changing the core.fsyncObjectFiles default\n> makes sense and is at least a step in the right direction.\n\nAgreed.\n\n> I do think it makes more sense for a v2 to split most of this out into\n> some section that generally discusses data integrity in the .git\n> directory. I.e. that says that currently where we use fsync() (such as\n> pack/commit-graph writes) we don't fsync() the corresponding\n> director{y,ies), and ref updates don't fsync() at all.\n>\n> Where to put that though? gitrepository-layout(5)? Or a new page like\n> gitrepository-integrity(5) (other suggestions welcome..).\n\nI think `gitrepository-layout` is probably the best location for now.\n\n> Looking at the code again it seems easier than I thought to make this\n> right if we ignore .git/refs (which reftable can fix for us). Just:\n>\n> 1. Change fsync_or_die() and its callsites to also pass/sync the\n> containing directory, which is always created already\n> (e.g. .git/objects/pack/)...).\n>\n> 2. ..Or in the case where it's not created already such as\n> .git/objects/??/ (or .git/objects/pack/) itself) it's not N-deep like\n> the refs hierarchy, so \"did we create it\" state is pretty simple, or\n> we can just always do it unconditionally.\n>\n> 3. Without reftable the .git/refs/ case shouldn't be too hard if we're\n> OK with redundantly fsyncing all the way down, i.e. to make it\n> simpler by not tracking the state of exactly what was changed.\n>\n> 4. Now that I'm writing this there's also .git/{config,rr-cache} and any\n> number of other things we need to change for 100% coverage, but the\n> above 1-3 should be good enough for repo integrity where repo = refs\n> & objects.\n\nI fear that the changes to `fsync` also the directory need to be guarded\nbehind a preprocessor flag, though: if you try to `open()` a directory on\nWindows, it fails (our emulated `open()` sets `errno = EISDIR`).\n\nCiao,\nDscho\n"},{"id":"410347","messageId":"nycvar.QRO.7.76.6.2011191232490.56@tvgsbejvaqbjf.bet","threadId":"47626","inReplyTo":"87a6xii5gs.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-19T11:38:28Z","receivedAt":"2020-11-19T11:38:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 22 Sep 2020, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, Sep 17 2020, Johannes Sixt wrote:\n>\n> > Am 17.09.20 um 13:28 schrieb Ævar Arnfjörð Bjarmason:\n> >> Change the behavior of core.fsyncObjectFiles to also sync the\n> >> directory entry. I don't have a case where this broke, just going by\n> >> paranoia and the fsync(2) manual page's guarantees about its behavior.\n> >>\n> >> Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> ---\n> >> sha1-file.c | 19 ++++++++++++++-----\n> >> 1 file changed, 14 insertions(+), 5 deletions(-)\n> >>\n> >> diff --git a/sha1-file.c b/sha1-file.c\n> >> index dd65bd5c68..d286346921 100644\n> >> --- a/sha1-file.c\n> >> +++ b/sha1-file.c\n> >> @@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n> >> }\n> >>\n> >> /* Finalize a file on disk, and close it. */\n> >> -static void close_loose_object(int fd)\n> >> +static void close_loose_object(int fd, const struct strbuf *dirname)\n> >> {\n> >> -\tif (fsync_object_files)\n> >> +\tint dirfd;\n> >> +\tif (fsync_object_files) {\n> >> \t\tfsync_or_die(fd, \"loose object file\");\n> >> +\t\tdirfd = xopen(dirname->buf, O_RDONLY);\n> >> +\t\tfsync_or_die(dirfd, \"loose object directory\");\n> >\n> > Did you have the opportunity to verify that this works on Windows?\n> > Opening a directory with open(2), I mean: It's disallowed according to\n> > the docs:\n> > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019#return-value\n>\n> I did not, just did a quick hack for an RFC discussion (didn't even\n> close() that fd), but if I pursue this I'll do it properly.\n>\n> Doing some research on it now reveals that we should probably have some\n> Windows-specific code here, e.g. browsing GNUlib's source code reveals\n> that it uses FlushFileBuffers(), and that code itself is taken from\n> sqlite. SQLite also has special-case code for some Unix warts,\n> e.g. OSX's and AIX's special fsync behaviors in its src/os_unix.c\n\nIf I understand correctly, the idea to `fsync()` directories is to ensure\nthat metadata updates (such as renames) are flushed, too?\n\nIf so (and please note that my understanding of NTFS is not super deep in\nthis regard), I think that we need not worry on Windows. I have come to\nbelieve that the `rename()` operations are flushed pretty much\nimmediately, without any `FlushFileBuffers()` (or `_commit()`, as we\nactually do in `compat/mingw.h`, to convince yourself see\nhttps://github.com/git/git/blob/v2.29.2/compat/mingw.h#L135-L136).\n\nDirectories are not mentioned in `FlushFileBuffers()`'s documentation\n(https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-flushfilebuffers)\nnor in the documentation of `_commit()`:\nhttps://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/commit?view=msvc-160\n\nTherefore, I believe that there is not even a Win32 equivalent of\n`fsync()`ing directories.\n\nCiao,\nDscho\n\n>\n> >> +\t} if (close(fd) != 0) die_errno(_(\"error when closing loose object\n> >> file\")); }\n> >\n> > -- Hannes\n>\n>\n>\n"}]}