git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff Hostetler <git@jeffhostetler.com>
To: Josh Steadmon <steadmon@google.com>, git@vger.kernel.org
Subject: Re: [RFC PATCH] trace2: don't overload target directories
Date: Tue, 30 Jul 2019 12:46:41 -0400	[thread overview]
Message-ID: <8612596d-1dfe-1b8e-9e01-c091e43c0556@jeffhostetler.com> (raw)
In-Reply-To: <99e4a0fe409a236d210d95e54cd03fce61daa291.1564438745.git.steadmon@google.com>



On 7/29/2019 6:20 PM, Josh Steadmon wrote:
> trace2 can write files into a target directory. With heavy usage, this
> directory can fill up with files, causing difficulty for
> trace-processing systems.
> 
> When trace2 would write a file to a target directory, first check
> whether or not the directory is overloaded. A directory is overloaded if
> there is a sentinel file declaring an overload, or if the number of
> files exceeds a threshold. If the latter, create a sentinel file to
> speed up later overload checks.

Something about this idea bothers me, but I can't quite put my finger
on it.  You're filling a directory with thousands of files while
(hopefully simultaneously) having a post-processor/aggregator app
read and delete them.  I understand that if the aggregator falls
behind or isn't running, the files will just accumulate and that the
total number of files is the problem.  But I have to wonder if
contention on that directory is going to be a bottleneck and/or
a source of problems.  That is, you'll have one process reading and
deleting and one or more Git processes scanning/counting/creating.
It seems like there might be opportunity for some kinds of races
here.

It have to wonder if it would be better to do some kind of directory
rotation rather than create a marker file.

Alternatively, I think it would be better to not have the marker
file be inside the directory, but rather have a lock file somewhere
to temporarily disable tracing.  Then your stat() call would not need
to effectively search the large directory.  Maybe make this
"<dirname>.lock" as a peer to "<dirname>/", for example.


> 
> The file count threshold is currently set to 1M files, but this can be
> overridden for testing with GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT.
> 
> The assumption is that a separate trace-processing system is dealing
> with the generated traces; once it processes and removes the sentinel
> file, it should be safe to generate new trace files again.
> 
> Potential future work:
> * Write a message into the sentinel file (should match the requested
>    trace2 output format).
> * Make the overload threshold (and the whole overload feature)
>    configurable.

I'm wondering if we should just make this setting another
value in `tr2_sysenv_settings[]` rather than a *_TEST_* env var.

That would give you both env and system/global config support,
since I'm assuming you'd eventually want to have this be in
the user's global config with the other trace2 settings.

All of your tests could be expressed in terms of this new setting
and we wouldn't need this new test env var.

> 
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>   t/t0210-trace2-normal.sh | 15 ++++++++
>   trace2/tr2_dst.c         | 81 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 96 insertions(+)
> 
> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index ce7574edb1..e8a03e9212 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -186,4 +186,19 @@ 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_cmp expected_filenames.txt second_ls_output.txt
> +'
> +
>   test_done
> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index 5dda0ca1cd..3286297918 100644
> --- a/trace2/tr2_dst.c
> +++ b/trace2/tr2_dst.c
> @@ -1,3 +1,5 @@
> +#include <dirent.h>
> +
>   #include "cache.h"
>   #include "trace2/tr2_dst.h"
>   #include "trace2/tr2_sid.h"
> @@ -8,6 +10,18 @@
>    */
>   #define MAX_AUTO_ATTEMPTS 10
>   
> +/*
> + * Sentinel file used to detect when we're overloading a directory with too many
> + * trace files.
> + */
> +#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
> + */
> +#define OVERLOAD_FILE_COUNT 1000000
> +
>   static int tr2_dst_want_warning(void)
>   {
>   	static int tr2env_dst_debug = -1;
> @@ -32,6 +46,63 @@ void tr2_dst_trace_disable(struct tr2_dst *dst)
>   	dst->need_close = 0;
>   }
>   
> +/*
> + * 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.
> + *
> + * We expect that some trace processing system is gradually collecting files
> + * from the target directory; after it removes the sentinel file we'll start
> + * writing traces again.
> + */
> +static int tr2_dst_overloaded(const char *tgt_prefix)
> +{
> +	int file_count = 0, overload_file_count = 0;
> +	char *test_threshold_val;
> +	DIR *dirp;
> +	struct strbuf path = STRBUF_INIT, sentinel_path = STRBUF_INIT;
> +	struct stat statbuf;
> +
> +	strbuf_addstr(&path, tgt_prefix);
> +	if (!is_dir_sep(path.buf[path.len - 1])) {
> +		strbuf_addch(&path, '/');
> +	}
> +
> +	/* check sentinel */
> +	strbuf_addstr(&sentinel_path, path.buf);
> +	strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
> +	if (!stat(sentinel_path.buf, &statbuf)) {
> +		strbuf_release(&path);

Also release sentinel_path ?
(And in both of the return statements below.)

> +		return 1;
> +	}
> +
> +	/* 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))
> +		file_count++;
> +	if (dirp)
> +		closedir(dirp);
> +
> +	if (file_count >= overload_file_count) {
> +		creat(sentinel_path.buf, S_IRUSR | S_IWUSR);
> +		/* TODO: Write a target-specific message? */
> +		strbuf_release(&path);
> +		return 1;
> +	}
> +
> +	strbuf_release(&path);
> +	return 0;
> +}
> +
>   static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
>   {
>   	int fd;
> @@ -50,6 +121,16 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
>   	strbuf_addstr(&path, sid);
>   	base_path_len = path.len;
>   
> +	if (tr2_dst_overloaded(tgt_prefix)) {
> +		strbuf_release(&path);
> +		if (tr2_dst_want_warning())
> +			warning("trace2: not opening %s trace file due to too "
> +				"many files in target directory %s",
> +				tr2_sysenv_display_name(dst->sysenv_var),
> +				tgt_prefix);
> +		return 0;
> +	}
> +
>   	for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
>   		if (attempt_count > 0) {
>   			strbuf_setlen(&path, base_path_len);
> 

hope this helps,
Jeff


  parent reply	other threads:[~2019-07-30 16:46 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-29 22:20 Josh Steadmon
2019-07-30 13:29 ` Derrick Stolee
2019-07-30 21:52   ` Josh Steadmon
2019-07-30 16:46 ` Jeff Hostetler [this message]
2019-07-30 22:01   ` Josh Steadmon
2019-07-30 22:02   ` Josh Steadmon
2019-07-30 18:00 ` Jeff Hostetler
2019-07-30 22:08   ` Josh Steadmon
2019-08-02 22:02 ` [RFC PATCH v2 0/2] " Josh Steadmon
2019-08-02 22:02   ` [RFC PATCH v2 1/2] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-08-02 22:02   ` [RFC PATCH v2 2/2] trace2: don't overload target directories Josh Steadmon
2019-08-05 15:34     ` Jeff Hostetler
2019-08-05 18:17       ` Josh Steadmon
2019-08-05 18:01     ` SZEDER Gábor
2019-08-05 18:09       ` Josh Steadmon
2019-09-14  0:25 ` [RFC PATCH v3 0/3] " Josh Steadmon
2019-09-14  0:25   ` [RFC PATCH v3 1/3] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-09-14  0:25   ` [RFC PATCH v3 2/3] trace2: don't overload target directories Josh Steadmon
2019-09-14  0:26   ` [RFC PATCH v3 3/3] trace2: write overload message to sentinel files Josh Steadmon
2019-09-16 12:07     ` Derrick Stolee
2019-09-16 14:11       ` Jeff Hostetler
2019-09-16 18:20         ` Josh Steadmon
2019-09-19 18:23           ` Jeff Hostetler
2019-09-19 22:47             ` Josh Steadmon
2019-09-20 15:59               ` Jeff Hostetler
2019-09-16 18:07       ` Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 0/4] trace2: don't overload target directories Josh Steadmon
2019-10-03 23:32   ` [PATCH v4 1/4] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-10-03 23:32   ` [PATCH v4 2/4] docs: clarify trace2 version invariants Josh Steadmon
2019-10-03 23:32   ` [PATCH v4 3/4] trace2: don't overload target directories Josh Steadmon
2019-10-04  0:25     ` Junio C Hamano
2019-10-04 21:57       ` Josh Steadmon
2019-10-04  9:12     ` Johannes Schindelin
2019-10-04 22:05       ` Josh Steadmon
2019-10-03 23:32   ` [PATCH v4 4/4] trace2: write overload message to sentinel files Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 0/4] trace2: discard new traces if the target directory contains too many files Josh Steadmon
2019-10-04 22:08   ` [PATCH v5 1/4] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-10-04 22:08   ` [PATCH v5 2/4] docs: clarify trace2 version invariants Josh Steadmon
2019-10-04 22:08   ` [PATCH v5 3/4] trace2: discard new traces if target directory has too many files Josh Steadmon
2019-10-04 22:08   ` [PATCH v5 4/4] trace2: write discard message to sentinel files Josh Steadmon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8612596d-1dfe-1b8e-9e01-c091e43c0556@jeffhostetler.com \
    --to=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=steadmon@google.com \
    --subject='Re: [RFC PATCH] trace2: don'\''t overload target directories' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git