| Message ID | 20241221-b4-pks-reftable-oom-fix-without-readers-v1-1-12db83a3267c@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: fix out-of-memory errors on NonStop | expand |
Patrick Steinhardt <ps@pks.im> writes: > In order to compact tables we need at least two tables. Bail out early > from `reftable_stack_auto_compact()` in case we have less than two > tables. While that is very true, bailing out on "< 2" would change the behaviour. Where there were only one table, earlier we still went ahead and exercised compaction code path, but now we no longer do. The end result of having just a single table might logically be the same with this change, but if we were relying on some side effects of exercising the compaction code path, do we know that the rest of the code is OK? That's the kind of questions I would ask, if this were somebody who hasn't been deeply involved in the reftable code and came this deep in the pre-release period. But since we all know you have been the main driver for this effort, we'd take your word for it ;-) Thanks, will queue. > This is mostly defense in depth: `stack_table_sizes_for_compaction()` > may try to allocate a zero-byte object when there aren't any readers, > and that may lead to a `NULL` pointer on some platforms like NonStop > which causes us to bail out with an out-of-memory error. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/stack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/reftable/stack.c b/reftable/stack.c > index 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st) > struct segment seg; > uint64_t *sizes; > > + if (st->merged->readers_len < 2) > + return 0; > + > sizes = stack_table_sizes_for_compaction(st); > if (!sizes) > return REFTABLE_OUT_OF_MEMORY_ERROR;
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> In order to compact tables we need at least two tables. Bail out early >> from `reftable_stack_auto_compact()` in case we have less than two >> tables. > > While that is very true, bailing out on "< 2" would change the > behaviour. Where there were only one table, earlier we still went > ahead and exercised compaction code path, but now we no longer do. > The end result of having just a single table might logically be the > same with this change, but if we were relying on some side effects > of exercising the compaction code path, do we know that the rest of > the code is OK? > > That's the kind of questions I would ask, if this were somebody who > hasn't been deeply involved in the reftable code and came this deep > in the pre-release period. But since we all know you have been the > main driver for this effort, we'd take your word for it ;-) > > Thanks, will queue. And with code inspection, it can trivially seen that this change is perfectly fine. In the original, when "== 1", stack_table_sizes_for_compaction(st) yields an array with a single element, suggest_compaction_segment() gives back segment with .start and .end both set to 0, for which segment_size() returns 0 hence we do not call stack_compact_range(). The original code happens to do the same when "== 0" (and 0-sized allocation does not give back NULL). So bypassing all of these when "== 1" is a no-op change, an optimization to avoid allocating a single-element array and then immediately freeing it. Doing so when "== 0" is a strict improvement on a platform with malloc that returns NULL for 0-sized allocation. So bypassing when "< 2" is totally safe and justifyable change. Thanks.
On Sat, Dec 21, 2024 at 09:51:34AM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >> In order to compact tables we need at least two tables. Bail out early > >> from `reftable_stack_auto_compact()` in case we have less than two > >> tables. > > > > While that is very true, bailing out on "< 2" would change the > > behaviour. Where there were only one table, earlier we still went > > ahead and exercised compaction code path, but now we no longer do. > > The end result of having just a single table might logically be the > > same with this change, but if we were relying on some side effects > > of exercising the compaction code path, do we know that the rest of > > the code is OK? > > > > That's the kind of questions I would ask, if this were somebody who > > hasn't been deeply involved in the reftable code and came this deep > > in the pre-release period. But since we all know you have been the > > main driver for this effort, we'd take your word for it ;-) > > > > Thanks, will queue. > > And with code inspection, it can trivially seen that this change is > perfectly fine. > > In the original, when "== 1", stack_table_sizes_for_compaction(st) > yields an array with a single element, suggest_compaction_segment() > gives back segment with .start and .end both set to 0, for which > segment_size() returns 0 hence we do not call stack_compact_range(). > The original code happens to do the same when "== 0" (and 0-sized > allocation does not give back NULL). > > So bypassing all of these when "== 1" is a no-op change, an > optimization to avoid allocating a single-element array and then > immediately freeing it. Doing so when "== 0" is a strict > improvement on a platform with malloc that returns NULL for 0-sized > allocation. So bypassing when "< 2" is totally safe and justifyable > change. Indeed. I'll include an explanation in v2 of this patch series. Patrick
diff --git a/reftable/stack.c b/reftable/stack.c index 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st) struct segment seg; uint64_t *sizes; + if (st->merged->readers_len < 2) + return 0; + sizes = stack_table_sizes_for_compaction(st); if (!sizes) return REFTABLE_OUT_OF_MEMORY_ERROR;
In order to compact tables we need at least two tables. Bail out early from `reftable_stack_auto_compact()` in case we have less than two tables. This is mostly defense in depth: `stack_table_sizes_for_compaction()` may try to allocate a zero-byte object when there aren't any readers, and that may lead to a `NULL` pointer on some platforms like NonStop which causes us to bail out with an out-of-memory error. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 3 +++ 1 file changed, 3 insertions(+)