@matheusbsbs / matheus-bagliano / commits / b05880d357

fsmonitor--daemon: use a cookie file to sync with file system

Teach fsmonitor--daemon client threads to create a cookie file inside the .git directory and then wait until FS events for the cookie are observed by the FS listener thread. This helps address the racy nature of file system events by blocking the client response until the kernel has drained any event backlog. This is especially important on MacOS where kernel events are only issued with a limited frequency. See the `latency` argument of `FSeventStreamCreate()`. The kernel only signals every `latency` seconds, but does not guarantee that the kernel queue is completely drained, so we may have to wait more than one interval. If we increase the latency, the system is more likely to drop events. We avoid these issues by having each client thread create a unique cookie file and then wait until it is seen in the event stream. Co-authored-by: Kevin Willford <Kevin.Willford@microsoft.com> Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff Hostetler committed Mar 25, 2022 at 18:03 UTC b05880d357c6dadba8d1d7943f4782fc25e06999

2 files changed +241 -1

builtin/fsmonitor--daemon.c

+236 -1

@@ -107,6 +107,162 @@ static int do_as_client__status(void)

107 }

108 }

109

110 +enum fsmonitor_cookie_item_result {

111 + FCIR_ERROR = -1, /* could not create cookie file ? */

112 + FCIR_INIT,

113 + FCIR_SEEN,

114 + FCIR_ABORT,

115 +};

116 +

117 +struct fsmonitor_cookie_item {

118 + struct hashmap_entry entry;

119 + char *name;

120 + enum fsmonitor_cookie_item_result result;

121 +};

122 +

123 +static int cookies_cmp(const void *data, const struct hashmap_entry *he1,

124 + const struct hashmap_entry *he2, const void *keydata)

125 +{

126 + const struct fsmonitor_cookie_item *a =

127 + container_of(he1, const struct fsmonitor_cookie_item, entry);

128 + const struct fsmonitor_cookie_item *b =

129 + container_of(he2, const struct fsmonitor_cookie_item, entry);

130 +

131 + return strcmp(a->name, keydata ? keydata : b->name);

132 +}

133 +

134 +static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(

135 + struct fsmonitor_daemon_state *state)

136 +{

137 + /* assert current thread holding state->main_lock */

138 +

139 + int fd;

140 + struct fsmonitor_cookie_item *cookie;

141 + struct strbuf cookie_pathname = STRBUF_INIT;

142 + struct strbuf cookie_filename = STRBUF_INIT;

143 + enum fsmonitor_cookie_item_result result;

144 + int my_cookie_seq;

145 +

146 + CALLOC_ARRAY(cookie, 1);

147 +

148 + my_cookie_seq = state->cookie_seq++;

149 +

150 + strbuf_addf(&cookie_filename, "%i-%i", getpid(), my_cookie_seq);

151 +

152 + strbuf_addbuf(&cookie_pathname, &state->path_cookie_prefix);

153 + strbuf_addbuf(&cookie_pathname, &cookie_filename);

154 +

155 + cookie->name = strbuf_detach(&cookie_filename, NULL);

156 + cookie->result = FCIR_INIT;

157 + hashmap_entry_init(&cookie->entry, strhash(cookie->name));

158 +

159 + hashmap_add(&state->cookies, &cookie->entry);

160 +

161 + trace_printf_key(&trace_fsmonitor, "cookie-wait: '%s' '%s'",

162 + cookie->name, cookie_pathname.buf);

163 +

164 + /*

165 + * Create the cookie file on disk and then wait for a notification

166 + * that the listener thread has seen it.

167 + */

168 + fd = open(cookie_pathname.buf, O_WRONLY | O_CREAT | O_EXCL, 0600);

169 + if (fd < 0) {

170 + error_errno(_("could not create fsmonitor cookie '%s'"),

171 + cookie->name);

172 +

173 + cookie->result = FCIR_ERROR;

174 + goto done;

175 + }

176 +

177 + /*

178 + * Technically, close() and unlink() can fail, but we don't

179 + * care here. We only created the file to trigger a watch

180 + * event from the FS to know that when we're up to date.

181 + */

182 + close(fd);

183 + unlink(cookie_pathname.buf);

184 +

185 + /*

186 + * Technically, this is an infinite wait (well, unless another

187 + * thread sends us an abort). I'd like to change this to

188 + * use `pthread_cond_timedwait()` and return an error/timeout

189 + * and let the caller do the trivial response thing, but we

190 + * don't have that routine in our thread-utils.

191 + *

192 + * After extensive beta testing I'm not really worried about

193 + * this. Also note that the above open() and unlink() calls

194 + * will cause at least two FS events on that path, so the odds

195 + * of getting stuck are pretty slim.

196 + */

197 + while (cookie->result == FCIR_INIT)

198 + pthread_cond_wait(&state->cookies_cond,

199 + &state->main_lock);

200 +

201 +done:

202 + hashmap_remove(&state->cookies, &cookie->entry, NULL);

203 +

204 + result = cookie->result;

205 +

206 + free(cookie->name);

207 + free(cookie);

208 + strbuf_release(&cookie_pathname);

209 +

210 + return result;

211 +}

