reftable/stack: use error codes when locking fails during compaction

Compaction of a reftable stack may fail gracefully when there is a concurrent process that writes to the reftable stack and which has thus locked either the "tables.list" file or one of the tables. This is expected and can be handled gracefully by some of the callers which invoke compaction. Thus, to indicate this situation to our callers, we return a positive return code from `stack_compact_range()` and bubble it up to the caller. This kind of error handling is somewhat awkward though as many callers in the call chain never even think of handling positive return values. Thus, the result is either that such errors are swallowed by accident, or that we abort operations with an unhelpful error message. Make the code more robust by always using negative error codes when compaction fails, with `REFTABLE_LOCK_ERROR` for the described benign error case. Note that only a single callsite knew to handle positive error codes gracefully in the first place. Subsequent commits will touch up some of the other sites to handle those errors better. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Mar 25, 2024 at 11:02 UTC 33358350eb857a5fb273d143dcdfe15ac6d4db5d
1 file changed +13 -5
reftable/stack.c
+13 -5
@@ -973,7 +973,15 @@ done:
973 return err;
974 }
975
976 -/* < 0: error. 0 == OK, > 0 attempt failed; could retry. */
976 +/*
977 + * Compact all tables in the range `[first, last)` into a single new table.
978 + *
979 + * This function returns `0` on success or a code `< 0` on failure. When the
980 + * stack or any of the tables in the specified range are already locked then
981 + * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that
982 + * callers can either ignore, or they may choose to retry compaction after some
983 + * amount of time.
984 + */
985 static int stack_compact_range(struct reftable_stack *st,
986 size_t first, size_t last,
987 struct reftable_log_expiry_config *expiry)
@@ -1003,7 +1011,7 @@ static int stack_compact_range(struct reftable_stack *st,
1011 LOCK_NO_DEREF);
1012 if (err < 0) {
1013 if (errno == EEXIST)
1006 - err = 1;
1014 + err = REFTABLE_LOCK_ERROR;
1015 else
1016 err = REFTABLE_IO_ERROR;
1017 goto done;
@@ -1025,7 +1033,7 @@ static int stack_compact_range(struct reftable_stack *st,
1033 table_name.buf, LOCK_NO_DEREF);
1034 if (err < 0) {
1035 if (errno == EEXIST)
1028 - err = 1;
1036 + err = REFTABLE_LOCK_ERROR;
1037 else
1038 err = REFTABLE_IO_ERROR;
1039 goto done;
@@ -1075,7 +1083,7 @@ static int stack_compact_range(struct reftable_stack *st,
1083 LOCK_NO_DEREF);
1084 if (err < 0) {
1085 if (errno == EEXIST)
1078 - err = 1;
1086 + err = REFTABLE_LOCK_ERROR;
1087 else
1088 err = REFTABLE_IO_ERROR;
1089 goto done;
@@ -1187,7 +1195,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,
1195 struct reftable_log_expiry_config *config)
1196 {
1197 int err = stack_compact_range(st, first, last, config);
1190 - if (err > 0)
1198 + if (err == REFTABLE_LOCK_ERROR)
1199 st->stats.failures++;
1200 return err;
1201 }