Re: [PATCH v2 2/3] core.fsync: introduce granular fsync control

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 11137 bytes --]

On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wrote:
> From: Neeraj Singh <neerajsi@microsoft.com>
[snip]
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index c23d01de7dc..c32534c13b4 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1286,7 +1286,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,
> @@ -1508,7 +1508,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 857be7826f3..916c55d6ce9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1204,11 +1204,13 @@ static void write_pack_file(void)
>  		 * If so, rewrite it like in fast-import
>  		 */
>  		if (pack_to_stdout) {
> -			finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
> +			finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
> +					  CSUM_HASH_IN_STREAM | CSUM_CLOSE);

It doesn't have any effect here given that we don't sync at all when
writing to stdout, but I wonder whether we should set up the component
correctly regardless of that such that it makes for a less confusing
read.

[snip]
> diff --git a/config.c b/config.c
> index c3410b8a868..29c867aab03 100644
> --- a/config.c
> +++ b/config.c
> @@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *value)
>  	return -1;
>  }
>  
> +static const struct fsync_component_entry {
> +	const char *name;
> +	enum fsync_component component_bits;
> +} fsync_component_table[] = {
> +	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
> +	{ "pack", FSYNC_COMPONENT_PACK },
> +	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
> +	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
> +	{ "objects", FSYNC_COMPONENTS_OBJECTS },
> +	{ "default", FSYNC_COMPONENTS_DEFAULT },
> +	{ "all", FSYNC_COMPONENTS_ALL },
> +};
> +
> +static enum fsync_component parse_fsync_components(const char *var, const char *string)
> +{
> +	enum fsync_component output = 0;
> +
> +	if (!strcmp(string, "none"))
> +		return output;
> +
> +	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 (*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_table); ++i) {
> +			const struct fsync_component_entry *entry = &fsync_component_table[i];
> +
> +			if (strncmp(entry->name, string, len))
> +				continue;
> +
> +			found = 1;
> +			if (negated)
> +				output &= ~entry->component_bits;
> +			else
> +				output |= entry->component_bits;
> +		}
> +
> +		if (!found) {
> +			char *component = xstrndup(string, len);
> +			warning(_("unknown %s value '%s'"), var, component);
> +			free(component);
> +		}
> +
> +		string = ep;
> +	}
> +
> +	return output;
> +}
> +
>  int git_parse_maybe_bool(const char *value)
>  {
>  	int v = git_parse_maybe_bool_text(value);
> @@ -1490,6 +1557,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);
> @@ -1503,7 +1577,7 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	if (!strcmp(var, "core.fsyncobjectfiles")) {
> -		fsync_object_files = git_config_bool(var, value);
> +		warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
>  		return 0;
>  	}

Shouldn't we continue to support this for now such that users can
migrate from the old, deprecated value first before we start to ignore
it?

Patrick

> diff --git a/csum-file.c b/csum-file.c
> index 26e8a6df44e..59ef3398ca2 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 291215b34eb..0d29f528fbc 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 f9140e842cf..09905adecf9 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -42,6 +42,7 @@ const char *git_hooks_path;
>  int zlib_compression_level = Z_BEST_SPEED;
>  int pack_compression_level = Z_DEFAULT_COMPRESSION;
>  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 837b46b2af5..882f91f7d57 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1406,7 +1406,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 | MIDX_WRITE_BITMAP))
> diff --git a/object-file.c b/object-file.c
> index eb972cdccd2..9d9c4a39e85 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1809,8 +1809,7 @@ 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 (fsync_object_files)
> -		fsync_or_die(fd, "loose object file");
> +	fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd, "loose object file");
>  	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 9c55c1531e1..c16e43d1669 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 a5846f3a346..51812cb1299 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 f3986596623..f3539681f49 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3060,7 +3060,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;
> -- 
> gitgitgadget
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]