Re: [PATCH 4/8] reftable/stack: simplify tracking of table locks
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 2, 2024, 23:00 UTC
- Message-ID
- <xmqqy15e8r8b.fsf@gitster.g>
- In-Reply-To
- <40d9f75cf20d4b76adb1683709e054e264d4e06f.1722435214.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 23 quoted lines
> - size_t i;
> + size_t i, nlocks = 0;
>
> if (first > last || (!expiry && first == last)) {
> err = 0;
> @@ -1051,7 +1051,7 @@ static int stack_compact_range(struct reftable_stack *st,
> for (i = first; i <= last; i++) {
> stack_filename(&table_name, st, reader_name(st->readers[i]));
>
> - err = hold_lock_file_for_update(&table_locks[i - first],
> + err = hold_lock_file_for_update(&table_locks[nlocks],
> table_name.buf, LOCK_NO_DEREF);
> if (err < 0) {
> if (errno == EEXIST)
> @@ -1066,7 +1066,7 @@ static int stack_compact_range(struct reftable_stack *st,
> * run into file descriptor exhaustion when we compress a lot
> * of tables.
> */
> - err = close_lock_file_gently(&table_locks[i - first]);
> + err = close_lock_file_gently(&table_locks[nlocks++]);
> if (err < 0) {
> err = REFTABLE_IO_ERROR;
> goto done;The only unusual control flow in this loop that runs i from first to last is to leave it upon an error, so "i - first" and "nlocks" is always the same, at this step in the patch series.
Show 11 quoted lines
> @@ -1183,8 +1183,8 @@ static int stack_compact_range(struct reftable_stack *st,
> * Delete the old tables. They may still be in use by concurrent
> * readers, so it is expected that unlinking tables may fail.
> */
> - for (i = first; i <= last; i++) {
> - struct lock_file *table_lock = &table_locks[i - first];
> + for (i = 0; i < nlocks; i++) {
> + struct lock_file *table_lock = &table_locks[i];
> char *table_path = get_locked_file_path(table_lock);
> unlink(table_path);
> free(table_path);And this one at this step in the patch series is skipped if the earlier loop saw even a single error, so again, this is a benign noop change.
Show 8 quoted lines
> @@ -1192,8 +1192,8 @@ static int stack_compact_range(struct reftable_stack *st, > > done: > rollback_lock_file(&tables_list_lock); > - for (i = first; table_locks && i <= last; i++) > - rollback_lock_file(&table_locks[i - first]); > + for (i = 0; table_locks && i < nlocks; i++) > + rollback_lock_file(&table_locks[i]);
This is a true bugfix, isn't it? If we failed to create lock file somewhere in the middle, we used to still go ahead and attempted rollback_lock_file() on all of them. Now we rollback only what we successfully called hold_lock_file_for_update().
I wonder why nobody segfaulted where after a failed lock. The answer probably is that lk->tempfile that is NULL will safely bypass most of the things because is_tempfile_active() would say "false" on such a lockfile. But still it probably still were wrong to call rollback_lock_file() on a "struct lockfile" full of NUL-bytes, and it is good that we no longer do that.
> reftable_free(table_locks); > > delete_tempfile(&new_table);