diff --git a/reftable/stack.c b/reftable/stack.c index 3f13c3eb34..2071e428a8 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -999,6 +999,15 @@ done: return err; } +enum stack_compact_range_flags { + /* + * Perform a best-effort compaction. That is, even if we cannot lock + * all tables in the specified range, we will try to compact the + * remaining slice. + */ + STACK_COMPACT_RANGE_BEST_EFFORT = (1 << 0), +}; + /* * Compact all tables in the range `[first, last)` into a single new table. * @@ -1010,7 +1019,8 @@ done: */ static int stack_compact_range(struct reftable_stack *st, size_t first, size_t last, - struct reftable_log_expiry_config *expiry) + struct reftable_log_expiry_config *expiry, + unsigned int flags) { struct strbuf tables_list_buf = STRBUF_INIT; struct strbuf new_table_name = STRBUF_INIT; @@ -1052,19 +1062,47 @@ static int stack_compact_range(struct reftable_stack *st, /* * Lock all tables in the user-provided range. This is the slice of our * stack which we'll compact. + * + * Note that we lock tables in reverse order from last to first. The + * intent behind this is to allow a newer process to perform best + * effort compaction of tables that it has added in the case where an + * older process is still busy compacting tables which are preexisting + * from the point of view of the newer process. */ REFTABLE_CALLOC_ARRAY(table_locks, last - first + 1); - for (i = first; i <= last; i++) { - stack_filename(&table_name, st, reader_name(st->readers[i])); + for (i = last + 1; i > first; i--) { + stack_filename(&table_name, st, reader_name(st->readers[i - 1])); err = hold_lock_file_for_update(&table_locks[nlocks], table_name.buf, LOCK_NO_DEREF); if (err < 0) { - if (errno == EEXIST) + /* + * When the table is locked already we may do a + * best-effort compaction and compact only the tables + * that we have managed to lock so far. This of course + * requires that we have been able to lock at least two + * tables, otherwise there would be nothing to compact. + * In that case, we return a lock error to our caller. + */ + if (errno == EEXIST && last - (i - 1) >= 2 && + flags & STACK_COMPACT_RANGE_BEST_EFFORT) { + err = 0; + /* + * The subtraction is to offset the index, the + * addition is to only compact up to the table + * of the preceding iteration. They obviously + * cancel each other out, but that may be + * non-obvious when it was omitted. + */ + first = (i - 1) + 1; + break; + } else if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; - else + goto done; + } else { err = REFTABLE_IO_ERROR; - goto done; + goto done; + } } /* @@ -1308,9 +1346,10 @@ done: static int stack_compact_range_stats(struct reftable_stack *st, size_t first, size_t last, - struct reftable_log_expiry_config *config) + struct reftable_log_expiry_config *config, + unsigned int flags) { - int err = stack_compact_range(st, first, last, config); + int err = stack_compact_range(st, first, last, config, flags); if (err == REFTABLE_LOCK_ERROR) st->stats.failures++; return err; @@ -1320,7 +1359,7 @@ int reftable_stack_compact_all(struct reftable_stack *st, struct reftable_log_expiry_config *config) { size_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0; - return stack_compact_range_stats(st, 0, last, config); + return stack_compact_range_stats(st, 0, last, config, 0); } static int segment_size(struct segment *s) @@ -1427,7 +1466,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) reftable_free(sizes); if (segment_size(&seg) > 0) return stack_compact_range_stats(st, seg.start, seg.end - 1, - NULL); + NULL, STACK_COMPACT_RANGE_BEST_EFFORT); return 0; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 3ed8e44924..8c36590ff0 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -917,13 +917,15 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void) write_file_buf(buf.buf, "", 0); /* - * Ideally, we'd handle the situation where any of the tables is locked - * gracefully. We don't (yet) do this though and thus fail. + * When parts of the stack are locked, then auto-compaction does a best + * effort compaction of those tables which aren't locked. So while this + * would in theory compact all tables, due to the preexisting lock we + * only compact the newest two tables. */ err = reftable_stack_auto_compact(st); - EXPECT(err == REFTABLE_LOCK_ERROR); - EXPECT(st->stats.failures == 1); - EXPECT(st->merged->stack_len == 5); + EXPECT_ERR(err); + EXPECT(st->stats.failures == 0); + EXPECT(st->merged->stack_len == 4); reftable_stack_destroy(st); strbuf_release(&buf); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index b06c46999d..37510c2b2a 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -478,19 +478,26 @@ test_expect_success "$command: auto compaction" ' test_oid blob17_2 | git hash-object -w --stdin && - # Lock all tables write some refs. Auto-compaction will be - # unable to compact tables and thus fails gracefully, leaving - # the stack in a sub-optimal state. - ls .git/reftable/*.ref | + # Lock all tables, write some refs. Auto-compaction will be + # unable to compact tables and thus fails gracefully, + # compacting only those tables which are not locked. + ls .git/reftable/*.ref | sort | while read table do - touch "$table.lock" || exit 1 + touch "$table.lock" && + basename "$table" >>tables.expect || exit 1 done && + test_line_count = 2 .git/reftable/tables.list && git branch B && git branch C && - rm .git/reftable/*.lock && - test_line_count = 4 .git/reftable/tables.list && + # The new tables are auto-compacted, but the locked tables are + # left intact. + test_line_count = 3 .git/reftable/tables.list && + head -n 2 .git/reftable/tables.list >tables.head && + test_cmp tables.expect tables.head && + + rm .git/reftable/*.lock && git $command --auto && test_line_count = 1 .git/reftable/tables.list )