Re: [PATCH 2/4] object-file: only flush batched fsync writes when enabled

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 09, 2026 at 06:22:49PM -0600, Justin Tobler wrote:
> When committing an ODB transaction for the "files" backend, a full
> hardware flush is issued against a temporary file before the staged
> objects are migrated into the main object database. This flush acts as a
> barrier for objects that have been written with batched fsync, where
> `odb_transaction_files_fsync()` only issues a writeout request for each
> object instead of fully syncing it.

Hmm, okay. The difference here is bascially that we don't issue a
hardware flush. So we know that the data is not in the page cache
anymore, but it may still only be cached by the disk.

> The flush is issued whenever the transaction has a temporary object
> directory though, regardless of whether batched fsync is actually
> enabled. For transactions configured for git-receive-pack(1), the
> temporary object directory is always created, so the flush is also
> performed when objects are synced individually or not synced at all. In
> those cases there are no pending writeout requests and the flush is
> unnecessary. Only issue the flush when batched fsync is enabled
> accordingly.

So this is essentially a performance optimization?

> diff --git a/object-file.c b/object-file.c
> index 201d9d6490..da189111bd 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1330,6 +1330,29 @@ static int write_pack_lockfiles(struct odb_transaction_files *transaction)
>  	return ret;
>  }
>  
> +static void flush_batched_fsync(struct repository *repo)

Okay. Instead of adding more nesting you extract the code into a
function that gets called conditionally. Fair.

> +{
> +	struct strbuf temp_path = STRBUF_INIT;
> +	struct tempfile *temp;
> +
> +	/*
> +	 * Issue a full hardware flush against a temporary file to ensure
> +	 * that all objects are durable before any renames occur. The code in
> +	 * odb_transaction_files_fsync has already issued a writeout
> +	 * request, but it has not flushed any writeback cache in the storage
> +	 * hardware or any filesystem logs. This fsync call acts as a barrier
> +	 * to ensure that the data in each new object file is durable before
> +	 * the final name is visible.
> +	 */
> +	strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX",
> +		    repo_get_object_directory(repo));
> +	temp = xmks_tempfile(temp_path.buf);
> +	fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
> +
> +	strbuf_release(&temp_path);
> +	delete_tempfile(&temp);
> +}
> +
>  static int odb_transaction_files_commit(struct odb_transaction *base)
>  {
>  	struct odb_transaction_files *transaction =
> @@ -1339,24 +1362,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  	flush_packfile_transaction(transaction);
>  
>  	if (transaction->objdir) {
> -		struct strbuf temp_path = STRBUF_INIT;
> -		struct tempfile *temp;
> -
> -		/*
> -		 * Issue a full hardware flush against a temporary file to ensure
> -		 * that all objects are durable before any renames occur. The code in
> -		 * odb_transaction_files_fsync has already issued a writeout
> -		 * request, but it has not flushed any writeback cache in the storage
> -		 * hardware or any filesystem logs. This fsync call acts as a barrier
> -		 * to ensure that the data in each new object file is durable before
> -		 * the final name is visible.
> -		 */
> -		strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX",
> -			    repo_get_object_directory(transaction->base.source->odb->repo));
> -		temp = xmks_tempfile(temp_path.buf);
> -		fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
> -		delete_tempfile(&temp);
> -		strbuf_release(&temp_path);
> +		if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
> +			flush_batched_fsync(transaction->base.source->odb->repo);

I assume that we only support batched fsyncs for loose objects in the
first place? In other words, if a transaction were to write a bunch of
packfiles, it would not be possible to batch-fsync those right now?

If so this new condition correct to me. But I think this should be
documented as part of the commit message, because it's an implicit
assumption.

Patrick