{"thread":{"id":"39705","subject":"[PATCH] Enable core.fsyncObjectFiles by default","startedAt":"2015-06-23T21:57:23Z","lastAt":"2015-06-24T14:30:07Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano","Theodore Ts'o","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264696","messageId":"1435096643-18159-1-git-send-email-sbeller@google.com","threadId":"39705","inReplyTo":null,"subject":"[PATCH] Enable core.fsyncObjectFiles by default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-23T21:57:23Z","receivedAt":"2015-06-23T21:57:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Linus Torvalds started a discussion[1] if we want to play rather safe\nthan use defaults which make sense only for the most power users of Git:\n\n> So git is \"safe\" in the sense that you won't really lose any data,\n> but you may well be inconvenienced.  The \"fsync each object\" config\n> option is there in case you don't want that inconvenience, but it\n> should be noted that it can make for a hell of a performance impact.\n\n> Of course, it might well be the case that the actual default\n> might be worth turning around. Most git users probably don't\n> care about that kind of \"apply two hundred patches from Andrew\n> Morton\" kind of workload, although \"rebase a big patch-series\"\n> does end up doing basically the same thing, and might be more\n> common.\n\nThis patch enables fsync_object_files by default.\n\n[1] https://plus.google.com/u/1/+JonathanCorbet/posts/JBxiKPe3VXa\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/config.txt | 8 ++++----\n environment.c            | 2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 43bb53c..dce2640 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -693,10 +693,10 @@ 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 ensures objects are written to disk instead of relying on the\n+operating systems cache and eventual write. Disabling this option will\n+yield performance with a trade off in safety for repository corruption\n+during power loss.\n \n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\ndiff --git a/environment.c b/environment.c\nindex 61c685b..b406f5e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,7 +35,7 @@ const char *git_attributes_file;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int core_compression_seen;\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.4.1.345.gab207b6.dirty\n"},{"id":"264697","messageId":"xmqqpp4maww7.fsf@gitster.dls.corp.google.com","threadId":"39705","inReplyTo":"1435096643-18159-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T22:21:44Z","receivedAt":"2015-06-23T22:21:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Linus Torvalds started a discussion[1] if we want to play rather safe\n> than use defaults which make sense only for the most power users of Git:\n>\n>> So git is \"safe\" in the sense that you won't really lose any data,\n>> but you may well be inconvenienced.  The \"fsync each object\" config\n>> option is there in case you don't want that inconvenience, but it\n>> should be noted that it can make for a hell of a performance impact.\n>\n>> Of course, it might well be the case that the actual default\n>> might be worth turning around. Most git users probably don't\n>> care about that kind of \"apply two hundred patches from Andrew\n>> Morton\" kind of workload, although \"rebase a big patch-series\"\n>> does end up doing basically the same thing, and might be more\n>> common.\n>\n> This patch enables fsync_object_files by default.\n\nSorry, but I fail to see which part of what Linus said (which is the\nonly thing you quoted from the discussion) argues for this patch.\nIf anything, I read that as cautioning people against making a\ntradeoff based on an incorrect perception of risks and blindly\nflipping this bit ON (the original discussion a few days ago, where\nTed says he has this bit ON while clarifying that he does so on SSD,\nis also a sensible description on how he made his trade-off).\n\nIt's a different matter whom you would want to align with when\nassessing your own risk tolerance.  If you quoted Corbet's original\nmessage, then that would have been more consistent.\n\n>\n> [1] https://plus.google.com/u/1/+JonathanCorbet/posts/JBxiKPe3VXa\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  Documentation/config.txt | 8 ++++----\n>  environment.c            | 2 +-\n>  2 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 43bb53c..dce2640 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -693,10 +693,10 @@ 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 ensures objects are written to disk instead of relying on the\n> +operating systems cache and eventual write. Disabling this option will\n> +yield performance with a trade off in safety for repository corruption\n> +during power loss.\n>  \n>  core.preloadIndex::\n>  \tEnable parallel index preload for operations like 'git diff'\n> diff --git a/environment.c b/environment.c\n> index 61c685b..b406f5e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -35,7 +35,7 @@ const char *git_attributes_file;\n>  int zlib_compression_level = Z_BEST_SPEED;\n>  int core_compression_level;\n>  int core_compression_seen;\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"},{"id":"264698","messageId":"20150623232921.GB14324@thunk.org","threadId":"39705","inReplyTo":"xmqqpp4maww7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2015-06-23T23:29:21Z","receivedAt":"2015-06-23T23:29:21Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"The main issue is that non-expert users might not realize that they\nreally need to run \"git fsck\" after a crash; otherwise, what might\nhappen is that although git is only appending, that you might have\nsome zero-length (or partially written) git object or pack files, and\nwhile you might not notice at that moment, it might come and bite you\nlater.  If you do try to recover immediately after a crash, in the\nworst case you might have to do that \"git am -s /tmp/mbox-filled-with\npatches\" command, but otherwise you won't lose much data.\n\nSo perhaps one alternative strategy to calling fsync(2) after every\nsingle git object file write might be to have git create a zero-length\n\".git/in-progress-<pid>\" file, which gets fsync'ed, and then it can do\nthe \"git am -s /tmp/akpm-big-bag-o-patches\" processing nice and fast,\nand once git is done, then we call call sync(2) and then delete the\nin-progress file.\n\nIf there is an in-progress file in the .git directory, git would then\nautomatically run git fsck to make sure the repository is consistent.\n\nFor people who care, maybe that's a good compromise.  (Me, the way\nthings are right now is just fine since I have a nice fast SSD, and so\nsetting fsyncObjectfiles is a perfectly fine thing as far as I am\nconcerned. :-)\n\n\t\t      \t\t     \t   - Ted\n"},{"id":"264699","messageId":"CACsJy8AZ7TOqZLKUD=tVaBVMH4af_jg42Y04ZryQ9dhTSoGu5A@mail.gmail.com","threadId":"39705","inReplyTo":"1435096643-18159-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-24T01:07:23Z","receivedAt":"2015-06-24T01:07:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 24, 2015 at 4:57 AM, Stefan Beller <sbeller@google.com> wrote:\n> Linus Torvalds started a discussion[1] if we want to play rather safe\n> than use defaults which make sense only for the most power users of Git:\n>\n>> So git is \"safe\" in the sense that you won't really lose any data,\n>> but you may well be inconvenienced.  The \"fsync each object\" config\n>> option is there in case you don't want that inconvenience, but it\n>> should be noted that it can make for a hell of a performance impact.\n>\n>> Of course, it might well be the case that the actual default\n>> might be worth turning around. Most git users probably don't\n>> care about that kind of \"apply two hundred patches from Andrew\n>> Morton\" kind of workload, although \"rebase a big patch-series\"\n>> does end up doing basically the same thing, and might be more\n>> common.\n>\n> This patch enables fsync_object_files by default.\n\nWill this make nfs performance a lot worse or still within acceptable range?\n-- \nDuy\n"},{"id":"264701","messageId":"20150624033725.GA19321@peff.net","threadId":"39705","inReplyTo":"1435096643-18159-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-24T03:37:25Z","receivedAt":"2015-06-24T03:37:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2015 at 02:57:23PM -0700, Stefan Beller wrote:\n\n> Linus Torvalds started a discussion[1] if we want to play rather safe\n> than use defaults which make sense only for the most power users of Git:\n> \n> > So git is \"safe\" in the sense that you won't really lose any data,\n> > but you may well be inconvenienced.  The \"fsync each object\" config\n> > option is there in case you don't want that inconvenience, but it\n> > should be noted that it can make for a hell of a performance impact.\n> \n> > Of course, it might well be the case that the actual default\n> > might be worth turning around. Most git users probably don't\n> > care about that kind of \"apply two hundred patches from Andrew\n> > Morton\" kind of workload, although \"rebase a big patch-series\"\n> > does end up doing basically the same thing, and might be more\n> > common.\n> \n> This patch enables fsync_object_files by default.\n\nIf you are looking for safety out of the box, I think this falls far\nshort, as we do not fsync all of the other files. For instance, we do\nnot fsync refs before they are written (nor anything else that uses the\ncommit_lock_file() interface to rename, such as the index).  We do\nalways fsync packfiles and their indices.\n\nI had always assumed this was fine on ext4 with data=ordered (i.e.,\neither the rename and its pointed-to content will go through, or not; so\nyou either get your update or the old state, but not a garbage or empty\nfile). But it sounds from what Ted wrote in:\n\n  http://article.gmane.org/gmane.linux.file-systems/97255\n\nthat this may not be the case. If it's not, I think we should consider\nfsyncing ref writes.\n\n-Peff\n"},{"id":"264703","messageId":"xmqqlhf9bs2b.fsf@gitster.dls.corp.google.com","threadId":"39705","inReplyTo":"20150624033725.GA19321@peff.net","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T05:20:44Z","receivedAt":"2015-06-24T05:20:44Z","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> I had always assumed this was fine on ext4 with data=ordered (i.e.,\n> either the rename and its pointed-to content will go through, or not; so\n> you either get your update or the old state, but not a garbage or empty\n> file). But it sounds from what Ted wrote in:\n>\n>   http://article.gmane.org/gmane.linux.file-systems/97255\n>\n> that this may not be the case. If it's not, I think we should consider\n> fsyncing ref writes.\n\nThat matches my understanding.  IIRC, we do fsync (without a knob to\nturn it off) packs, we do not fsync refs (as you described above),\nthe index, or working tree files (via write_entry()).  Among these:\n\n * If we are so paranoid that loss of new loose objects matter (as\n   opposed to \"Heh, it is just the matter of 'git add' again\") to\n   cause us to set core.fsyncObjectFiles, we should definitely fsync\n   ref writes.  They are even more precious.\n\n * The index is much less precious.  In the worst case, you can 'git\n   reset' (no flags) and re-add from the working tree and nothing\n   unrecoverable is lost.  I do not mind a knob to force us fsync,\n   but we may want it to be separate from core.fsyncObjectFiles.\n\n * The working tree files?  I am not sure.  Again, recovery is just\n   to run \"git diff\" to notice what was eaten by the power failure\n   and then \"git checkout $path\" to restore from a known state,\n   so...\n"},{"id":"264704","messageId":"xmqqegl1brjb.fsf@gitster.dls.corp.google.com","threadId":"39705","inReplyTo":"20150623232921.GB14324@thunk.org","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T05:32:08Z","receivedAt":"2015-06-24T05:32:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Theodore Ts'o <tytso@mit.edu> writes:\n\n> The main issue is that non-expert users might not realize that they\n> really need to run \"git fsck\" after a crash; otherwise, what might\n> happen is that although git is only appending, that you might have\n> some zero-length (or partially written) git object or pack files, and\n> while you might not notice at that moment, it might come and bite you\n> later.\n\nRegarding loose object files, given that we write to a temporary,\noptionally fsync, close and then move to the final name, would we\nstill see partially written file if we omit the fsync, or would the\ncorruption be limited to either empty or missing?\n\nThe reason I am wondering is because the codepath to create an\nobject (i.e. \"update-index --add\", \"hash-object -w\", or \"add\") first\nchecks if a packed or a loose object file _exists_ and if so\nbypasses writing the same thing anew, but the existence check for a\nloose object is to merely making sure that access(F_OK) (and\noptionally utime()) succeeds.  If the potential breakage is limited\nto truncation to empty, then we could replace it with stat(2) and\nst.st_size check, as no loose object file can be empty.\n"},{"id":"264723","messageId":"20150624143007.GC14324@thunk.org","threadId":"39705","inReplyTo":"xmqqegl1brjb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Enable core.fsyncObjectFiles by default","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2015-06-24T14:30:07Z","receivedAt":"2015-06-24T14:30:07Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Tue, Jun 23, 2015 at 10:32:08PM -0700, Junio C Hamano wrote:\n> \n> Regarding loose object files, given that we write to a temporary,\n> optionally fsync, close and then move to the final name, would we\n> still see partially written file if we omit the fsync, or would the\n> corruption be limited to either empty or missing?\n\n*Most* of the time the corruption will be an empty or missing file.\nIt's possible that the file could be partially written.  This is a\nrelatively low-probability event, with the probability going up if the\nobject file is large, and/or if the system is under memory pressure.\n\n> The reason I am wondering is because the codepath to create an\n> object (i.e. \"update-index --add\", \"hash-object -w\", or \"add\") first\n> checks if a packed or a loose object file _exists_ and if so\n> bypasses writing the same thing anew, but the existence check for a\n> loose object is to merely making sure that access(F_OK) (and\n> optionally utime()) succeeds.  If the potential breakage is limited\n> to truncation to empty, then we could replace it with stat(2) and\n> st.st_size check, as no loose object file can be empty.\n\nIt would certainly be a good thing to do a st_size check; it can't\npossible hurt, and it will catch a large number of failures after a\npower failure.  I could also imagine some hueristics that force an\nfsync if the object file is larger than a certain size (say, 4k if you\nare very paranoid, a few hundred kilobytes if you are less so), but\npast a certain point, it might be better just to tell the user to use\nfsyncObjectFiles and be done with it.\n\n\t\t\t\t\t\t- Ted\n"}]}