[RFC,v2,0/2] trace2: don't overload target directories

Message

Josh Steadmon Aug. 2, 2019, 10:02 p.m. UTC

I'm sending out V2 still as an RFC because I haven't yet had time to
check that directory contention doesn't create problems with multiple
processes sharing the same target directory. I'll be on vacation for the
next couple of weeks, so I wanted to get the new config variable version
out before then.

I noticed that git-config doesn't currently mention target directories,
so I've included a patch to fix that as well.

Changes since V1:
* Adds a new patch that includes a description of trace2's
  target-directory mode in the git-config documentation.
* Moves the file count threshold from a #define constant to a config
  option.
* Renames the threshold override envvar to be consistent with other
  trace2 envvars.
* Simplified the new test case in t0210.

Josh Steadmon (2):
  docs: mention trace2 target-dir mode in git-config
  trace2: don't overload target directories

 Documentation/config/trace2.txt        |  6 ++
 Documentation/technical/api-trace2.txt |  7 +--
 Documentation/trace2-target-values.txt |  4 +-
 t/t0210-trace2-normal.sh               | 19 ++++++
 trace2/tr2_dst.c                       | 86 ++++++++++++++++++++++++++
 trace2/tr2_sysenv.c                    |  3 +
 trace2/tr2_sysenv.h                    |  2 +
 7 files changed, 122 insertions(+), 5 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  65e05a3db5 docs: mention trace2 target-dir mode in git-config