212 +

213 +/*

214 + * Mark these cookies as _SEEN and wake up the corresponding client threads.

215 + */

216 +static void with_lock__mark_cookies_seen(struct fsmonitor_daemon_state *state,

217 + const struct string_list *cookie_names)

218 +{

219 + /* assert current thread holding state->main_lock */

220 +

221 + int k;

222 + int nr_seen = 0;

223 +

224 + for (k = 0; k < cookie_names->nr; k++) {

225 + struct fsmonitor_cookie_item key;

226 + struct fsmonitor_cookie_item *cookie;

227 +

228 + key.name = cookie_names->items[k].string;

229 + hashmap_entry_init(&key.entry, strhash(key.name));

230 +

231 + cookie = hashmap_get_entry(&state->cookies, &key, entry, NULL);

232 + if (cookie) {

233 + trace_printf_key(&trace_fsmonitor, "cookie-seen: '%s'",

234 + cookie->name);

235 + cookie->result = FCIR_SEEN;

236 + nr_seen++;

237 + }

238 + }

239 +

240 + if (nr_seen)

241 + pthread_cond_broadcast(&state->cookies_cond);

242 +}

243 +

244 +/*

245 + * Set _ABORT on all pending cookies and wake up all client threads.

246 + */

247 +static void with_lock__abort_all_cookies(struct fsmonitor_daemon_state *state)

248 +{

249 + /* assert current thread holding state->main_lock */

250 +

251 + struct hashmap_iter iter;

252 + struct fsmonitor_cookie_item *cookie;

253 + int nr_aborted = 0;

254 +

255 + hashmap_for_each_entry(&state->cookies, &iter, cookie, entry) {

256 + trace_printf_key(&trace_fsmonitor, "cookie-abort: '%s'",

257 + cookie->name);

258 + cookie->result = FCIR_ABORT;

259 + nr_aborted++;

260 + }

261 +

262 + if (nr_aborted)

263 + pthread_cond_broadcast(&state->cookies_cond);

264 +}

265 +

266 /*

267 * Requests to and from a FSMonitor Protocol V2 provider use an opaque

268 * "token" as a virtual timestamp. Clients can request a summary of all

@@ -404,6 +560,9 @@ static void fsmonitor_free_token_data(struct fsmonitor_token_data *token)

560 * We should create a new token and start fresh (as if we just

561 * booted up).

562 *

563 + * [2] Some of those lost events may have been for cookie files. We

564 + * should assume the worst and abort them rather letting them starve.

565 + *

566 * If there are no concurrent threads reading the current token data

567 * series, we can free it now. Otherwise, let the last reader free

568 * it.

@@ -425,6 +584,8 @@ static void with_lock__do_force_resync(struct fsmonitor_daemon_state *state)

584 state->current_token_data = new_one;

585

586 fsmonitor_free_token_data(free_me);

587 +

588 + with_lock__abort_all_cookies(state);

589 }

590

591 void fsmonitor_force_resync(struct fsmonitor_daemon_state *state)

@@ -500,6 +661,8 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,

661 int hash_ret;

662 int do_trivial = 0;

663 int do_flush = 0;

664 + int do_cookie = 0;

665 + enum fsmonitor_cookie_item_result cookie_result;

666

667 /*

668 * We expect `command` to be of the form:

@@ -560,6 +723,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,

723 * We have a V2 valid token:

724 * "builtin:<token_id>:<seq_nr>"

725 */

726 + do_cookie = 1;

727 }

728 }

729

@@ -568,6 +732,30 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,

732 if (!state->current_token_data)

733 BUG("fsmonitor state does not have a current token");

734

735 + /*

736 + * Write a cookie file inside the directory being watched in

737 + * an effort to flush out existing filesystem events that we

738 + * actually care about. Suspend this client thread until we

739 + * see the filesystem events for this cookie file.

740 + *

741 + * Creating the cookie lets us guarantee that our FS listener

742 + * thread has drained the kernel queue and we are caught up

743 + * with the kernel.

744 + *

745 + * If we cannot create the cookie (or otherwise guarantee that

746 + * we are caught up), we send a trivial response. We have to

747 + * assume that there might be some very, very recent activity

748 + * on the FS still in flight.

749 + */

750 + if (do_cookie) {

751 + cookie_result = with_lock__wait_for_cookie(state);

752 + if (cookie_result != FCIR_SEEN) {

753 + error(_("fsmonitor: cookie_result '%d' != SEEN"),

754 + cookie_result);

755 + do_trivial = 1;

756 + }

757 + }

