Skip to content

Add optional ZFile index compression - #465

Open
williswus wants to merge 3 commits into
containerd:mainfrom
williswus:feat/zfile-index-compression
Open

williswus wants to merge 3 commits into
containerd:mainfrom
williswus:feat/zfile-index-compression

Conversation

@williswus

Copy link
Copy Markdown
Contributor

ZFile already compresses data blocks, but stores the block-length index uncompressed. This PR adds optional index compression using the existing format’s flag bit 5.

Changes:

  • Add CompressArgs::compress_index and the --compress-index CLI flag, disabled by default.
  • Share index writing across the whole-file writer and both streaming builders.
  • Support LZ4 and ZSTD, retaining raw indexes when compression provides no space savings or exceeds codec limits.
  • Validate metadata, sizes, checksums and decoded block lengths before exposing the file for reads.
  • Handle short source reads and enforce index limits before buffering additional input.

Format choices:

  • With bit 5 clear, index_size remains the entry count.
  • With bit 5 set, it records compressed bytes; the decoded entry count is derived from the original file size and block size.
  • The index checksum covers the stored bytes.

Existing raw indexes remain supported. Older readers cannot read compressed indexes, so compression remains opt-in. I have not compared these format choices with the historical ease implementation.

Validation:

  • Debug build with QAT disabled.
  • Full zfile_test suite: all 12 tests passed.
  • Coverage includes both codecs, all writers, header/trailer paths, raw fallback, short reads and malformed inputs.
  • git diff --check passed.

Signed-off-by: Willis Wu <willis060510@gmail.com>
Comment thread src/overlaybd/zfile/index_compression.h Outdated
See the License for the specific language governing permissions and
limitations under the License.
*/
#pragma once

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge this file into zfile.cpp

Comment thread src/overlaybd/zfile/zfile.cpp Outdated
}

// Shared by the whole-file writer and both streaming builders.
static int write_index(IFile *file, const std::vector<uint32_t> &lengths,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static? why not let it be a member of ZFileBuilderBase?

pht->opt.use_dict);

if (index_bytes > trailer_offset - pht->index_offset)
LOG_ERROR_RETURN(0, false, "invalid index bytes or size. ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to change so much here?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

An unresolved out-of-bounds read needs correction, and historical format compatibility still requires human review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds opt-in ZFile index compression while preserving raw indexes by default.

Changes:

  • Adds LZ4/ZSTD index compression shared by all writers.
  • Exposes API and CLI options.
  • Strengthens validation and adds round-trip and malformed-input tests.
File Description
src/​tools/​overlaybd-zfile.cpp Adds --compress-index.
src/​overlaybd/​zfile/​zfile.cpp Integrates index compression and validation.
src/​overlaybd/​zfile/​test/​test.cpp Tests writers, codecs, and invalid inputs.
src/​overlaybd/​zfile/​index_compression.h Implements index codec helpers.
src/​overlaybd/​zfile/​format_spec.md Documents compressed-index storage and compatibility.
src/​overlaybd/​zfile/​compressor.h Adds the opt-in API setting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/overlaybd/zfile/zfile.cpp
Signed-off-by: Willis Wu <willis060510@gmail.com>
Comment thread src/overlaybd/zfile/zfile.cpp Outdated

// Whole-index codecs use CPU routines: data-block/QAT buffer limits do not apply.
// Return 1 for compressed output, 0 to keep the original, or -1 on codec error.
static int compress_index_buffer(const void *src, size_t size, uint8_t algo,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it a member of ZFileBuilderBase.

Comment thread src/overlaybd/zfile/zfile.cpp Outdated
}

// A successful decode must restore exactly the expected index byte count.
static int decompress_index_buffer(const void *src, size_t size, void *dst,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it a member of ZFileBuilderBase.

Signed-off-by: Willis Wu <willis060510@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reusing on-disk flag bit 5 requires human compatibility review against historical compressed-index implementations.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

}

// Check before buffering or queueing data, not after the index has already grown.
static int check_input_size(uint64_t current_size, size_t count, uint32_t block_size) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put it into ZFileBuilderBase, not necessarily static.

virtual int fini() = 0;
// Whole-index codecs use CPU routines: data-block/QAT buffer limits do not apply.
// Return 1 for compressed output, 0 to keep the original, or -1 on codec error.
static int compress_index_buffer(const void *src, size_t size, uint8_t algo,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily static

}

// A successful decode must restore exactly the expected index byte count.
static int decompress_index_buffer(const void *src, size_t size, void *dst,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily static

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants