git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Josh Steadmon <steadmon@google.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, git@jeffhostetler.com, stolee@gmail.com
Subject: Re: [PATCH v4 3/4] trace2: don't overload target directories
Date: Fri, 4 Oct 2019 15:05:48 -0700	[thread overview]
Message-ID: <20191004220548.GC224668@google.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1910041105290.46@tvgsbejvaqbjf.bet>

On 2019.10.04 11:12, Johannes Schindelin wrote:
> Hi Josh,
> 
> On Thu, 3 Oct 2019, Josh Steadmon wrote:
> 
> > [...]
> > diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> > index 5dda0ca1cd..af3405f179 100644
> > --- a/trace2/tr2_dst.c
> > +++ b/trace2/tr2_dst.c
> > @@ -1,3 +1,5 @@
> > +#include <dirent.h>
> > +
> 
> This completely breaks the Windows build:
> 
> 	In file included from trace2/tr2_dst.c:1:
> 	compat/win32/dirent.h:13:14: error: 'MAX_PATH' undeclared here (not in a
> 	function)
> 	   13 |  char d_name[MAX_PATH * 3]; /* file name (* 3 for UTF-8 conversion) */
> 	      |              ^~~~~~~~
> 
> See the full build log in all its glory here:
> 
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=17409&view=logs&jobId=fd490c07-0b22-5182-fac9-6d67fe1e939b&taskId=ce91d5d6-0c55-50f5-8ab9-6695c03ab102&lineStart=252&lineEnd=255&colStart=1&colEnd=30
> 
> The fix is as easy as probably surprising: simply delete that
> `#include`. It is redundant anyway:
> https://github.com/git/git/blob/v2.23.0/git-compat-util.h#L184

Sorry about that. Fixed in V5, which will be out shortly. Is there a way
to trigger the Azure pipeline apart from using GitGitGadget?

> Deleting that `#include` line from your patch not only lets the file
> compile cleanly on Windows, it also conforms to our coding style where
> `cache.h` or `git-compat-util.h` need to be the first `#include`. That
> rule's purpose is precisely to prevent platform-specific compile errors
> like the one shown above.

Hmm, I wonder if Coccinelle could catch more CodingGuideline mistakes
such as this? Although it seems there are a few exceptions to this rule,
so maybe it's not a good fit in this particular case.

> Ciao,
> Johannes
> 
> >  #include "cache.h"
> >  #include "trace2/tr2_dst.h"
> >  #include "trace2/tr2_sid.h"
> > @@ -8,6 +10,19 @@
> >   */
> >  #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"
> > +
> > +/*
> > + * 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.
> > + */
> > +static int tr2env_max_files = 0;
> > +
> >  static int tr2_dst_want_warning(void)
> >  {
> >  	static int tr2env_dst_debug = -1;
> > @@ -32,6 +47,67 @@ 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 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
> > + * writing traces again.
> > + */
> > +static int tr2_dst_overloaded(const char *tgt_prefix)
> > +{
> > +	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;
> > +
> > +	/* 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;
> > +	}
> > +
> > +	strbuf_addstr(&path, tgt_prefix);
> > +	if (!is_dir_sep(path.buf[path.len - 1])) {
> > +		strbuf_addch(&path, '/');
> > +	}
> > +
> > +	/* check sentinel */
> > +	strbuf_addbuf(&sentinel_path, &path);
> > +	strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
> > +	if (!stat(sentinel_path.buf, &statbuf)) {
> > +		ret = 1;
> > +		goto cleanup;
> > +	}
> > +
> > +	/* check file count */
> > +	dirp = opendir(path.buf);
> > +	while (file_count < tr2env_max_files && dirp && readdir(dirp))
> > +		file_count++;
> > +	if (dirp)
> > +		closedir(dirp);
> > +
> > +	if (file_count >= tr2env_max_files) {
> > +		creat(sentinel_path.buf, 0666);
> > +		ret = 1;
> > +		goto cleanup;
> > +	}
> > +
> > +cleanup:
> > +	strbuf_release(&path);
> > +	strbuf_release(&sentinel_path);
> > +	return ret;
> > +}
> > +
> >  static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> >  {
> >  	int fd;
> > @@ -50,6 +126,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);
> > diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
> > index 5958cfc424..3c3792eca2 100644
> > --- a/trace2/tr2_sysenv.c
> > +++ b/trace2/tr2_sysenv.c
> > @@ -49,6 +49,9 @@ 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 */
> >
> > diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h
> > index 8dd82a7a56..d4364a7b85 100644
> > --- a/trace2/tr2_sysenv.h
> > +++ b/trace2/tr2_sysenv.h
> > @@ -24,6 +24,8 @@ enum tr2_sysenv_variable {
> >  	TR2_SYSENV_PERF,
> >  	TR2_SYSENV_PERF_BRIEF,
> >
> > +	TR2_SYSENV_MAX_FILES,
> > +
> >  	TR2_SYSENV_MUST_BE_LAST
> >  };
> >
> > --
> > 2.23.0.581.g78d2f28ef7-goog
> >
> >

  reply	other threads:[~2019-10-04 22:05 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-29 22:20 [RFC PATCH] " Josh Steadmon
2019-07-30 13:29 ` Derrick Stolee
2019-07-30 21:52   ` Josh Steadmon
2019-07-30 16:46 ` Jeff Hostetler
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 [this message]
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=20191004220548.GC224668@google.com \
    --to=steadmon@google.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v4 3/4] 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