From abf38abec201cded6094801766d69e11a6c112b6 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 10 Mar 2022 22:43:20 +0000 Subject: core.fsyncmethod: add writeout-only mode This commit introduces the `core.fsyncMethod` configuration knob, which can currently be set to `fsync` or `writeout-only`. The new writeout-only mode attempts to tell the operating system to flush its in-memory page cache to the storage hardware without issuing a CACHE_FLUSH command to the storage controller. Writeout-only fsync is significantly faster than a vanilla fsync on common hardware, since data is written to a disk-side cache rather than all the way to a durable medium. Later changes in this patch series will take advantage of this primitive to implement batching of hardware flushes. When git_fsync is called with FSYNC_WRITEOUT_ONLY, it may fail and the caller is expected to do an ordinary fsync as needed. On Apple platforms, the fsync system call does not issue a CACHE_FLUSH directive to the storage controller. This change updates fsync to do fcntl(F_FULLFSYNC) to make fsync actually durable. We maintain parity with existing behavior on Apple platforms by setting the default value of the new core.fsyncMethod option. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- environment.c | 1 + 1 file changed, 1 insertion(+) (limited to 'environment.c') diff --git a/environment.c b/environment.c index fd0501e77a..3e3620d759 100644 --- a/environment.c +++ b/environment.c @@ -44,6 +44,7 @@ int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; int fsync_object_files; int use_fsync = -1; +enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT; 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; -- cgit v1.3 From 020406eaa52e67440d9b78087ec2ce25532cb219 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 10 Mar 2022 22:43:21 +0000 Subject: core.fsync: introduce granular fsync control infrastructure This commit introduces the infrastructure for the core.fsync configuration knob. The repository components we want to sync are identified by flags so that we can turn on or off syncing for specific components. If core.fsyncObjectFiles is set and the core.fsync configuration also includes FSYNC_COMPONENT_LOOSE_OBJECT, we will fsync any loose objects. This picks the strictest data integrity behavior if core.fsync and core.fsyncObjectFiles are set to conflicting values. This change introduces the currently unused fsync_component helper, which will be used by a later patch that adds fsyncing to the refs backend. Actual configuration and documentation of the fsync components list are in other patches in the series to separate review of the underlying mechanism from the policy of how it's configured. Helped-by: Patrick Steinhardt Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- builtin/fast-import.c | 2 +- builtin/index-pack.c | 4 ++-- builtin/pack-objects.c | 24 +++++++++++++++++------- bulk-checkin.c | 5 +++-- cache.h | 23 +++++++++++++++++++++++ commit-graph.c | 3 ++- csum-file.c | 5 +++-- csum-file.h | 3 ++- environment.c | 1 + midx.c | 3 ++- object-file.c | 13 +++++++++---- pack-bitmap-write.c | 3 ++- pack-write.c | 13 +++++++------ read-cache.c | 2 +- write-or-die.c | 26 ++++++++++++++++++++++---- 15 files changed, 97 insertions(+), 33 deletions(-) (limited to 'environment.c') diff --git a/builtin/fast-import.c b/builtin/fast-import.c index b7105fcad9..f2c036a895 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -865,7 +865,7 @@ static void end_packfile(void) struct tag *t; close_pack_windows(pack_data); - finalize_hashfile(pack_file, cur_pack_oid.hash, 0); + finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0); fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash, pack_data->pack_name, object_count, cur_pack_oid.hash, pack_size); diff --git a/builtin/index-pack.c b/builtin/index-pack.c index c45273de3b..c5f12f14df 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1290,7 +1290,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha nr_objects - nr_objects_initial); stop_progress_msg(&progress, msg.buf); strbuf_release(&msg); - finalize_hashfile(f, tail_hash, 0); + finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0); hashcpy(read_hash, pack_hash); fixup_pack_header_footer(output_fd, pack_hash, curr_pack, nr_objects, @@ -1512,7 +1512,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, if (!from_stdin) { close(input_fd); } else { - fsync_or_die(output_fd, curr_pack_name); + fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name); err = close(output_fd); if (err) die_errno(_("error while closing pack file")); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 178e611f09..c14fee8e99 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1199,16 +1199,26 @@ static void write_pack_file(void) display_progress(progress_state, written); } - /* - * Did we write the wrong # entries in the header? - * If so, rewrite it like in fast-import - */ if (pack_to_stdout) { - finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE); + /* + * We never fsync when writing to stdout since we may + * not be writing to an actual pack file. For instance, + * the upload-pack code passes a pipe here. Calling + * fsync on a pipe results in unnecessary + * synchronization with the reader on some platforms. + */ + finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE, + CSUM_HASH_IN_STREAM | CSUM_CLOSE); } else if (nr_written == nr_remaining) { - finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); + finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, + CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); } else { - int fd = finalize_hashfile(f, hash, 0); + /* + * If we wrote the wrong number of entries in the + * header, rewrite it like in fast-import. + */ + + int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0); fixup_pack_header_footer(fd, hash, pack_tmp_name, nr_written, hash, offset); close(fd); diff --git a/bulk-checkin.c b/bulk-checkin.c index 8785b2ac80..a2cf9dcbc8 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -53,9 +53,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state) unlink(state->pack_tmp_name); goto clear_exit; } else if (state->nr_written == 1) { - finalize_hashfile(state->f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); + finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, + CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); } else { - int fd = finalize_hashfile(state->f, hash, 0); + int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0); fixup_pack_header_footer(fd, hash, state->pack_tmp_name, state->nr_written, hash, state->offset); diff --git a/cache.h b/cache.h index 82f0194a3d..7ac1959258 100644 --- a/cache.h +++ b/cache.h @@ -993,6 +993,27 @@ void reset_shared_repository(void); extern int read_replace_refs; extern char *git_replace_ref_base; +/* + * These values are used to help identify parts of a repository to fsync. + * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the + * repository and so shouldn't be fsynced. + */ +enum fsync_component { + FSYNC_COMPONENT_NONE, + FSYNC_COMPONENT_LOOSE_OBJECT = 1 << 0, + FSYNC_COMPONENT_PACK = 1 << 1, + FSYNC_COMPONENT_PACK_METADATA = 1 << 2, + FSYNC_COMPONENT_COMMIT_GRAPH = 1 << 3, +}; + +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \ + FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH) + +/* + * A bitmask indicating which components of the repo should be fsynced. + */ +extern enum fsync_component fsync_components; extern int fsync_object_files; extern int use_fsync; @@ -1707,6 +1728,8 @@ int copy_file_with_time(const char *dst, const char *src, int mode); void write_or_die(int fd, const void *buf, size_t count); void fsync_or_die(int fd, const char *); +int fsync_component(enum fsync_component component, int fd); +void fsync_component_or_die(enum fsync_component component, int fd, const char *msg); ssize_t read_in_full(int fd, void *buf, size_t count); ssize_t write_in_full(int fd, const void *buf, size_t count); diff --git a/commit-graph.c b/commit-graph.c index 265c010122..64897f57d9 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1942,7 +1942,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) } close_commit_graph(ctx->r->objects); - finalize_hashfile(f, file_hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC); + finalize_hashfile(f, file_hash, FSYNC_COMPONENT_COMMIT_GRAPH, + CSUM_HASH_IN_STREAM | CSUM_FSYNC); free_chunkfile(cf); if (ctx->split) { diff --git a/csum-file.c b/csum-file.c index 26e8a6df44..59ef3398ca 100644 --- a/csum-file.c +++ b/csum-file.c @@ -58,7 +58,8 @@ static void free_hashfile(struct hashfile *f) free(f); } -int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags) +int finalize_hashfile(struct hashfile *f, unsigned char *result, + enum fsync_component component, unsigned int flags) { int fd; @@ -69,7 +70,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl if (flags & CSUM_HASH_IN_STREAM) flush(f, f->buffer, the_hash_algo->rawsz); if (flags & CSUM_FSYNC) - fsync_or_die(f->fd, f->name); + fsync_component_or_die(component, f->fd, f->name); if (flags & CSUM_CLOSE) { if (close(f->fd)) die_errno("%s: sha1 file error on close", f->name); diff --git a/csum-file.h b/csum-file.h index 291215b34e..0d29f528fb 100644 --- a/csum-file.h +++ b/csum-file.h @@ -1,6 +1,7 @@ #ifndef CSUM_FILE_H #define CSUM_FILE_H +#include "cache.h" #include "hash.h" struct progress; @@ -38,7 +39,7 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *); struct hashfile *hashfd(int fd, const char *name); struct hashfile *hashfd_check(const char *name); struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp); -int finalize_hashfile(struct hashfile *, unsigned char *, unsigned int); +int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int); void hashwrite(struct hashfile *, const void *, unsigned int); void hashflush(struct hashfile *f); void crc32_begin(struct hashfile *); diff --git a/environment.c b/environment.c index 3e3620d759..36ca5fb2e7 100644 --- a/environment.c +++ b/environment.c @@ -45,6 +45,7 @@ int pack_compression_level = Z_DEFAULT_COMPRESSION; int fsync_object_files; int use_fsync = -1; enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT; +enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT; 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; diff --git a/midx.c b/midx.c index 865170bad0..107365d211 100644 --- a/midx.c +++ b/midx.c @@ -1438,7 +1438,8 @@ static int write_midx_internal(const char *object_dir, write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs); write_chunkfile(cf, &ctx); - finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM); + finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA, + CSUM_FSYNC | CSUM_HASH_IN_STREAM); free_chunkfile(cf); if (flags & MIDX_WRITE_REV_INDEX && diff --git a/object-file.c b/object-file.c index 03bd6a3baf..e3f0bf27ff 100644 --- a/object-file.c +++ b/object-file.c @@ -1849,11 +1849,16 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf, /* Finalize a file on disk, and close it. */ static void close_loose_object(int fd) { - if (!the_repository->objects->odb->will_destroy) { - if (fsync_object_files) - fsync_or_die(fd, "loose object file"); - } + if (the_repository->objects->odb->will_destroy) + goto out; + if (fsync_object_files > 0) + fsync_or_die(fd, "loose object file"); + else + fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd, + "loose object file"); + +out: if (close(fd) != 0) die_errno(_("error when closing loose object file")); } diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index cab3eaa2ac..cf681547f2 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -719,7 +719,8 @@ void bitmap_writer_finish(struct pack_idx_entry **index, if (options & BITMAP_OPT_HASH_CACHE) write_hash_cache(f, index, index_nr); - finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); + finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA, + CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); if (adjust_shared_perm(tmp_file.buf)) die_errno("unable to make temporary bitmap file readable"); diff --git a/pack-write.c b/pack-write.c index a5846f3a34..51812cb129 100644 --- a/pack-write.c +++ b/pack-write.c @@ -159,9 +159,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec } hashwrite(f, sha1, the_hash_algo->rawsz); - finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE | - ((opts->flags & WRITE_IDX_VERIFY) - ? 0 : CSUM_FSYNC)); + finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA, + CSUM_HASH_IN_STREAM | CSUM_CLOSE | + ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC)); return index_name; } @@ -281,8 +281,9 @@ const char *write_rev_file_order(const char *rev_name, if (rev_name && adjust_shared_perm(rev_name) < 0) die(_("failed to make %s readable"), rev_name); - finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE | - ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC)); + finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA, + CSUM_HASH_IN_STREAM | CSUM_CLOSE | + ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC)); return rev_name; } @@ -390,7 +391,7 @@ void fixup_pack_header_footer(int pack_fd, the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx); the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx); write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz); - fsync_or_die(pack_fd, pack_name); + fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name); } char *index_pack_lockfile(int ip_out, int *is_well_formed) diff --git a/read-cache.c b/read-cache.c index 79b9b99ebf..df869691fd 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3089,7 +3089,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, return -1; } - finalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM); + finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_NONE, CSUM_HASH_IN_STREAM); if (close_tempfile_gently(tempfile)) { error(_("could not close '%s'"), get_tempfile_path(tempfile)); return -1; diff --git a/write-or-die.c b/write-or-die.c index 9faa5f9f56..c4fd91b5b4 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -56,21 +56,39 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) } } -void fsync_or_die(int fd, const char *msg) +static int maybe_fsync(int fd) { if (use_fsync < 0) use_fsync = git_env_bool("GIT_TEST_FSYNC", 1); if (!use_fsync) - return; + return 0; if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY && git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) - return; + return 0; + + return git_fsync(fd, FSYNC_HARDWARE_FLUSH); +} - if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) +void fsync_or_die(int fd, const char *msg) +{ + if (maybe_fsync(fd) < 0) die_errno("fsync error on '%s'", msg); } +int fsync_component(enum fsync_component component, int fd) +{ + if (fsync_components & component) + return maybe_fsync(fd); + return 0; +} + +void fsync_component_or_die(enum fsync_component component, int fd, const char *msg) +{ + if (fsync_components & component) + fsync_or_die(fd, msg); +} + void write_or_die(int fd, const void *buf, size_t count) { if (write_in_full(fd, buf, count) < 0) { -- cgit v1.3 From 844a8ad4f868dcac8851012fe6dafd49b301b2ae Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Thu, 10 Mar 2022 22:43:22 +0000 Subject: core.fsync: add configuration parsing This change introduces code to parse the core.fsync setting and configure the fsync_components variable. core.fsync is configured as a comma-separated list of component names to sync. Each time a core.fsync variable is encountered in the configuration heirarchy, we start off with a clean state with the platform default value. Passing 'none' resets the value to indicate nothing will be synced. We gather all negative and positive entries from the comma separated list and then compute the new value by removing all the negative entries and adding all of the positive entries. We issue a warning for components that are not recognized so that the configuration code is compatible with configs from future versions of Git with more repo components. Complete documentation for the new setting is included in a later patch in the series so that it can be reviewed once in final form. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- Documentation/config/core.txt | 9 ++--- config.c | 76 +++++++++++++++++++++++++++++++++++++++++++ environment.c | 2 +- 3 files changed, 82 insertions(+), 5 deletions(-) (limited to 'environment.c') diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index dbb134f713..ab911d6e26 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -558,11 +558,12 @@ core.fsyncMethod:: core.fsyncObjectFiles:: This boolean will enable 'fsync()' when writing object files. + This setting is deprecated. Use core.fsync instead. + -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 setting affects data added to the Git repository in loose-object +form. When set to true, Git will issue an fsync or similar system call +to flush caches so that loose-objects remain consistent in the face +of a unclean system shutdown. core.preloadIndex:: Enable parallel index preload for operations like 'git diff' diff --git a/config.c b/config.c index f3ff80b01c..94a4598b5f 100644 --- a/config.c +++ b/config.c @@ -1323,6 +1323,73 @@ static int git_parse_maybe_bool_text(const char *value) return -1; } +static const struct fsync_component_name { + const char *name; + enum fsync_component component_bits; +} fsync_component_names[] = { + { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT }, + { "pack", FSYNC_COMPONENT_PACK }, + { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA }, + { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, +}; + +static enum fsync_component parse_fsync_components(const char *var, const char *string) +{ + enum fsync_component current = FSYNC_COMPONENTS_DEFAULT; + enum fsync_component positive = 0, negative = 0; + + while (string) { + int i; + size_t len; + const char *ep; + int negated = 0; + int found = 0; + + string = string + strspn(string, ", \t\n\r"); + ep = strchrnul(string, ','); + len = ep - string; + if (!strcmp(string, "none")) { + current = FSYNC_COMPONENT_NONE; + goto next_name; + } + + if (*string == '-') { + negated = 1; + string++; + len--; + if (!len) + warning(_("invalid value for variable %s"), var); + } + + if (!len) + break; + + for (i = 0; i < ARRAY_SIZE(fsync_component_names); ++i) { + const struct fsync_component_name *n = &fsync_component_names[i]; + + if (strncmp(n->name, string, len)) + continue; + + found = 1; + if (negated) + negative |= n->component_bits; + else + positive |= n->component_bits; + } + + if (!found) { + char *component = xstrndup(string, len); + warning(_("ignoring unknown core.fsync component '%s'"), component); + free(component); + } + +next_name: + string = ep; + } + + return (current & ~negative) | positive; +} + int git_parse_maybe_bool(const char *value) { int v = git_parse_maybe_bool_text(value); @@ -1600,6 +1667,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "core.fsync")) { + if (!value) + return config_error_nonbool(var); + fsync_components = parse_fsync_components(var, value); + return 0; + } + if (!strcmp(var, "core.fsyncmethod")) { if (!value) return config_error_nonbool(var); @@ -1613,6 +1687,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.fsyncobjectfiles")) { + if (fsync_object_files < 0) + warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead")); fsync_object_files = git_config_bool(var, value); return 0; } diff --git a/environment.c b/environment.c index 36ca5fb2e7..698f03a2f4 100644 --- a/environment.c +++ b/environment.c @@ -42,7 +42,7 @@ const char *git_attributes_file; const char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; -int fsync_object_files; +int fsync_object_files = -1; int use_fsync = -1; enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT; enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT; -- cgit v1.3