758 +

759 if (do_flush)

760 with_lock__do_force_resync(state);

761

@@ -787,7 +975,9 @@ static int handle_client(void *data,

975 return result;

976 }

977

790 -#define FSMONITOR_COOKIE_PREFIX ".fsmonitor-daemon-"

978 +#define FSMONITOR_DIR "fsmonitor--daemon"

979 +#define FSMONITOR_COOKIE_DIR "cookies"

980 +#define FSMONITOR_COOKIE_PREFIX (FSMONITOR_DIR "/" FSMONITOR_COOKIE_DIR "/")

981

982 enum fsmonitor_path_type fsmonitor_classify_path_workdir_relative(

983 const char *rel)

@@ -940,6 +1130,9 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state,

1130 }

1131 }

1132

1133 + if (cookie_names->nr)

1134 + with_lock__mark_cookies_seen(state, cookie_names);

1135 +

1136 pthread_mutex_unlock(&state->main_lock);

1137 }

1138

@@ -1031,7 +1224,9 @@ static int fsmonitor_run_daemon(void)

1224

1225 memset(&state, 0, sizeof(state));

1226

1227 + hashmap_init(&state.cookies, cookies_cmp, NULL, 0);

1228 pthread_mutex_init(&state.main_lock, NULL);

1229 + pthread_cond_init(&state.cookies_cond, NULL);

1230 state.error_code = 0;

1231 state.current_token_data = fsmonitor_new_token_data();

1232

@@ -1056,6 +1251,44 @@ static int fsmonitor_run_daemon(void)

1251 state.nr_paths_watching = 2;

1252 }

1253

1254 + /*

1255 + * We will write filesystem syncing cookie files into

1256 + * <gitdir>/<fsmonitor-dir>/<cookie-dir>/<pid>-<seq>.

1257 + *

1258 + * The extra layers of subdirectories here keep us from

1259 + * changing the mtime on ".git/" or ".git/foo/" when we create

1260 + * or delete cookie files.

1261 + *

1262 + * There have been problems with some IDEs that do a

1263 + * non-recursive watch of the ".git/" directory and run a

1264 + * series of commands any time something happens.

1265 + *

1266 + * For example, if we place our cookie files directly in

1267 + * ".git/" or ".git/foo/" then a `git status` (or similar

1268 + * command) from the IDE will cause a cookie file to be

1269 + * created in one of those dirs. This causes the mtime of

1270 + * those dirs to change. This triggers the IDE's watch

1271 + * notification. This triggers the IDE to run those commands

1272 + * again. And the process repeats and the machine never goes

1273 + * idle.

1274 + *

1275 + * Adding the extra layers of subdirectories prevents the

1276 + * mtime of ".git/" and ".git/foo" from changing when a

1277 + * cookie file is created.

1278 + */

1279 + strbuf_init(&state.path_cookie_prefix, 0);

1280 + strbuf_addbuf(&state.path_cookie_prefix, &state.path_gitdir_watch);

1281 +

1282 + strbuf_addch(&state.path_cookie_prefix, '/');

1283 + strbuf_addstr(&state.path_cookie_prefix, FSMONITOR_DIR);

1284 + mkdir(state.path_cookie_prefix.buf, 0777);

1285 +

1286 + strbuf_addch(&state.path_cookie_prefix, '/');

1287 + strbuf_addstr(&state.path_cookie_prefix, FSMONITOR_COOKIE_DIR);

1288 + mkdir(state.path_cookie_prefix.buf, 0777);

1289 +

1290 + strbuf_addch(&state.path_cookie_prefix, '/');

1291 +

1292 /*

1293 * Confirm that we can create platform-specific resources for the

1294 * filesystem listener before we bother starting all the threads.

@@ -1068,6 +1301,7 @@ static int fsmonitor_run_daemon(void)

1301 err = fsmonitor_run_daemon_1(&state);

1302

1303 done:

1304 + pthread_cond_destroy(&state.cookies_cond);

1305 pthread_mutex_destroy(&state.main_lock);

1306 fsm_listen__dtor(&state);

1307

@@ -1075,6 +1309,7 @@ done:

1309

1310 strbuf_release(&state.path_worktree_watch);

1311 strbuf_release(&state.path_gitdir_watch);

1312 + strbuf_release(&state.path_cookie_prefix);

1313

1314 return err;

1315 }

fsmonitor--daemon.h

@@ -45,6 +45,11 @@ struct fsmonitor_daemon_state {

46 struct fsmonitor_token_data *current_token_data;

48 + struct strbuf path_cookie_prefix;

49 + pthread_cond_t cookies_cond;

50 + int cookie_seq;

51 + struct hashmap cookies;

52 +

53 int error_code;

54 struct fsmonitor_daemon_backend_data *backend_data;