[v2,00/10] reftable: fix reload with active iterators

Message

Patrick Steinhardt Aug. 23, 2024, 2:12 p.m. UTC

Hi,

this is the second version of my patch series that fixes issues in the
reftable libary caused by having concurrent readers and writers.

Changes compared to v1:

  - Remove a spurious change that renamed `init_reader()` to
    `reader_init()`. That function went away in the subsequent commit
    anyway.

  - Fix a typo in a commit message.

  - Fix some assertions in one of the added tests.

Thanks!

Patrick

Patrick Steinhardt (10):
  reftable/blocksource: drop malloc block source
  reftable/stack: inline `stack_compact_range_stats()`
  reftable/reader: rename `reftable_new_reader()`
  reftable/reader: inline `init_reader()`
  reftable/reader: inline `reader_close()`
  reftable/stack: fix broken refnames in `write_n_ref_tables()`
  reftable/reader: introduce refcounting
  reftable/reader: keep readers alive during iteration
  reftable/stack: reorder swapping in the reloaded stack contents
  reftable/stack: fix segfault when reload with reused readers fails

 reftable/block_test.c            |   3 +-
 reftable/blocksource.c           |  20 -----
 reftable/blocksource.h           |   2 -
 reftable/reader.c                | 149 ++++++++++++++++---------------
 reftable/reader.h                |   5 +-
 reftable/readwrite_test.c        |  85 +++++++++---------
 reftable/reftable-reader.h       |  19 ++--
 reftable/stack.c                 |  90 +++++++++++--------
 reftable/stack_test.c            | 116 +++++++++++++++++++++++-
 t/helper/test-reftable.c         |   4 +-
 t/unit-tests/t-reftable-merged.c |  10 +--
 11 files changed, 312 insertions(+), 191 deletions(-)

Range-diff against v1:
 1:  fee3d3523eb =  1:  fee3d3523eb reftable/blocksource: drop malloc block source
 2:  3c0cf2bf46f =  2:  3c0cf2bf46f reftable/stack: inline `stack_compact_range_stats()`
 3:  e658b372f04 !  3:  b4cf97bf758 reftable/reader: rename `reftable_new_reader()`
    @@ reftable/reader.c: int reftable_reader_print_blocks(const char *tablename)
      		goto done;
      
     
    - ## reftable/reader.h ##
    -@@ reftable/reader.h: struct reftable_reader {
    - 	struct reftable_reader_offsets log_offsets;
    - };
    - 
    --int init_reader(struct reftable_reader *r, struct reftable_block_source *source,
    -+int reader_init(struct reftable_reader *r, struct reftable_block_source *source,
    - 		const char *name);
    - void reader_close(struct reftable_reader *r);
    - const char *reader_name(struct reftable_reader *r);
    -
      ## reftable/readwrite_test.c ##
     @@ reftable/readwrite_test.c: static void test_write_empty_table(void)
      
 4:  f628b7dafb9 !  4:  3b667097501 reftable/reader: inline `init_reader()`
    @@ reftable/reader.h: struct reftable_reader {
      	struct reftable_reader_offsets log_offsets;
      };
      
    --int reader_init(struct reftable_reader *r, struct reftable_block_source *source,
    +-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,
     -		const char *name);
      void reader_close(struct reftable_reader *r);
      const char *reader_name(struct reftable_reader *r);
 5:  4a9fe150427 =  5:  b129d8a8687 reftable/reader: inline `reader_close()`
 6:  4965402e7bf =  6:  e3b28709b5f reftable/stack: fix broken refnames in `write_n_ref_tables()`
 7:  fc0ed68d467 =  7:  6535d1ca9de reftable/reader: introduce refcounting
 8:  02682056288 !  8:  8d08c3bc515 reftable/reader: keep readers alive during iteration
    @@ Metadata
      ## Commit message ##
         reftable/reader: keep readers alive during iteration
     
    -    The lifetime of a table iterator may surive the lifetime of a reader
    +    The lifetime of a table iterator may survive the lifetime of a reader
         when the stack gets reloaded. Keep the reader from being released by
         increasing its refcount while the iterator is still being used.
     
    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_cle
     +	EXPECT(st2->merged->readers_len == 2);
     +	err = reftable_stack_compact_all(st2, NULL);
     +	EXPECT_ERR(err);
    ++	EXPECT(st2->merged->readers_len == 1);
     +
     +	/*
     +	 * Verify that we can continue to use the old iterator even after we
    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_cle
     +	 */
     +	err = reftable_stack_reload(st1);
     +	EXPECT_ERR(err);
    -+	EXPECT(st2->merged->readers_len == 1);
    ++	EXPECT(st1->merged->readers_len == 1);
     +	err = reftable_iterator_next_ref(&it, &rec);
     +	EXPECT_ERR(err);
     +	EXPECT(!strcmp(rec.refname, "refs/heads/branch-0000"));
 9:  d98316fbf4c =  9:  5aee91de25e reftable/stack: reorder swapping in the reloaded stack contents
10:  b777818ea99 = 10:  4a8d45cc9b4 reftable/stack: fix segfault when reload with reused readers fails

Comments

Junio C Hamano Aug. 23, 2024, 4:49 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> Changes compared to v1:
>
>   - Remove a spurious change that renamed `init_reader()` to
>     `reader_init()`. That function went away in the subsequent commit
>     anyway.
>
>   - Fix a typo in a commit message.
>
>   - Fix some assertions in one of the added tests.

All changes relative to v1 looked sensible to me.

Will replace.