Re: [PATCH] Enable core.fsyncObjectFiles by default

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2015, 22:21 UTC
Message-ID: <xmqqpp4maww7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1435096643-18159-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 16 quoted lines

> Linus Torvalds started a discussion[1] if we want to play rather safe
> than use defaults which make sense only for the most power users of Git:
>
>> So git is "safe" in the sense that you won't really lose any data,
>> but you may well be inconvenienced.  The "fsync each object" config
>> option is there in case you don't want that inconvenience, but it
>> should be noted that it can make for a hell of a performance impact.
>
>> Of course, it might well be the case that the actual default
>> might be worth turning around. Most git users probably don't
>> care about that kind of "apply two hundred patches from Andrew
>> Morton" kind of workload, although "rebase a big patch-series"
>> does end up doing basically the same thing, and might be more
>> common.
>
> This patch enables fsync_object_files by default.

Sorry, but I fail to see which part of what Linus said (which is the only thing you quoted from the discussion) argues for this patch. If anything, I read that as cautioning people against making a tradeoff based on an incorrect perception of risks and blindly flipping this bit ON (the original discussion a few days ago, where Ted says he has this bit ON while clarifying that he does so on SSD, is also a sensible description on how he made his trade-off).

It's a different matter whom you would want to align with when assessing your own risk tolerance. If you quoted Corbet's original message, then that would have been more consistent.

Show 41 quoted lines

>
> [1] https://plus.google.com/u/1/+JonathanCorbet/posts/JBxiKPe3VXa
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/config.txt | 8 ++++----
>  environment.c            | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 43bb53c..dce2640 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -693,10 +693,10 @@ core.whitespace::
>  core.fsyncObjectFiles::
>  	This boolean will enable 'fsync()' when writing object files.
>  +
> -This is a total waste of time and effort on a filesystem that orders
> -data writes properly, but can be useful for filesystems that do not use
> -journalling (traditional UNIX filesystems) or that only journal metadata
> -and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
> +This ensures objects are written to disk instead of relying on the
> +operating systems cache and eventual write. Disabling this option will
> +yield performance with a trade off in safety for repository corruption
> +during power loss.
>  
>  core.preloadIndex::
>  	Enable parallel index preload for operations like 'git diff'
> diff --git a/environment.c b/environment.c
> index 61c685b..b406f5e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -35,7 +35,7 @@ const char *git_attributes_file;
>  int zlib_compression_level = Z_BEST_SPEED;
>  int core_compression_level;
>  int core_compression_seen;
> -int fsync_object_files;
> +int fsync_object_files = 1;
>  size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
>  size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
>  size_t delta_base_cache_limit = 96 * 1024 * 1024;