1:  99e4a0fe40 ! 2:  a779e272df trace2: don't overload target directories
    @@ Commit message
     
     
    + ## Documentation/config/trace2.txt ##
    +@@ Documentation/config/trace2.txt: trace2.destinationDebug::
    + 	By default, these errors are suppressed and tracing is
    + 	silently disabled.  May be overridden by the
    + 	`GIT_TRACE2_DST_DEBUG` environment variable.
    ++
    ++trace2.maxFiles::
    ++	Integer.  When writing trace files to a target directory, do not
    ++	write additional traces if we would exceed this many files. Instead,
    ++	write a sentinel file that will block further tracing to this
    ++	directory. Defaults to 0, which disables this check.
    +
      ## t/t0210-trace2-normal.sh ##
     @@ t/t0210-trace2-normal.sh: test_expect_success 'using global config with include' '
      	test_cmp expect actual
      '
      
     +test_expect_success "don't overload target directory" '
    -+	GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT=100 &&
    -+	export GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT &&
    -+	test_when_finished "rm -r trace_target_dir" &&
     +	mkdir trace_target_dir &&
    -+	test_seq $GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT | sed "s#^#trace_target_dir/#" | sort > expected_filenames.txt &&
    -+	xargs touch < expected_filenames.txt &&
    -+	ls trace_target_dir | sed "s#^#trace_target_dir/#" > first_ls_output.txt &&
    -+	test_cmp expected_filenames.txt first_ls_output.txt &&
    -+	GIT_TRACE2="$(pwd)/trace_target_dir" test-tool trace2 001return 0 &&
    -+	echo "trace_target_dir/git-trace2-overload" >> expected_filenames.txt &&
    -+	ls trace_target_dir | sed "s#^#trace_target_dir/#" > second_ls_output.txt &&
    ++	test_when_finished "rm -r trace_target_dir" &&
    ++	(
    ++		GIT_TRACE2_MAX_FILES=5 &&
    ++		export GIT_TRACE2_MAX_FILES &&
    ++		cd trace_target_dir &&
    ++		test_seq $GIT_TRACE2_MAX_FILES >../expected_filenames.txt &&
    ++		xargs touch <../expected_filenames.txt &&
    ++		cd .. &&
    ++		ls trace_target_dir >first_ls_output.txt &&
    ++		test_cmp expected_filenames.txt first_ls_output.txt &&
    ++		GIT_TRACE2="$(pwd)/trace_target_dir" test-tool trace2 001return 0
    ++	) &&
    ++	echo git-trace2-overload >>expected_filenames.txt &&
    ++	ls trace_target_dir >second_ls_output.txt &&
     +	test_cmp expected_filenames.txt second_ls_output.txt
     +'
     +
    @@ trace2/tr2_dst.c
     +#define OVERLOAD_SENTINEL_NAME "git-trace2-overload"
     +
     +/*
    -+ * How many files we can write to a directory before entering overload mode.
    -+ * This can be overridden with the envvar GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT
    ++ * When set to zero, disables directory overload checking. Otherwise, controls
    ++ * how many files we can write to a directory before entering overload mode.
    ++ * This can be overridden via the TR2_SYSENV_MAX_FILES setting.
     + */
    -+#define OVERLOAD_FILE_COUNT 1000000
    ++static int tr2env_max_files = 0;
     +
      static int tr2_dst_want_warning(void)
      {
    @@ trace2/tr2_dst.c: void tr2_dst_trace_disable(struct tr2_dst *dst)
      
     +/*
     + * Check to make sure we're not overloading the target directory with too many
    -+ * files. First check for the presence of a sentinel file, then check file
    -+ * count. If we are overloaded, create the sentinel file if it doesn't already
    -+ * exist.
    ++ * files. First get the threshold (if present) from the config or envvar. If
    ++ * it's zero or unset, disable this check.  Next check for the presence of a
    ++ * sentinel file, then check file count. If we are overloaded, create the
    ++ * sentinel file if it doesn't already exist.
     + *
     + * We expect that some trace processing system is gradually collecting files
     + * from the target directory; after it removes the sentinel file we'll start
    @@ trace2/tr2_dst.c: void tr2_dst_trace_disable(struct tr2_dst *dst)
     + */
     +static int tr2_dst_overloaded(const char *tgt_prefix)
     +{
    -+	int file_count = 0, overload_file_count = 0;
    -+	char *test_threshold_val;
    ++	int file_count = 0, max_files = 0, ret = 0;
    ++	const char *max_files_var;
     +	DIR *dirp;
     +	struct strbuf path = STRBUF_INIT, sentinel_path = STRBUF_INIT;
     +	struct stat statbuf;
    @@ trace2/tr2_dst.c: void tr2_dst_trace_disable(struct tr2_dst *dst)
     +		strbuf_addch(&path, '/');
     +	}
     +
    ++	/* Get the config or envvar and decide if we should continue this check */
    ++	max_files_var = tr2_sysenv_get(TR2_SYSENV_MAX_FILES);
    ++	if (max_files_var && *max_files_var && ((max_files = atoi(max_files_var)) >= 0))
    ++		tr2env_max_files = max_files;
    ++
    ++	if (!tr2env_max_files) {
    ++		ret = 0;
    ++		goto cleanup;
    ++	}
    ++
     +	/* check sentinel */
     +	strbuf_addstr(&sentinel_path, path.buf);
     +	strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
     +	if (!stat(sentinel_path.buf, &statbuf)) {
    -+		strbuf_release(&path);
    -+		return 1;
    ++		ret = 1;
    ++		goto cleanup;
     +	}
     +
    -+	/* check if we're overriding the threshold (e.g., for testing) */
    -+	test_threshold_val = getenv("GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT");
    -+	if (test_threshold_val)
    -+		overload_file_count = atoi(test_threshold_val);
    -+	if (overload_file_count <= 0)
    -+		overload_file_count = OVERLOAD_FILE_COUNT;
    -+
    -+
     +	/* check file count */
     +	dirp = opendir(path.buf);
    -+	while (file_count < overload_file_count && dirp && readdir(dirp))
    ++	while (file_count < tr2env_max_files && dirp && readdir(dirp))
     +		file_count++;
     +	if (dirp)
     +		closedir(dirp);
     +
    -+	if (file_count >= overload_file_count) {
    ++	if (file_count >= tr2env_max_files) {
     +		creat(sentinel_path.buf, S_IRUSR | S_IWUSR);
    -+		/* TODO: Write a target-specific message? */
    -+		strbuf_release(&path);
    -+		return 1;
    ++		ret = 1;
    ++		goto cleanup;
     +	}
     +
    ++cleanup:
     +	strbuf_release(&path);
    -+	return 0;
    ++	strbuf_release(&sentinel_path);
    ++	return ret;
     +}
     +
      static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
    @@ trace2/tr2_dst.c: static int tr2_dst_try_auto_path(struct tr2_dst *dst, const ch
      	for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
      		if (attempt_count > 0) {
      			strbuf_setlen(&path, base_path_len);
    +
    + ## trace2/tr2_sysenv.c ##
    +@@ trace2/tr2_sysenv.c: static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
    + 				       "trace2.perftarget" },
    + 	[TR2_SYSENV_PERF_BRIEF]    = { "GIT_TRACE2_PERF_BRIEF",
    + 				       "trace2.perfbrief" },
    ++
    ++	[TR2_SYSENV_MAX_FILES]     = { "GIT_TRACE2_MAX_FILES",
    ++				       "trace2.maxfiles" },
    + };
    + /* clang-format on */
    + 
    +
    + ## trace2/tr2_sysenv.h ##
    +@@ trace2/tr2_sysenv.h: enum tr2_sysenv_variable {
    + 	TR2_SYSENV_PERF,
    + 	TR2_SYSENV_PERF_BRIEF,
    + 
    ++	TR2_SYSENV_MAX_FILES,
    ++
    + 	TR2_SYSENV_MUST_BE_LAST
    + };
    + 
--
2.22.0.770.g0f2c4a37fd-goog