git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] reftable/stack: fix race in up-to-date check

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 18, 2024, 20:07 UTC
Message-ID
<xmqqcytyquc5.fsf@gitster.g>
In-Reply-To
<713e51a25c1c4cfa830db97f71cd7c39e85864d4.1705585037.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> This should address the race in a POSIX-compliant way. The only real
> downside is that this mechanism cannot be used on non-POSIX-compliant
> systems like Windows. But we at least have the second-level caching
> mechanism in place that compares contents of "files.list" with the
> currently loaded list of tables.
OK.
Show 13 quoted lines
> +	/*
> +	 * Cache stat information in case it provides a useful signal to us.
> +	 * According to POSIX, "The st_ino and st_dev fields taken together
> +	 * uniquely identify the file within the system." That being said,
> +	 * Windows is not POSIX compliant and we do not have these fields
> +	 * available. So the information we have there is insufficient to
> +	 * determine whether two file descriptors point to the same file.
> +	 *
> +	 * While we could fall back to using other signals like the file's
> +	 * mtime, those are not sufficient to avoid races. We thus refrain from
> +	 * using the stat cache on such systems and fall back to the secondary
> +	 * caching mechanism, which is to check whether contents of the file
> +	 * have changed.
OK.
Show 23 quoted lines
> +	 *
> +	 * On other systems which are POSIX compliant we must keep the file
> +	 * descriptor open. This is to avoid a race condition where two
> +	 * processes access the reftable stack at the same point in time:
> +	 *
> +	 *   1. A reads the reftable stack and caches its stat info.
> +	 *
> +	 *   2. B updates the stack, appending a new table to "tables.list".
> +	 *      This will both use a new inode and result in a different file
> +	 *      size, thus invalidating A's cache in theory.
> +	 *
> +	 *   3. B decides to auto-compact the stack and merges two tables. The
> +	 *      file size now matches what A has cached again. Furthermore, the
> +	 *      filesystem may decide to recycle the inode number of the file
> +	 *      we have replaced in (2) because it is not in use anymore.
> +	 *
> +	 *   4. A reloads the reftable stack. Neither the inode number nor the
> +	 *      file size changed. If the timestamps did not change either then
> +	 *      we think the cached copy of our stack is up-to-date.
> +	 *
> +	 * By keeping the file descriptor open the inode number cannot be
> +	 * recycled, mitigating the race.
> +	 */
This is nasty.  Well diagnosed and fixed.
Will queue.
Thanks.
Previous: Patrick SteinhardtNext: Jeff King
Message 4 of 7 in “reftable/stack: fix race in up-to-date check”
  1. 0/2 reftable/stack: fix race in up-to-date checkPatrick Steinhardt, Jan 18, 2024
  2. 1/2 reftable/stack: unconditionally reload stack after commitPatrick Steinhardt, Jan 18, 2024
  3. 2/2 reftable/stack: fix race in up-to-date checkPatrick Steinhardt, Jan 18, 2024
  4. Junio C HamanoJan 18, 2024
  5. Jeff KingJan 20, 2024
  6. Patrick SteinhardtJan 22, 2024
  7. Jeff KingJan 23, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.