diff mbox series

[11/23] fsmonitor--daemon: define token-ids

Message ID 304fe03034f8622aa8728d8872cc9bc539bab861.1617291666.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Builtin FSMonitor Feature | expand

Commit Message

Jeff Hostetler April 1, 2021, 3:40 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Teach fsmonitor--daemon to create token-ids and define the
overall token naming scheme.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fsmonitor--daemon.c | 108 +++++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

Comments

Derrick Stolee April 26, 2021, 7:49 p.m. UTC | #1 
On 4/1/2021 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Teach fsmonitor--daemon to create token-ids and define the
> overall token naming scheme.
...
> +/*
> + * Requests to and from a FSMonitor Protocol V2 provider use an opaque
> + * "token" as a virtual timestamp.  Clients can request a summary of all
> + * created/deleted/modified files relative to a token.  In the response,
> + * clients receive a new token for the next (relative) request.
> + *
> + *
> + * Token Format
> + * ============
> + *
> + * The contents of the token are private and provider-specific.
> + *
> + * For the built-in fsmonitor--daemon, we define a token as follows:
> + *
> + *     "builtin" ":" <token_id> ":" <sequence_nr>
> + *
> + * The <token_id> is an arbitrary OPAQUE string, such as a GUID,
> + * UUID, or {timestamp,pid}.  It is used to group all filesystem
> + * events that happened while the daemon was monitoring (and in-sync
> + * with the filesystem).
> + *
> + *     Unlike FSMonitor Protocol V1, it is not defined as a timestamp
> + *     and does not define less-than/greater-than relationships.
> + *     (There are too many race conditions to rely on file system
> + *     event timestamps.)
> + *
> + * The <sequence_nr> is a simple integer incremented for each event
> + * received.  When a new <token_id> is created, the <sequence_nr> is
> + * reset to zero.
> + *
> + *
> + * About Token Ids
> + * ===============
> + *
> + * A new token_id is created:
> + *
> + * [1] each time the daemon is started.
> + *
> + * [2] any time that the daemon must re-sync with the filesystem
> + *     (such as when the kernel drops or we miss events on a very
> + *     active volume).
> + *
> + * [3] in response to a client "flush" command (for dropped event
> + *     testing).
> + *
> + * [4] MAYBE We might want to change the token_id after very complex
> + *     filesystem operations are performed, such as a directory move
> + *     sequence that affects many files within.  It might be simpler
> + *     to just give up and fake a re-sync (and let the client do a
> + *     full scan) than try to enumerate the effects of such a change.
> + *
> + * When a new token_id is created, the daemon is free to discard all
> + * cached filesystem events associated with any previous token_ids.
> + * Events associated with a non-current token_id will never be sent
> + * to a client.  A token_id change implicitly means that the daemon
> + * has gap in its event history.
> + *
> + * Therefore, clients that present a token with a stale (non-current)
> + * token_id will always be given a trivial response.

From this comment, it seems to be the case that concurrent Git
commands will race to advance the FS Monitor token and one of them
will lose, causing a full working directory scan. There is no list
of "recent" tokens.

I could see this changing in the future, but for now it is a
reasonable simplification.

> + */
> +struct fsmonitor_token_data {
> +	struct strbuf token_id;
> +	struct fsmonitor_batch *batch_head;
> +	struct fsmonitor_batch *batch_tail;
> +	uint64_t client_ref_count;
> +};
> +
> +static struct fsmonitor_token_data *fsmonitor_new_token_data(void)
> +{
> +	static int test_env_value = -1;
> +	static uint64_t flush_count = 0;
> +	struct fsmonitor_token_data *token;
> +
> +	token = (struct fsmonitor_token_data *)xcalloc(1, sizeof(*token));

I think the best practice here is "CALLOC_ARRAY(token, 1);"

> +
> +	strbuf_init(&token->token_id, 0);

This is likely overkill since you used calloc() above.

> +	token->batch_head = NULL;
> +	token->batch_tail = NULL;
> +	token->client_ref_count = 0;
> +
> +	if (test_env_value < 0)
> +		test_env_value = git_env_bool("GIT_TEST_FSMONITOR_TOKEN", 0);
> +
> +	if (!test_env_value) {
> +		struct timeval tv;
> +		struct tm tm;
> +		time_t secs;
> +
> +		gettimeofday(&tv, NULL);
> +		secs = tv.tv_sec;
> +		gmtime_r(&secs, &tm);
> +
> +		strbuf_addf(&token->token_id,
> +			    "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",
> +			    flush_count++,
> +			    getpid(),
> +			    tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> +			    tm.tm_hour, tm.tm_min, tm.tm_sec,
> +			    (long)tv.tv_usec);

Between the PID, the flush count, and how deep you go in the
timestamp, this seems to be specific enough.

> +	} else {
> +		strbuf_addf(&token->token_id, "test_%08x", test_env_value++);

And this will be nice for testing.

> +	}
> +
> +	return token;
> +}
> +
>  static ipc_server_application_cb handle_client;
>  
>  static int handle_client(void *data, const char *command,
> @@ -330,7 +436,7 @@ static int fsmonitor_run_daemon(void)
>  
>  	pthread_mutex_init(&state.main_lock, NULL);
>  	state.error_code = 0;
> -	state.current_token_data = NULL;
> +	state.current_token_data = fsmonitor_new_token_data();
>  	state.test_client_delay_ms = 0;
>  
>  	/* Prepare to (recursively) watch the <worktree-root> directory. */
> 

Thanks,
-Stolee
Eric Sunshine April 26, 2021, 8:01 p.m. UTC | #2 
On Mon, Apr 26, 2021 at 3:49 PM Derrick Stolee <stolee@gmail.com> wrote:
> On 4/1/2021 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
> > +     token = (struct fsmonitor_token_data *)xcalloc(1, sizeof(*token));
>
> I think the best practice here is "CALLOC_ARRAY(token, 1);"
>
> > +
> > +     strbuf_init(&token->token_id, 0);
>
> This is likely overkill since you used calloc() above.

Not quite. A strbuf must be initialized either with STRBUF_INIT or
strbuf_init() in order to make strbuf.buf point at strbuf_slopbuf.
Derrick Stolee April 26, 2021, 8:03 p.m. UTC | #3 
On 4/26/2021 4:01 PM, Eric Sunshine wrote:
> On Mon, Apr 26, 2021 at 3:49 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 4/1/2021 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
>>> +     token = (struct fsmonitor_token_data *)xcalloc(1, sizeof(*token));
>>
>> I think the best practice here is "CALLOC_ARRAY(token, 1);"
>>
>>> +
>>> +     strbuf_init(&token->token_id, 0);
>>
>> This is likely overkill since you used calloc() above.
> 
> Not quite. A strbuf must be initialized either with STRBUF_INIT or
> strbuf_init() in order to make strbuf.buf point at strbuf_slopbuf.

Thanks! I didn't know that detail, but it makes a lot of sense.

-Stolee
Jeff Hostetler April 30, 2021, 4:17 p.m. UTC | #4 
On 4/26/21 3:49 PM, Derrick Stolee wrote:
> On 4/1/2021 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach fsmonitor--daemon to create token-ids and define the
>> overall token naming scheme.
> ...
>> +/*
>> + * Requests to and from a FSMonitor Protocol V2 provider use an opaque
>> + * "token" as a virtual timestamp.  Clients can request a summary of all
>> + * created/deleted/modified files relative to a token.  In the response,
>> + * clients receive a new token for the next (relative) request.
>> + *
>> + *
>> + * Token Format
>> + * ============
>> + *
>> + * The contents of the token are private and provider-specific.
>> + *
>> + * For the built-in fsmonitor--daemon, we define a token as follows:
>> + *
>> + *     "builtin" ":" <token_id> ":" <sequence_nr>
>> + *
>> + * The <token_id> is an arbitrary OPAQUE string, such as a GUID,
>> + * UUID, or {timestamp,pid}.  It is used to group all filesystem
>> + * events that happened while the daemon was monitoring (and in-sync
>> + * with the filesystem).
>> + *
>> + *     Unlike FSMonitor Protocol V1, it is not defined as a timestamp
>> + *     and does not define less-than/greater-than relationships.
>> + *     (There are too many race conditions to rely on file system
>> + *     event timestamps.)
>> + *
>> + * The <sequence_nr> is a simple integer incremented for each event
>> + * received.  When a new <token_id> is created, the <sequence_nr> is
>> + * reset to zero.
>> + *
>> + *
>> + * About Token Ids
>> + * ===============
>> + *
>> + * A new token_id is created:
>> + *
>> + * [1] each time the daemon is started.
>> + *
>> + * [2] any time that the daemon must re-sync with the filesystem
>> + *     (such as when the kernel drops or we miss events on a very
>> + *     active volume).
>> + *
>> + * [3] in response to a client "flush" command (for dropped event
>> + *     testing).
>> + *
>> + * [4] MAYBE We might want to change the token_id after very complex
>> + *     filesystem operations are performed, such as a directory move
>> + *     sequence that affects many files within.  It might be simpler
>> + *     to just give up and fake a re-sync (and let the client do a
>> + *     full scan) than try to enumerate the effects of such a change.
>> + *
>> + * When a new token_id is created, the daemon is free to discard all
>> + * cached filesystem events associated with any previous token_ids.
>> + * Events associated with a non-current token_id will never be sent
>> + * to a client.  A token_id change implicitly means that the daemon
>> + * has gap in its event history.
>> + *
>> + * Therefore, clients that present a token with a stale (non-current)
>> + * token_id will always be given a trivial response.
> 
>  From this comment, it seems to be the case that concurrent Git
> commands will race to advance the FS Monitor token and one of them
> will lose, causing a full working directory scan. There is no list
> of "recent" tokens.
> 
> I could see this changing in the future, but for now it is a
> reasonable simplification.

The daemon only creates a new token-id when it needs to because of
a loss of sync with the FS.  And the sequence-nr is advanced based
upon the quantity of FS activity.  Clients don't cause either to
change or advance (except for the flush, which is a testing hack).

Ideally, the token-id is created when the daemon starts up and is
never changed.

Concurrent clients all receive normalized event data from the
in-memory cache/queue from threads reading the queue in parallel.


I included [4] as a possible future enhancement, but so far haven't
actually needed it.  The event stream (at least on Windows and MacOS)
from the OS is sufficient that I didn't need to implement that.

I'll remove [4] from the comments.

Thanks,
Jeff
diff mbox series

Patch

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 16252487240a..2d25e36601fe 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -149,6 +149,112 @@  static int do_as_client__send_flush(void)
 	return 0;
 }
 
+/*
+ * Requests to and from a FSMonitor Protocol V2 provider use an opaque
+ * "token" as a virtual timestamp.  Clients can request a summary of all
+ * created/deleted/modified files relative to a token.  In the response,
+ * clients receive a new token for the next (relative) request.
+ *
+ *
+ * Token Format
+ * ============
+ *
+ * The contents of the token are private and provider-specific.
+ *
+ * For the built-in fsmonitor--daemon, we define a token as follows:
+ *
+ *     "builtin" ":" <token_id> ":" <sequence_nr>
+ *
+ * The <token_id> is an arbitrary OPAQUE string, such as a GUID,
+ * UUID, or {timestamp,pid}.  It is used to group all filesystem
+ * events that happened while the daemon was monitoring (and in-sync
+ * with the filesystem).
+ *
+ *     Unlike FSMonitor Protocol V1, it is not defined as a timestamp
+ *     and does not define less-than/greater-than relationships.
+ *     (There are too many race conditions to rely on file system
+ *     event timestamps.)
+ *
+ * The <sequence_nr> is a simple integer incremented for each event
+ * received.  When a new <token_id> is created, the <sequence_nr> is
+ * reset to zero.
+ *
+ *
+ * About Token Ids
+ * ===============
+ *
+ * A new token_id is created:
+ *
+ * [1] each time the daemon is started.
+ *
+ * [2] any time that the daemon must re-sync with the filesystem
+ *     (such as when the kernel drops or we miss events on a very
+ *     active volume).
+ *
+ * [3] in response to a client "flush" command (for dropped event
+ *     testing).
+ *
+ * [4] MAYBE We might want to change the token_id after very complex
+ *     filesystem operations are performed, such as a directory move
+ *     sequence that affects many files within.  It might be simpler
+ *     to just give up and fake a re-sync (and let the client do a
+ *     full scan) than try to enumerate the effects of such a change.
+ *
+ * When a new token_id is created, the daemon is free to discard all
+ * cached filesystem events associated with any previous token_ids.
+ * Events associated with a non-current token_id will never be sent
+ * to a client.  A token_id change implicitly means that the daemon
+ * has gap in its event history.
+ *
+ * Therefore, clients that present a token with a stale (non-current)
+ * token_id will always be given a trivial response.
+ */
+struct fsmonitor_token_data {
+	struct strbuf token_id;
+	struct fsmonitor_batch *batch_head;
+	struct fsmonitor_batch *batch_tail;
+	uint64_t client_ref_count;
+};
+
+static struct fsmonitor_token_data *fsmonitor_new_token_data(void)
+{
+	static int test_env_value = -1;
+	static uint64_t flush_count = 0;
+	struct fsmonitor_token_data *token;
+
+	token = (struct fsmonitor_token_data *)xcalloc(1, sizeof(*token));
+
+	strbuf_init(&token->token_id, 0);
+	token->batch_head = NULL;
+	token->batch_tail = NULL;
+	token->client_ref_count = 0;
+
+	if (test_env_value < 0)
+		test_env_value = git_env_bool("GIT_TEST_FSMONITOR_TOKEN", 0);
+
+	if (!test_env_value) {
+		struct timeval tv;
+		struct tm tm;
+		time_t secs;
+
+		gettimeofday(&tv, NULL);
+		secs = tv.tv_sec;
+		gmtime_r(&secs, &tm);
+
+		strbuf_addf(&token->token_id,
+			    "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",
+			    flush_count++,
+			    getpid(),
+			    tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			    tm.tm_hour, tm.tm_min, tm.tm_sec,
+			    (long)tv.tv_usec);
+	} else {
+		strbuf_addf(&token->token_id, "test_%08x", test_env_value++);
+	}
+
+	return token;
+}
+
 static ipc_server_application_cb handle_client;
 
 static int handle_client(void *data, const char *command,
@@ -330,7 +436,7 @@  static int fsmonitor_run_daemon(void)
 
 	pthread_mutex_init(&state.main_lock, NULL);
 	state.error_code = 0;
-	state.current_token_data = NULL;
+	state.current_token_data = fsmonitor_new_token_data();
 	state.test_client_delay_ms = 0;
 
 	/* Prepare to (recursively) watch the <worktree-root> directory. */