Re: [PATCH 7/8] core.fsync: new option to harden loose references

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 973805e8a9..b67d3c340e 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -564,8 +564,10 @@ core.fsync::
>  * `pack-metadata` hardens packfile bitmaps and indexes.
>  * `commit-graph` hardens the commit graph file.
>  * `index` hardens the index when it is modified.
> +* `loose-ref` hardens references modified in the repo in loose-ref form.
>  * `objects` is an aggregate option that is equivalent to
>    `loose-object,pack`.
> +* `refs` is an aggregate option that is equivalent to `loose-ref`.

Aggregate of one feels strange.  I do not see a strong reason to
have two separate classes loose vs packed and allow them be
protected to different robustness, but that aside, if we were to
have two, this "aggregate" is better added when the second one is.

Having said that, given that a separate ref backend that has no
distinction between loose or packed is on horizen, I think we would
rather prefer to see a single "ref" component that governs all
backends.

> @@ -1026,7 +1029,8 @@ enum fsync_component {
>  			      FSYNC_COMPONENT_PACK | \
>  			      FSYNC_COMPONENT_PACK_METADATA | \
>  			      FSYNC_COMPONENT_COMMIT_GRAPH | \
> -			      FSYNC_COMPONENT_INDEX)
> +			      FSYNC_COMPONENT_INDEX | \
> +			      FSYNC_COMPONENT_LOOSE_REF)

OK.

> +static int files_sync_loose_ref(struct ref_lock *lock, struct strbuf *err)

This file-scope static function will not be in the vtable (in other
words, it is not like "sync_loose_ref" method must be defined across
all ref backends and this function is called as the implementation
of the method for the files backend), so we do not have to give it
"files_" prefix if we do not want to.

sync_loose_ref() may be easier to read, perhaps?

> +{
> +	int ret = fsync_component(FSYNC_COMPONENT_LOOSE_REF, get_lock_file_fd(&lock->lk));
> +	if (ret)
> +		strbuf_addf(err, "could not sync loose ref '%s': %s", lock->ref_name,
> +			    strerror(errno));
> +	return ret;
> +}

OK.

Good illustration how the new helper in 6/8 is useful.  It would be
nice if the reroll of the base topic by Neeraj reorders the patches
to introduce fsync_component() much earlier, at the same time it
introduces the fsync_component_or_die().

> @@ -1504,6 +1513,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
>  	oidcpy(&lock->old_oid, &orig_oid);
>  
>  	if (write_ref_to_lockfile(lock, &orig_oid, 0, &err) ||
> +	    files_sync_loose_ref(lock, &err) ||
>  	    commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {
>  		error("unable to write current sha1 into %s: %s", newrefname, err.buf);
>  		strbuf_release(&err);
> @@ -1524,6 +1534,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
>  	flag = log_all_ref_updates;
>  	log_all_ref_updates = LOG_REFS_NONE;
>  	if (write_ref_to_lockfile(lock, &orig_oid, 0, &err) ||
> +	    files_sync_loose_ref(lock, &err) ||
>  	    commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {
>  		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
>  		strbuf_release(&err);

We used to skip commit_ref_update() and gone to the error code path
upon write failure, and we do the same upon fsync failure now.  The
error code path will do the rollback the same way as before.

OK, these look sensible.

> @@ -2819,6 +2830,24 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  		}
>  	}
>  
> +	/*
> +	 * Sync all lockfiles to disk to ensure data consistency. We do this in
> +	 * a separate step such that we can sync all modified refs in a single
> +	 * step, which may be more efficient on some filesystems.
> +	 */
> +	for (i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> +		struct ref_lock *lock = update->backend_data;
> +
> +		if (!(update->flags & REF_NEEDS_COMMIT))
> +			continue;
> +
> +		if (files_sync_loose_ref(lock, err)) {
> +			ret  = TRANSACTION_GENERIC_ERROR;
> +			goto cleanup;
> +		}
> +	}

An obvious alternative that naïvely comes to mind is to keep going
after seeing the first failure to sync and attempt to sync all the
rest, but remember that we had an error.  But I think what this
patch does makes a lot more sense, as the error code path will just
cleans the transaction up.  If any of them fails, there is no reason
to spend more effort.

Thanks.

>  cleanup:
>  	free(head_ref);
>  	string_list_clear(&affected_refnames, 0);