Re: [PATCH 7/8] core.fsync: new option to harden loose references
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 10, 2022, 18:25 UTC
- Message-ID
- <xmqq4k45fyvc.fsf@gitster.g>
- In-Reply-To
- <f1e8a7bb3bf0f4c0414819cb1d5579dc08fd2a4f.1646905589.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 12 quoted lines
> 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.
Show 7 quoted lines
> @@ -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?
Show 7 quoted lines
> +{
> + 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().
Show 16 quoted lines
> @@ -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.
Show 21 quoted lines
> @@ -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);