{"thread":{"id":"56799","subject":"[PATCH] allow disabling fsync everywhere","startedAt":"2021-10-28T00:21:04Z","lastAt":"2021-10-30T10:49:31Z","messageCount":17,"participants":["Eric Wong","Eric Sunshine","Jeff King","Junio C Hamano","brian m. carlson","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"439851","messageId":"20211028002102.19384-1-e@80x24.org","threadId":"56799","inReplyTo":null,"subject":"[PATCH] allow disabling fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-28T00:21:02Z","receivedAt":"2021-10-28T00:21:04Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"\"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\nfor disabling fsync() even on packfiles and other critical data.\n\nRunning \"make test -j8 NO_SVN_TESTS=1\" on a noisy 8-core system\non an HDD, adding \"GIT_FSYNC=0\" to the invocation brings my test\nruntime from ~4 minutes down to ~3 minutes.\n\nSVN interopability tests are minimally affected since SVN will\nstill use fsync in various places.\n\nThis will also be useful for 3rd-party tools which create\nthrowaway git repositories of temporary data.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n Documentation/config/core.txt |  7 +++++++\n Documentation/git.txt         |  5 +++++\n cache.h                       |  1 +\n config.c                      |  5 +++++\n environment.c                 |  1 +\n git-cvsserver.perl            | 22 ++++++++++++++++++++++\n perl/Git/SVN.pm               | 20 ++++++++++++++++++--\n t/test-lib.sh                 |  1 +\n write-or-die.c                |  5 ++++-\n 9 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex c04f62a54a..2ad5364246 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -555,6 +555,13 @@ 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 \n+core.fsync::\n+\tA boolean to control 'fsync()' on all files.\n++\n+Setting this to false can speed up writes of unimportant data.\n+Disabling fsync will lead to data loss on power failure.  If set\n+to false, this also overrides core.fsyncObjectFiles.  Defaults to true.\n+\n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\n +\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex d63c65e67d..cda4504e41 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -703,6 +703,11 @@ for further details.\n \tnot set, Git will choose buffered or record-oriented flushing\n \tbased on whether stdout appears to be redirected to a file or not.\n \n+`GIT_FSYNC`::\n+\tSetting this environment variable to \"0\" disables fsync() entirely.\n+\tThis is intended for running test suites and other repositories of\n+\tunimportant data.  See also core.fsync in linkgit:git-config[1].\n+\n `GIT_TRACE`::\n \tEnables general trace messages, e.g. alias expansion, built-in\n \tcommand execution and external command execution.\ndiff --git a/cache.h b/cache.h\nindex eba12487b9..de6c45cf44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -986,6 +986,7 @@ extern int read_replace_refs;\n extern char *git_replace_ref_base;\n \n extern int fsync_object_files;\n+extern int use_fsync;\n extern int core_preload_index;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/config.c b/config.c\nindex 2dcbe901b6..1ea7cb801b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1492,6 +1492,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.fsync\")) {\n+\t\tuse_fsync = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.preloadindex\")) {\n \t\tcore_preload_index = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 9da7f3c1a1..0d06a31024 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,6 +42,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n int fsync_object_files;\n+int use_fsync = -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;\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 64319bed43..7679819e4d 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -3607,6 +3607,25 @@ package GITCVS::updater;\n use strict;\n use warnings;\n use DBI;\n+our $_use_fsync;\n+\n+# n.b. consider using Git.pm\n+sub use_fsync {\n+    if (!defined($_use_fsync)) {\n+        my $x = $ENV{GIT_FSYNC};\n+        if (defined $x) {\n+            local $ENV{GIT_CONFIG};\n+            delete $ENV{GIT_CONFIG};\n+            my $v = ::safe_pipe_capture('git', '-c', \"core.fsync=$x\",\n+                                        qw(config --type=bool core.fsync));\n+            $_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n+        } else {\n+            my $v = `git config --type=bool core.fsync`;\n+            $_use_fsync = $v eq \"false\\n\" ? 0 : 1;\n+        }\n+    }\n+    $_use_fsync;\n+}\n \n =head1 METHODS\n \n@@ -3676,6 +3695,9 @@ sub new\n                                 $self->{dbuser},\n                                 $self->{dbpass});\n     die \"Error connecting to database\\n\" unless defined $self->{dbh};\n+    if ($self->{dbdriver} eq 'SQLite' && !use_fsync()) {\n+        $self->{dbh}->do('PRAGMA synchronous = OFF');\n+    }\n \n     $self->{tables} = {};\n     foreach my $table ( keys %{$self->{dbh}->table_info(undef,undef,undef,'TABLE')->fetchall_hashref('TABLE_NAME')} )\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 35ff5a6896..7b333ea62e 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -6,7 +6,7 @@ package Git::SVN;\n use vars qw/$_no_metadata\n             $_repack $_repack_flags $_use_svm_props $_head\n             $_use_svnsync_props $no_reuse_existing\n-\t    $_use_log_author $_add_author_from $_localtime/;\n+\t    $_use_log_author $_add_author_from $_localtime $_use_fsync/;\n use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use IPC::Open3;\n@@ -2269,6 +2269,22 @@ sub mkfile {\n \t}\n }\n \n+# TODO: move this to Git.pm?\n+sub use_fsync {\n+\tif (!defined($_use_fsync)) {\n+\t\tmy $x = $ENV{GIT_FSYNC};\n+\t\tif (defined $x) {\n+\t\t\tmy $v = command_oneline('git', '-c', \"core.fsync=$x\",\n+\t\t\t\tqw(config --type=bool core.fsync));\n+\t\t\t$_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n+\t\t} else {\n+\t\t\t$_use_fsync = Git::config_bool('core.fsync');\n+\t\t\t$_use_fsync = 1 if !defined($_use_fsync);\n+\t\t}\n+\t}\n+\t$_use_fsync;\n+}\n+\n sub rev_map_set {\n \tmy ($self, $rev, $commit, $update_ref, $uuid) = @_;\n \tdefined $commit or die \"missing arg3\\n\";\n@@ -2290,7 +2306,7 @@ sub rev_map_set {\n \tmy $sync;\n \t# both of these options make our .rev_db file very, very important\n \t# and we can't afford to lose it because rebuild() won't work\n-\tif ($self->use_svm_props || $self->no_metadata) {\n+\tif (($self->use_svm_props || $self->no_metadata) && use_fsync()) {\n \t\trequire File::Copy;\n \t\t$sync = 1;\n \t\tFile::Copy::copy($db, $db_lock) or die \"rev_map_set(@_): \",\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex a291a5d4a2..61bb24444c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -442,6 +442,7 @@ unset VISUAL EMAIL LANGUAGE $(\"$PERL_PATH\" -e '\n \t\tPERF_\n \t\tCURL_VERBOSE\n \t\tTRACE_CURL\n+\t\tFSYNC\n \t));\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 0b1ec8190b..d2962dc423 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"config.h\"\n #include \"run-command.h\"\n \n /*\n@@ -57,7 +58,9 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n \n void fsync_or_die(int fd, const char *msg)\n {\n-\twhile (fsync(fd) < 0) {\n+\tif (use_fsync < 0)\n+\t\tuse_fsync = git_env_bool(\"GIT_FSYNC\", 1);\n+\twhile (use_fsync && fsync(fd) < 0) {\n \t\tif (errno != EINTR)\n \t\t\tdie_errno(\"fsync error on '%s'\", msg);\n \t}\n"},{"id":"439854","messageId":"CAPig+cSTL-Rr+-aHqm3rsM=PLrz+BaDj7+nXGYKnKcEHgEiGOQ@mail.gmail.com","threadId":"56799","inReplyTo":"20211028002102.19384-1-e@80x24.org","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-28T01:21:25Z","receivedAt":"2021-10-28T01:21:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 27, 2021 at 8:21 PM Eric Wong <e@80x24.org> wrote:\n> \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n> for disabling fsync() even on packfiles and other critical data.\n> [...]\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n> diff --git a/write-or-die.c b/write-or-die.c\n> @@ -57,7 +58,9 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n>  void fsync_or_die(int fd, const char *msg)\n>  {\n> -       while (fsync(fd) < 0) {\n> +       if (use_fsync < 0)\n> +               use_fsync = git_env_bool(\"GIT_FSYNC\", 1);\n> +       while (use_fsync && fsync(fd) < 0) {\n>                 if (errno != EINTR)\n>                         die_errno(\"fsync error on '%s'\", msg);\n>         }\n\nThis is minor, but placing `use_fsync` in the `while` condition makes\nthe logic harder to digest. The intent would be clearer at-a-glance if\nstructured like this:\n\n    if (use_fsync < 0)\n        use_fsync = git_env_bool(\"GIT_FSYNC\", 1);\n    if (!use_fsync)\n        return;\n    while (fsync(fd) < 0) {\n        if (errno != EINTR)\n            die_errno(\"fsync error on '%s'\", msg);\n    }\n"},{"id":"439874","messageId":"YXq1g4Zwfq8gJEoC@coredump.intra.peff.net","threadId":"56799","inReplyTo":"20211028002102.19384-1-e@80x24.org","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-28T14:36:51Z","receivedAt":"2021-10-28T14:38:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 28, 2021 at 12:21:02AM +0000, Eric Wong wrote:\n\n> \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n> for disabling fsync() even on packfiles and other critical data.\n> \n> Running \"make test -j8 NO_SVN_TESTS=1\" on a noisy 8-core system\n> on an HDD, adding \"GIT_FSYNC=0\" to the invocation brings my test\n> runtime from ~4 minutes down to ~3 minutes.\n> \n> SVN interopability tests are minimally affected since SVN will\n> still use fsync in various places.\n> \n> This will also be useful for 3rd-party tools which create\n> throwaway git repositories of temporary data.\n\nDo you mostly just care about the tests, or is the third-party tool\nsupport important to you? I ask because most of us switched to running\nthe tests with --root=/some/tmpfs long ago to achieve the same speedup.\n\n-Peff\n"},{"id":"439908","messageId":"20211028182824.GA1307@dcvr","threadId":"56799","inReplyTo":"YXq1g4Zwfq8gJEoC@coredump.intra.peff.net","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-28T18:28:24Z","receivedAt":"2021-10-28T18:28:26Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Thu, Oct 28, 2021 at 12:21:02AM +0000, Eric Wong wrote:\n> \n> > \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n> > for disabling fsync() even on packfiles and other critical data.\n\nFwiw, I'm questioning the need for core.fsync.  GIT_FSYNC alone\nmay be sufficient.\n\n> > This will also be useful for 3rd-party tools which create\n> > throwaway git repositories of temporary data.\n> \n> Do you mostly just care about the tests, or is the third-party tool\n> support important to you? I ask because most of us switched to running\n> the tests with --root=/some/tmpfs long ago to achieve the same speedup.\n\nThird-party tools and OSes which don't have a tmpfs mounted by\ndefault (I don't think most *BSDs have tmpfs enabled by\ndefault).\n\nI try to use libeatmydata everywhere I can; but that's not\nalways installed.\n\nI'm also strongly considering making GIT_FSYNC=0 the default for\nour own test suite since it's less setup for newbies.\n"},{"id":"439928","messageId":"xmqqilxhq774.fsf@gitster.g","threadId":"56799","inReplyTo":"20211028182824.GA1307@dcvr","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T19:29:19Z","receivedAt":"2021-10-28T19:29:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Jeff King <peff@peff.net> wrote:\n>> On Thu, Oct 28, 2021 at 12:21:02AM +0000, Eric Wong wrote:\n>> \n>> > \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n>> > for disabling fsync() even on packfiles and other critical data.\n>\n> Fwiw, I'm questioning the need for core.fsync.  GIT_FSYNC alone\n> may be sufficient.\n>\n>> > This will also be useful for 3rd-party tools which create\n>> > throwaway git repositories of temporary data.\n>> \n>> Do you mostly just care about the tests, or is the third-party tool\n>> support important to you? I ask because most of us switched to running\n>> the tests with --root=/some/tmpfs long ago to achieve the same speedup.\n>\n> Third-party tools and OSes which don't have a tmpfs mounted by\n> default (I don't think most *BSDs have tmpfs enabled by\n> default).\n>\n> I try to use libeatmydata everywhere I can; but that's not\n> always installed.\n>\n> I'm also strongly considering making GIT_FSYNC=0 the default for\n> our own test suite since it's less setup for newbies.\n\nIf the primary focus is for testing, perhaps GIT_TEST_FSYNC would be\nbetter?  I do not think we want to even advertise an option for not\nsyncing at all for end users working with any real repositories.\nNot even when they promise that the end user accepts all the\nresponsibility and will keep both halves when things got broken.\n\nThanks.\n"},{"id":"439951","messageId":"YXsYvgPs7d/0E+sK@camp.crustytoothpaste.net","threadId":"56799","inReplyTo":"20211028182824.GA1307@dcvr","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-10-28T21:40:14Z","receivedAt":"2021-10-28T21:40:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-10-28 at 18:28:24, Eric Wong wrote:\n> Third-party tools and OSes which don't have a tmpfs mounted by\n> default (I don't think most *BSDs have tmpfs enabled by\n> default).\n> \n> I try to use libeatmydata everywhere I can; but that's not\n> always installed.\n\nI was going to suggest libeatmydata for this purpose.  I don't think we\nshould grow this option when there's a library that does all of this for\nus without any mistakes and also is universally applicable.  Presumably\nif you find Git too slow in this case, you'll also find dpkg and other\nprograms too slow, and want to use it there as well.\n\nIt's also potentially a footgun where people end up \"making Git faster\"\nand then get corrupted data.  I'm imagining blog posts and Stack\nOverflow suggestions that people do this, just like we see tons of\nsuggestions for people to set http.postBuffer without understanding what\nit does.  I think \"eat my data\" is pretty clear about the consequences\nso we don't have to be, and it comes with a giant warning that you're\nalmost certainly going to experience data loss.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"439975","messageId":"20211029001552.GA29647@dcvr","threadId":"56799","inReplyTo":"xmqqilxhq774.fsf@gitster.g","subject":"[PATCH] tests: disable fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-29T00:15:52Z","receivedAt":"2021-10-29T00:15:54Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> >> > This will also be useful for 3rd-party tools which create\n> >> > throwaway git repositories of temporary data.\n> >> \n> >> Do you mostly just care about the tests, or is the third-party tool\n> >> support important to you? I ask because most of us switched to running\n> >> the tests with --root=/some/tmpfs long ago to achieve the same speedup.\n> >\n> > Third-party tools and OSes which don't have a tmpfs mounted by\n> > default (I don't think most *BSDs have tmpfs enabled by\n> > default).\n\n> > I'm also strongly considering making GIT_FSYNC=0 the default for\n> > our own test suite since it's less setup for newbies.\n> \n> If the primary focus is for testing, perhaps GIT_TEST_FSYNC would be\n> better?  I do not think we want to even advertise an option for not\n> syncing at all for end users working with any real repositories.\n> Not even when they promise that the end user accepts all the\n> responsibility and will keep both halves when things got broken.\n\nI think having an undocumented GIT_TEST_FSYNC is a good\ncompromise and saves us the support burden.\n\nFwiw, I do currently have a 3rd-party tool which creates\nthrowaway repos, but the throwaways are currently small enough\nthat objects stay loose.  They might get bigger and pack in\nin the future.\n\nRelying on an LD_PRELOAD-based solution such as eatmydata\ndoesn't work for staticly-linked systems, and can conflict\nwith other LD_PRELOAD-based tools one might use(*)\n\nv2 changes:\n* s/GIT_FSYNC/GIT_TEST_FSYNC/\n* disable fsync by default for tests, reduces setup for newcomers\n* fix style nit noted by Eric Sunshine\n\n--------------8<------------\nSubject: [PATCH] tests: disable fsync everywhere\n\nThe \"GIT_TEST_FSYNC\" environment variable now exists for\ndisabling fsync() even on packfiles and other \"critical\" data.\n\nRunning \"make test -j8 NO_SVN_TESTS=1\" on a noisy 8-core system\non an HDD, test runtime drops from ~4 minutes down to ~3 minutes.\nUsing \"GIT_TEST_FSYNC=1\" re-enables fsync() for comparison\npurposes.\n\nSVN interopability tests are minimally affected since SVN will\nstill use fsync in various places.\n\nThis will also be useful for 3rd-party tools which create\nthrowaway git repositories of temporary data, but remains\nundocumented for end users.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n cache.h            |  1 +\n environment.c      |  1 +\n git-cvsserver.perl | 19 +++++++++++++++++++\n perl/Git/SVN.pm    | 17 +++++++++++++++--\n t/test-lib.sh      |  7 +++++++\n write-or-die.c     |  5 +++++\n 6 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex eba12487b9..de6c45cf44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -986,6 +986,7 @@ extern int read_replace_refs;\n extern char *git_replace_ref_base;\n \n extern int fsync_object_files;\n+extern int use_fsync;\n extern int core_preload_index;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/environment.c b/environment.c\nindex 9da7f3c1a1..0d06a31024 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,6 +42,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n int fsync_object_files;\n+int use_fsync = -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;\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 64319bed43..4c8118010a 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -3607,6 +3607,22 @@ package GITCVS::updater;\n use strict;\n use warnings;\n use DBI;\n+our $_use_fsync;\n+\n+# n.b. consider using Git.pm\n+sub use_fsync {\n+    if (!defined($_use_fsync)) {\n+        my $x = $ENV{GIT_TEST_FSYNC};\n+        if (defined $x) {\n+            local $ENV{GIT_CONFIG};\n+            delete $ENV{GIT_CONFIG};\n+            my $v = ::safe_pipe_capture('git', '-c', \"test.fsync=$x\",\n+                                        qw(config --type=bool test.fsync));\n+            $_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n+        }\n+    }\n+    $_use_fsync;\n+}\n \n =head1 METHODS\n \n@@ -3676,6 +3692,9 @@ sub new\n                                 $self->{dbuser},\n                                 $self->{dbpass});\n     die \"Error connecting to database\\n\" unless defined $self->{dbh};\n+    if ($self->{dbdriver} eq 'SQLite' && !use_fsync()) {\n+        $self->{dbh}->do('PRAGMA synchronous = OFF');\n+    }\n \n     $self->{tables} = {};\n     foreach my $table ( keys %{$self->{dbh}->table_info(undef,undef,undef,'TABLE')->fetchall_hashref('TABLE_NAME')} )\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 35ff5a6896..df5a87a151 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -6,7 +6,7 @@ package Git::SVN;\n use vars qw/$_no_metadata\n             $_repack $_repack_flags $_use_svm_props $_head\n             $_use_svnsync_props $no_reuse_existing\n-\t    $_use_log_author $_add_author_from $_localtime/;\n+\t    $_use_log_author $_add_author_from $_localtime $_use_fsync/;\n use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use IPC::Open3;\n@@ -2269,6 +2269,19 @@ sub mkfile {\n \t}\n }\n \n+# TODO: move this to Git.pm?\n+sub use_fsync {\n+\tif (!defined($_use_fsync)) {\n+\t\tmy $x = $ENV{GIT_TEST_FSYNC};\n+\t\tif (defined $x) {\n+\t\t\tmy $v = command_oneline('git', '-c', \"test.fsync=$x\",\n+\t\t\t\t\tqw(config --type=bool test.fsync));\n+\t\t\t$_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n+\t\t}\n+\t}\n+\t$_use_fsync;\n+}\n+\n sub rev_map_set {\n \tmy ($self, $rev, $commit, $update_ref, $uuid) = @_;\n \tdefined $commit or die \"missing arg3\\n\";\n@@ -2290,7 +2303,7 @@ sub rev_map_set {\n \tmy $sync;\n \t# both of these options make our .rev_db file very, very important\n \t# and we can't afford to lose it because rebuild() won't work\n-\tif ($self->use_svm_props || $self->no_metadata) {\n+\tif (($self->use_svm_props || $self->no_metadata) && use_fsync()) {\n \t\trequire File::Copy;\n \t\t$sync = 1;\n \t\tFile::Copy::copy($db, $db_lock) or die \"rev_map_set(@_): \",\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex a291a5d4a2..21f5fab999 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -489,6 +489,13 @@ then\n \texport GIT_PERL_FATAL_WARNINGS\n fi\n \n+case $GIT_TEST_FSYNC in\n+'')\n+\tGIT_TEST_FSYNC=0\n+\texport GIT_TEST_FSYNC\n+\t;;\n+esac\n+\n # Add libc MALLOC and MALLOC_PERTURB test\n # only if we are not executing the test with valgrind\n if test -n \"$valgrind\" ||\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 0b1ec8190b..a3d5784cec 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"config.h\"\n #include \"run-command.h\"\n \n /*\n@@ -57,6 +58,10 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n \n void fsync_or_die(int fd, const char *msg)\n {\n+\tif (use_fsync < 0)\n+\t\tuse_fsync = git_env_bool(\"GIT_TEST_FSYNC\", 1);\n+\tif (!use_fsync)\n+\t\treturn;\n \twhile (fsync(fd) < 0) {\n \t\tif (errno != EINTR)\n \t\t\tdie_errno(\"fsync error on '%s'\", msg);\n-- \n\n(*) I happen to be working on an LD_PRELOAD-based malloc tracer\n    for Perl which may help us cut memory use in git-svn;\n    but interposing malloc/free gets tricky...\n"},{"id":"439977","messageId":"xmqqr1c4mmsf.fsf@gitster.g","threadId":"56799","inReplyTo":"20211029001552.GA29647@dcvr","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T05:18:24Z","receivedAt":"2021-10-29T05:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> @@ -42,6 +42,7 @@ const char *git_hooks_path;\n>  int zlib_compression_level = Z_BEST_SPEED;\n>  int pack_compression_level = Z_DEFAULT_COMPRESSION;\n>  int fsync_object_files;\n> +int use_fsync = -1;\n\nOK, (-1) is \"undetermined yet\", as usual.\n\n> diff --git a/git-cvsserver.perl b/git-cvsserver.perl\n> index 64319bed43..4c8118010a 100755\n> --- a/git-cvsserver.perl\n> +++ b/git-cvsserver.perl\n> @@ -3607,6 +3607,22 @@ package GITCVS::updater;\n>  use strict;\n>  use warnings;\n>  use DBI;\n> +our $_use_fsync;\n> +\n> +# n.b. consider using Git.pm\n> +sub use_fsync {\n> +    if (!defined($_use_fsync)) {\n> +        my $x = $ENV{GIT_TEST_FSYNC};\n> +        if (defined $x) {\n\nI would have expected to see \"exists $ENV{GIT_TEST_FSYNC}\", but I\nguess there is no way to place in %ENV anyway, so it would be OK.\n\n> +            local $ENV{GIT_CONFIG};\n> +            delete $ENV{GIT_CONFIG};\n\nOK, \"git -c test.fsync=no cvsserver\" would added something to\nGIT_CONFIG that would affect test.fsync, but wouldn't the usual\nlast-one-wins rule be sufficient to check the value of $x using the\nnext construction, no matter what is in GIT_CONFIG?  I do not think\nit would hurt to delete $ENV{GIT_CONFIG}, but I am not sure how it\nis necessary.\n\n> +            my $v = ::safe_pipe_capture('git', '-c', \"test.fsync=$x\",\n> +                                        qw(config --type=bool test.fsync));\n\nTHis is an interesting idiom.\n\n> +            $_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n> +        }\n> +    }\n> +    $_use_fsync;\n> +}\n\n\n> +# TODO: move this to Git.pm?\n> +sub use_fsync {\n\nPossibly, but in a slightly more general form, taking the name of\nthe environment variable that holds a boolean value as an argument,\nor something?\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index a291a5d4a2..21f5fab999 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -489,6 +489,13 @@ then\n>  \texport GIT_PERL_FATAL_WARNINGS\n>  fi\n>  \n> +case $GIT_TEST_FSYNC in\n> +'')\n> +\tGIT_TEST_FSYNC=0\n> +\texport GIT_TEST_FSYNC\n> +\t;;\n> +esac\n\n> diff --git a/write-or-die.c b/write-or-die.c\n> index 0b1ec8190b..a3d5784cec 100644\n> --- a/write-or-die.c\n> +++ b/write-or-die.c\n> @@ -1,4 +1,5 @@\n>  #include \"cache.h\"\n> +#include \"config.h\"\n>  #include \"run-command.h\"\n>  \n>  /*\n> @@ -57,6 +58,10 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n>  \n>  void fsync_or_die(int fd, const char *msg)\n>  {\n> +\tif (use_fsync < 0)\n> +\t\tuse_fsync = git_env_bool(\"GIT_TEST_FSYNC\", 1);\n> +\tif (!use_fsync)\n> +\t\treturn;\n\nOK.  That's quite straight-forward.\n\n>  \twhile (fsync(fd) < 0) {\n>  \t\tif (errno != EINTR)\n>  \t\t\tdie_errno(\"fsync error on '%s'\", msg);\n\nWill queue.\n"},{"id":"439985","messageId":"xmqqy26cl1zd.fsf@gitster.g","threadId":"56799","inReplyTo":"20211029001552.GA29647@dcvr","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T07:33:10Z","receivedAt":"2021-10-29T07:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> v2 changes:\n> * s/GIT_FSYNC/GIT_TEST_FSYNC/\n> * disable fsync by default for tests, reduces setup for newcomers\n> * fix style nit noted by Eric Sunshine\n\nhttps://github.com/git/git/runs/4043532265?check_suite_focus=true#step:5:70\n\nSeems to be dying in \"git svn\" tests somehow.\n"},{"id":"439986","messageId":"20211029074857.GA9214@dcvr","threadId":"56799","inReplyTo":"xmqqy26cl1zd.fsf@gitster.g","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-29T07:48:57Z","receivedAt":"2021-10-29T07:49:24Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> > v2 changes:\n> > * s/GIT_FSYNC/GIT_TEST_FSYNC/\n> > * disable fsync by default for tests, reduces setup for newcomers\n> > * fix style nit noted by Eric Sunshine\n> \n> https://github.com/git/git/runs/4043532265?check_suite_focus=true#step:5:70\n\nFwiw, I couldn't view that (not sure if it's from lack of JS\nor lack of GH account).  Either way it's accessibility problem.\n\n> Seems to be dying in \"git svn\" tests somehow.\n\nEasy repro+fix, though.  I only tested my final patch with NO_SVN_TESTS :x\nCan you squash this in or do you want a reroll?\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex df5a87a151..6ce2e283c8 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -2274,7 +2274,7 @@ sub use_fsync {\n \tif (!defined($_use_fsync)) {\n \t\tmy $x = $ENV{GIT_TEST_FSYNC};\n \t\tif (defined $x) {\n-\t\t\tmy $v = command_oneline('git', '-c', \"test.fsync=$x\",\n+\t\t\tmy $v = command_oneline('-c', \"test.fsync=$x\",\n \t\t\t\t\tqw(config --type=bool test.fsync));\n \t\t\t$_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n \t\t}\n"},{"id":"439987","messageId":"20211029075640.M183252@dcvr","threadId":"56799","inReplyTo":"xmqqr1c4mmsf.fsf@gitster.g","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-29T07:56:40Z","receivedAt":"2021-10-29T07:56:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> > +# n.b. consider using Git.pm\n> > +sub use_fsync {\n> > +    if (!defined($_use_fsync)) {\n> > +        my $x = $ENV{GIT_TEST_FSYNC};\n> > +        if (defined $x) {\n> \n> I would have expected to see \"exists $ENV{GIT_TEST_FSYNC}\", but I\n> guess there is no way to place in %ENV anyway, so it would be OK.\n\nWas that meant to say: \"no way to place `undef' in %ENV anyway\"?\n\nIf so, `undef' can actually be in Perl's %ENV, though it appears\nto get coerced into \"\" (empty string) when spawning processes.\n\n> > +            local $ENV{GIT_CONFIG};\n> > +            delete $ENV{GIT_CONFIG};\n> \n> OK, \"git -c test.fsync=no cvsserver\" would added something to\n> GIT_CONFIG that would affect test.fsync, but wouldn't the usual\n> last-one-wins rule be sufficient to check the value of $x using the\n> next construction, no matter what is in GIT_CONFIG?  I do not think\n> it would hurt to delete $ENV{GIT_CONFIG}, but I am not sure how it\n> is necessary.\n\nLeaving GIT_CONFIG set was actually causing \"git config\" to\nexit(1) since git-cvsserver sets GIT_CONFIG and the GIT_CONFIG\nfile doesn't have a test.fsync setting.  This is the current\nbehavior, I think it's a weird quirk, but intended behavior of\ngit-config.\n\n# this assumes you don't have foo.bar set in your ~/.gitconfig :>\n$ GIT_CONFIG=$HOME/.gitconfig git -c foo.bar=0 config --type=bool foo.bar\n$ echo $?\n1\n\n> > +            my $v = ::safe_pipe_capture('git', '-c', \"test.fsync=$x\",\n> > +                                        qw(config --type=bool test.fsync));\n> \n> THis is an interesting idiom.\n\nHeh, I just thought of it before sending my original.  I was\ngoing to use a regexp originally (in git-svn, too), but didn't\nwant to get into corner cases such as hex and +/- prefixes).\n\n> > +            $_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n> > +        }\n> > +    }\n> > +    $_use_fsync;\n> > +}\n> \n> \n> > +# TODO: move this to Git.pm?\n> > +sub use_fsync {\n> \n> Possibly, but in a slightly more general form, taking the name of\n> the environment variable that holds a boolean value as an argument,\n> or something?\n\nYeah.  It would've been more useful if git-cvsserver used Git.pm;\nbut I didn't want to introduce Git.pm into git-cvsserver in case\nsomebody relies on git-cvsserver being standalone.\n"},{"id":"439996","messageId":"211029.86v91g3vv3.gmgdl@evledraar.gmail.com","threadId":"56799","inReplyTo":"20211028002102.19384-1-e@80x24.org","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-29T11:20:29Z","receivedAt":"2021-10-29T11:37:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 28 2021, Eric Wong wrote:\n\n> \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n> for disabling fsync() even on packfiles and other critical data.\n\nFirst off, I think this would be useful to add, even as a non-test\nthing.\n\nThere's plenty of use-cases where users don't care about fsync(), and\naside from data reliability it also has implications for say laptop\nbattery use.\n\nI care about my E-Mail, but my E-Mail syncing thingy's (mbsync) has an\nfsync=off in its config, I'm pretty confident that I won't lose power on\nthat *nix laptop, and even if I did I have other recovery methods.\n\nI wouldn't use this in general, but might for some operations, e.g. when\nI'm doing some batch operation with git-annex or whatever.\n\n> Running \"make test -j8 NO_SVN_TESTS=1\" on a noisy 8-core system\n> on an HDD, adding \"GIT_FSYNC=0\" to the invocation brings my test\n> runtime from ~4 minutes down to ~3 minutes.\n\nOn a related topic: Have you tried having it use an empty template\ndirectory? I have some unsubmitted patches for that, and if you retain\nall trash dirs you'll find that we have IIRC 100-200MB of just\naccumulated *.sample etc. hooks, out of ~1GB total for the tests.\n\n> +core.fsync::\n> +\tA boolean to control 'fsync()' on all files.\n> ++\n> +Setting this to false can speed up writes of unimportant data.\n> +Disabling fsync will lead to data loss on power failure.  If set\n> +to false, this also overrides core.fsyncObjectFiles.  Defaults to true.\n> +\n\nPer the recent threads about fsync semantics this should really be\nworded to be more scary, i.e. it's not guaranteed that your data is\non-disk at all. So subsequent git operations might see repo corruption,\nif we're being POSIX-pedantic.\n\nSo some more \"all bets are off\" wording would be better here, and any\nextra promises could be e.g.:\n\n    On commonly deployed OS's such as Linux, OSX etc. a lack of fsync()\n    gives you extra guarantees that are non-standard, primarily that a\n    subsequent running process that accesses the data will see the\n    updated version, as the VFS will serve up the new version, even if\n    it's not on-disk yet. Even these light guarantees may not be\n    something you're getting from our OS. Here be dragons!\n\n> [...]\n> --- a/git-cvsserver.perl\n> +++ b/git-cvsserver.perl\n> @@ -3607,6 +3607,25 @@ package GITCVS::updater;\n>  use strict;\n>  use warnings;\n>  use DBI;\n> +our $_use_fsync;\n\ns/our/my/?\n\n> +# n.b. consider using Git.pm\n> +sub use_fsync {\n> +    if (!defined($_use_fsync)) {\n> +        my $x = $ENV{GIT_FSYNC};\n> +        if (defined $x) {\n> +            local $ENV{GIT_CONFIG};\n> +            delete $ENV{GIT_CONFIG};\n> +            my $v = ::safe_pipe_capture('git', '-c', \"core.fsync=$x\",\n> +                                        qw(config --type=bool core.fsync));\n> +            $_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n> +        } else {\n> +            my $v = `git config --type=bool core.fsync`;\n> +            $_use_fsync = $v eq \"false\\n\" ? 0 : 1;\n> +        }\n> +    }\n> +    $_use_fsync;\n> +}\n\nI wonder most of all if we really need these perl changes, does it\nreally matter for the overall performance that you want, or was it just\nto get all fsync() uses out of the test suite?\n\nThis is a more general comment, but so much of scaffolding like that in\nthe *.perl scripts could go away if we taught git.c some \"not quite a\nbuiltin, but it's ours\" mode, and had say code for git-send-email,\ngit-svn etc. to just set the various data they need in the environment\nbefore we invoke them. Then this would just be say:\n\n    our $use_fsync = $ENV{GIT_FOR_CVSSERVER_FSYNC};\n\n\n> +    if ($self->{dbdriver} eq 'SQLite' && !use_fsync()) {\n> +        $self->{dbh}->do('PRAGMA synchronous = OFF');\n> +    }\n\n..in particular if we're doing this we should update the docs to say\nthat we'll also tweak third-party software's use of fsync() in some\ncases.\n\nEven if we \"own\" that sqlite DB a user might not expect a git option to\nhave gone so far as to be the cause of their on-disk sqlite DB's\ncorruption.\n\n> [...]\n>  \tmy $sync;\n>  \t# both of these options make our .rev_db file very, very important\n>  \t# and we can't afford to lose it because rebuild() won't work\n> -\tif ($self->use_svm_props || $self->no_metadata) {\n> +\tif (($self->use_svm_props || $self->no_metadata) && use_fsync()) {\n>  \t\trequire File::Copy;\n>  \t\t$sync = 1;\n>  \t\tFile::Copy::copy($db, $db_lock) or die \"rev_map_set(@_): \",\n\nThis doesn't just impact $io->sync, but also $io->flush, which is a\ndifferent thing. PerlIO flushing to the OS != fsync().\n\nSo even on OS's that make the above noted guarantees you'd get nothing,\nsince we're now at the mercy of perl not crashing in the interim, not\nkernel or stdlib behavior.\n\n> @@ -57,7 +58,9 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n>  \n>  void fsync_or_die(int fd, const char *msg)\n>  {\n> -\twhile (fsync(fd) < 0) {\n> +\tif (use_fsync < 0)\n> +\t\tuse_fsync = git_env_bool(\"GIT_FSYNC\", 1);\n> +\twhile (use_fsync && fsync(fd) < 0) {\n>  \t\tif (errno != EINTR)\n>  \t\t\tdie_errno(\"fsync error on '%s'\", msg);\n>  \t}\n\nThis patch direction looks good to me, aside from the above we should\nreally update any other fsync() docs we have, maybe with a\ncross-reference in core.fsyncObjectFiles?\n\nI'm not sure offhand what the state of the other fsync patches that\nNeeraj Singh has been submitting is, but let's make sure that whatever\ndocs/config knobs etc. we have all play nicely together, and are\nsomewhat future-proof if possible.\n"},{"id":"440027","messageId":"xmqqtugzlpa3.fsf@gitster.g","threadId":"56799","inReplyTo":"20211029074857.GA9214@dcvr","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T17:22:12Z","receivedAt":"2021-10-29T17:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Easy repro+fix, though.  I only tested my final patch with NO_SVN_TESTS :x\n> Can you squash this in or do you want a reroll?\n>\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> index df5a87a151..6ce2e283c8 100644\n> --- a/perl/Git/SVN.pm\n> +++ b/perl/Git/SVN.pm\n> @@ -2274,7 +2274,7 @@ sub use_fsync {\n>  \tif (!defined($_use_fsync)) {\n>  \t\tmy $x = $ENV{GIT_TEST_FSYNC};\n>  \t\tif (defined $x) {\n> -\t\t\tmy $v = command_oneline('git', '-c', \"test.fsync=$x\",\n> +\t\t\tmy $v = command_oneline('-c', \"test.fsync=$x\",\n>  \t\t\t\t\tqw(config --type=bool test.fsync));\n>  \t\t\t$_use_fsync = defined($v) ? ($v eq \"true\\n\") : 1;\n>  \t\t}\n\nYuck, I should have known that command_oneline() does not want the\nleading 'git'.\n\nA sad part of the story is that this is not exactly the fault of the\ninterface, as the full name of the thing is Git::command_oneline(),\nwhich makes it sufficiently clear that it is a \"git\" specific thing,\nand Git::oneline_result_from_git_command_do_not_give_git_at_front()\nis not a good sub name X-<.\n\nThanks for quickly diagnosing.\n\n\n\n\n"},{"id":"440035","messageId":"xmqqpmrnlmyx.fsf@gitster.g","threadId":"56799","inReplyTo":"20211029075640.M183252@dcvr","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T18:12:06Z","receivedAt":"2021-10-29T18:12:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>> Eric Wong <e@80x24.org> writes:\n>> > +# n.b. consider using Git.pm\n>> > +sub use_fsync {\n>> > +    if (!defined($_use_fsync)) {\n>> > +        my $x = $ENV{GIT_TEST_FSYNC};\n>> > +        if (defined $x) {\n>> \n>> I would have expected to see \"exists $ENV{GIT_TEST_FSYNC}\", but I\n>> guess there is no way to place in %ENV anyway, so it would be OK.\n>\n> Was that meant to say: \"no way to place `undef' in %ENV anyway\"?\n\nYes.  Nothing the external callers of a Perl script does by futzing\nthe environment with setenv(3) and unsetenv(3) can make undef appear\nas a value in $ENV{SomeKey}, so defined $ENV{V} and exists $ENV{V}\nare equivalent.\n\nI still prefer \"exists $ENV{V}\", which I think conveys the intent of\nthe check better, i.e. \"we do this iff the environment variable X is\nthere\".\n\n> If so, `undef' can actually be in Perl's %ENV, though it appears\n> to get coerced into \"\" (empty string) when spawning processes.\n\nYes, but you are talking about the opposite direction, what Perl can\ndo to %ENV to affect processes it spawns, which is not what I meant.\n\n> Leaving GIT_CONFIG set was actually causing \"git config\" to\n> exit(1) since git-cvsserver sets GIT_CONFIG and the GIT_CONFIG\n> file doesn't have a test.fsync setting.  This is the current\n> behavior, I think it's a weird quirk, but intended behavior of\n> git-config.\n\nAh, sorry, I misread the variable.  GIT_CONFIG was the thing that\nsays \"read from this file and nowhere else\"; we do want to disable\nit locally for \"-c var=val\" to take effect.\n\n> # this assumes you don't have foo.bar set in your ~/.gitconfig :>\n> $ GIT_CONFIG=$HOME/.gitconfig git -c foo.bar=0 config --type=bool foo.bar\n> $ echo $?\n> 1\n>\n>> > +            my $v = ::safe_pipe_capture('git', '-c', \"test.fsync=$x\",\n>> > +                                        qw(config --type=bool test.fsync));\n>> \n>> THis is an interesting idiom.\n>\n> Heh, I just thought of it before sending my original.  I was\n> going to use a regexp originally (in git-svn, too), but didn't\n> want to get into corner cases such as hex and +/- prefixes).\n\nAnd this I think is the best way to do so ;-)\n"},{"id":"440060","messageId":"YXxa3i3gXsutG4Cx@coredump.intra.peff.net","threadId":"56799","inReplyTo":"20211029001552.GA29647@dcvr","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-29T20:34:38Z","receivedAt":"2021-10-29T20:34:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 29, 2021 at 12:15:52AM +0000, Eric Wong wrote:\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index a291a5d4a2..21f5fab999 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -489,6 +489,13 @@ then\n>  \texport GIT_PERL_FATAL_WARNINGS\n>  fi\n>  \n> +case $GIT_TEST_FSYNC in\n> +'')\n> +\tGIT_TEST_FSYNC=0\n> +\texport GIT_TEST_FSYNC\n> +\t;;\n> +esac\n\nI don't really have a problem with doing this by default, as it might\nhelp and shouldn't hurt. But I didn't find it actually changed much for\nme. Here are timings running the test suite on my machine:\n\n  Benchmark 1: GIT_TEST_FSYNC=1 make\n    Time (mean ± σ):     60.973 s ±  0.158 s    [User: 575.914 s, System: 333.243 s]\n    Range (min … max):   60.731 s … 61.228 s    10 runs\n\n  Benchmark 2: GIT_TEST_FSYNC=0 make\n    Time (mean ± σ):     59.800 s ±  0.094 s    [User: 575.151 s, System: 337.111 s]\n    Range (min … max):   59.643 s … 59.996 s    10 runs\n\n  Benchmark 3: GIT_TEST_FSYNC=1 make GIT_TEST_OPTS=--root=/var/ram\n    Time (mean ± σ):     56.966 s ±  0.066 s    [User: 572.353 s, System: 300.808 s]\n    Range (min … max):   56.874 s … 57.063 s    10 runs\n\n  Summary\n    'GIT_TEST_FSYNC=1 make GIT_TEST_OPTS=--root=/var/ram' ran\n      1.05 ± 0.00 times faster than 'GIT_TEST_FSYNC=0 make'\n      1.07 ± 0.00 times faster than 'GIT_TEST_FSYNC=1 make'\n\nSo using an actual ram disk provided a much bigger edge for me than just\ndisabling fsync. This may be because the system is using a decent SSD\nfor its actual disk. But it may also be because I'm running 32 tests\nsimultaneously. So fsync introducing latency in tests isn't a big deal;\nthe bottleneck is CPU and there's always another script ready to run.\nThough it is also interesting that the system CPU time is so much lower\nin the tmpfs case.\n\nAgain, not really an objection, but I don't think this replaces the\n\"you're better off running the test suite on a RAM disk\" wisdom.\n\n-Peff\n"},{"id":"440063","messageId":"xmqqo877k1f5.fsf@gitster.g","threadId":"56799","inReplyTo":"YXxa3i3gXsutG4Cx@coredump.intra.peff.net","subject":"Re: [PATCH] tests: disable fsync everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:42:54Z","receivedAt":"2021-10-29T20:43:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Again, not really an objection, but I don't think this replaces the\n> \"you're better off running the test suite on a RAM disk\" wisdom.\n\nNice summary.  This was sold as \"not everybody has ready access to\ntmpfs\", so those of us who can use either should stick to our tmpfs\nas we have done so far.\n\n"},{"id":"440120","messageId":"20211030103950.M489266@dcvr","threadId":"56799","inReplyTo":"211029.86v91g3vv3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] allow disabling fsync everywhere","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-10-30T10:39:50Z","receivedAt":"2021-10-30T10:49:31Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> On Thu, Oct 28 2021, Eric Wong wrote:\n> \n> > \"core.fsync\" and the \"GIT_FSYNC\" environment variable now exist\n> > for disabling fsync() even on packfiles and other critical data.\n> \n> First off, I think this would be useful to add, even as a non-test\n> thing.\n\n<snip>\n\nAgreed, I've LD_PRELOAD-ed libeatmydata via ~/.bashrc at times :>\n\nOTOH, I understand Junio and brian's positions on dealing with\nmisinformed users losing data.  I'm also lazy when it comes to\ndocs and support, so I'm leaning towards keeping this exclusively\nfor those who read source code.\n\nI also know it's easy to add a sysctl knob for disabling fsync\net. al. in the kernel; but it's not something I want to deal\nwith supporting and documenting, either.\n\n> > Running \"make test -j8 NO_SVN_TESTS=1\" on a noisy 8-core system\n> > on an HDD, adding \"GIT_FSYNC=0\" to the invocation brings my test\n> > runtime from ~4 minutes down to ~3 minutes.\n> \n> On a related topic: Have you tried having it use an empty template\n> directory? I have some unsubmitted patches for that, and if you retain\n> all trash dirs you'll find that we have IIRC 100-200MB of just\n> accumulated *.sample etc. hooks, out of ~1GB total for the tests.\n\nNot, not yet.  I also expect using reflinks on some Linux FSes\nwill help users save space in real-world use\n(ioctl(dst_fd, FICLONE, src_fd)).\n\n> Per the recent threads about fsync semantics this should really be\n> worded to be more scary, i.e. it's not guaranteed that your data is\n> on-disk at all. So subsequent git operations might see repo corruption,\n> if we're being POSIX-pedantic.\n\nYeah, I wasn't sure how strongly to word it.  Now it's\nundocumented; I suppose we can use your wording if we decide\nit's worth documenting.\n\n> > --- a/git-cvsserver.perl\n> > +++ b/git-cvsserver.perl\n> > @@ -3607,6 +3607,25 @@ package GITCVS::updater;\n> >  use strict;\n> >  use warnings;\n> >  use DBI;\n> > +our $_use_fsync;\n> \n> s/our/my/?\n\nErm, yes.  Though it doesn't matter for a standalone script;\nprobably not worth a reroll..\n\n> I wonder most of all if we really need these perl changes, does it\n> really matter for the overall performance that you want, or was it just\n> to get all fsync() uses out of the test suite?\n\nYes to both, or at least an attempt to...  A single fsync() can\nend up being equivalent to syncfs().\n\nFully-disabling fsync in SVN doesn't seem possible, though.\nThere's a few svnadmin switches (--no-flush-to-disk +\n--bdb-txn-nosync) but AFAIK they aren't 100% solutions.\nFortuntanately, NO_SVN_TESTS=1 exists.\n\n> This is a more general comment, but so much of scaffolding like that in\n> the *.perl scripts could go away if we taught git.c some \"not quite a\n> builtin, but it's ours\" mode, and had say code for git-send-email,\n> git-svn etc. to just set the various data they need in the environment\n> before we invoke them. Then this would just be say:\n> \n>     our $use_fsync = $ENV{GIT_FOR_CVSSERVER_FSYNC};\n\nThat's potentially a lot of environment variables, though.\nMaybe just:  eval($ENV{GIT_CFG_CVSSERVER}) if $ENV{GIT_CFG_PID} == $$;\nThe PID check should be sufficient to avoid mismatches and\nmalicious code injection.\n\n> > [...]\n> >  \tmy $sync;\n> >  \t# both of these options make our .rev_db file very, very important\n> >  \t# and we can't afford to lose it because rebuild() won't work\n> > -\tif ($self->use_svm_props || $self->no_metadata) {\n> > +\tif (($self->use_svm_props || $self->no_metadata) && use_fsync()) {\n> >  \t\trequire File::Copy;\n> >  \t\t$sync = 1;\n> >  \t\tFile::Copy::copy($db, $db_lock) or die \"rev_map_set(@_): \",\n> \n> This doesn't just impact $io->sync, but also $io->flush, which is a\n> different thing. PerlIO flushing to the OS != fsync().\n\nRight, close($fh) (right below[1]) also takes care of PerlIO\nflushing.  ->flush was only there for fsync.\n\n[1] https://yhbt.net/lore/git/7b333ea62e/s/?b=perl/Git/SVN.pm#n2334\n\n> This patch direction looks good to me, aside from the above we should\n> really update any other fsync() docs we have, maybe with a\n> cross-reference in core.fsyncObjectFiles?\n\nAlright, though I'm happy with it being undocumented, for now.\n\n> I'm not sure offhand what the state of the other fsync patches that\n> Neeraj Singh has been submitting is, but let's make sure that whatever\n> docs/config knobs etc. we have all play nicely together, and are\n> somewhat future-proof if possible.\n\nIt looks like it some changes will be necessary to keep tests fast\nafter that lands.\n"}]}