From b23bf16efc0d73d00e1c55139e8454abcaa89f7b Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Wed, 19 Oct 2022 17:29:51 +0000 Subject: [PATCH] libsnapshot: Changes to AddCopy() API If the copy blocks are contiguous, add a third argument which takes the number of blocks which are contiguous. With this, update engine can call the API in one shot for all the contiguous COPY operations. This is required for batching the I/O for async writes. This should still continue to support the existing API where we pass one COPY block at a time. Bug: 254188450 Test: Incremental OTA from A->B with new API changes in A Incremental OTA from A->B with plain VAB cow_api_test Signed-off-by: Akilesh Kailash Change-Id: I7edc52a152e02de28a44ef1dc2c88b76a28c4109 --- .../include/libsnapshot/cow_writer.h | 9 ++-- .../libsnapshot/mock_snapshot_writer.h | 2 +- .../include/libsnapshot/snapshot_writer.h | 4 +- .../libsnapshot_cow/cow_api_test.cpp | 42 +++++++++++++++++++ .../libsnapshot_cow/cow_writer.cpp | 32 +++++++++----- fs_mgr/libsnapshot/snapshot_writer.cpp | 29 +++++++++---- 6 files changed, 92 insertions(+), 26 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/cow_writer.h b/fs_mgr/libsnapshot/include/libsnapshot/cow_writer.h index e7a2f02f0..19f3649a7 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/cow_writer.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/cow_writer.h @@ -52,8 +52,9 @@ class ICowWriter { virtual ~ICowWriter() {} // Encode an operation that copies the contents of |old_block| to the - // location of |new_block|. - bool AddCopy(uint64_t new_block, uint64_t old_block); + // location of |new_block|. 'num_blocks' is the number of contiguous + // COPY operations from |old_block| to |new_block|. + bool AddCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks = 1); // Encode a sequence of raw blocks. |size| must be a multiple of the block size. bool AddRawBlocks(uint64_t new_block_start, const void* data, size_t size); @@ -84,7 +85,7 @@ class ICowWriter { const CowOptions& options() { return options_; } protected: - virtual bool EmitCopy(uint64_t new_block, uint64_t old_block) = 0; + virtual bool EmitCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks = 1) = 0; virtual bool EmitRawBlocks(uint64_t new_block_start, const void* data, size_t size) = 0; virtual bool EmitXorBlocks(uint32_t new_block_start, const void* data, size_t size, uint32_t old_block, uint16_t offset) = 0; @@ -122,7 +123,7 @@ class CowWriter : public ICowWriter { uint32_t GetCowVersion() { return header_.major_version; } protected: - virtual bool EmitCopy(uint64_t new_block, uint64_t old_block) override; + virtual bool EmitCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks = 1) override; virtual bool EmitRawBlocks(uint64_t new_block_start, const void* data, size_t size) override; virtual bool EmitXorBlocks(uint32_t new_block_start, const void* data, size_t size, uint32_t old_block, uint16_t offset) override; diff --git a/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot_writer.h b/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot_writer.h index b0be5a594..29828bcfb 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot_writer.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot_writer.h @@ -34,7 +34,7 @@ class MockSnapshotWriter : public ISnapshotWriter { // Returns true if AddCopy() operations are supported. MOCK_METHOD(bool, SupportsCopyOperation, (), (const override)); - MOCK_METHOD(bool, EmitCopy, (uint64_t, uint64_t), (override)); + MOCK_METHOD(bool, EmitCopy, (uint64_t, uint64_t, uint64_t), (override)); MOCK_METHOD(bool, EmitRawBlocks, (uint64_t, const void*, size_t), (override)); MOCK_METHOD(bool, EmitXorBlocks, (uint32_t, const void*, size_t, uint32_t, uint16_t), (override)); diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_writer.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_writer.h index 545f1171d..0e3b1db6b 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_writer.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_writer.h @@ -74,7 +74,7 @@ class CompressedSnapshotWriter final : public ISnapshotWriter { bool VerifyMergeOps() const noexcept; protected: - bool EmitCopy(uint64_t new_block, uint64_t old_block) override; + bool EmitCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks = 1) override; bool EmitRawBlocks(uint64_t new_block_start, const void* data, size_t size) override; bool EmitXorBlocks(uint32_t new_block_start, const void* data, size_t size, uint32_t old_block, uint16_t offset) override; @@ -113,7 +113,7 @@ class OnlineKernelSnapshotWriter final : public ISnapshotWriter { bool EmitZeroBlocks(uint64_t new_block_start, uint64_t num_blocks) override; bool EmitXorBlocks(uint32_t new_block_start, const void* data, size_t size, uint32_t old_block, uint16_t offset) override; - bool EmitCopy(uint64_t new_block, uint64_t old_block) override; + bool EmitCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks = 1) override; bool EmitLabel(uint64_t label) override; bool EmitSequenceData(size_t num_ops, const uint32_t* data) override; diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_api_test.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_api_test.cpp index ba4044f7d..2c1187fd2 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_api_test.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_api_test.cpp @@ -62,6 +62,48 @@ class StringSink : public IByteSink { std::string stream_; }; +TEST_F(CowTest, CopyContiguous) { + CowOptions options; + options.cluster_ops = 0; + CowWriter writer(options); + + ASSERT_TRUE(writer.Initialize(cow_->fd)); + + ASSERT_TRUE(writer.AddCopy(10, 1000, 100)); + ASSERT_TRUE(writer.Finalize()); + ASSERT_EQ(lseek(cow_->fd, 0, SEEK_SET), 0); + + CowReader reader; + CowHeader header; + CowFooter footer; + ASSERT_TRUE(reader.Parse(cow_->fd)); + ASSERT_TRUE(reader.GetHeader(&header)); + ASSERT_TRUE(reader.GetFooter(&footer)); + ASSERT_EQ(header.magic, kCowMagicNumber); + ASSERT_EQ(header.major_version, kCowVersionMajor); + ASSERT_EQ(header.minor_version, kCowVersionMinor); + ASSERT_EQ(header.block_size, options.block_size); + ASSERT_EQ(footer.op.num_ops, 100); + + auto iter = reader.GetOpIter(); + ASSERT_NE(iter, nullptr); + ASSERT_FALSE(iter->Done()); + + size_t i = 0; + while (!iter->Done()) { + auto op = &iter->Get(); + ASSERT_EQ(op->type, kCowCopyOp); + ASSERT_EQ(op->compression, kCowCompressNone); + ASSERT_EQ(op->data_length, 0); + ASSERT_EQ(op->new_block, 10 + i); + ASSERT_EQ(op->source, 1000 + i); + iter->Next(); + i += 1; + } + + ASSERT_EQ(i, 100); +} + TEST_F(CowTest, ReadWrite) { CowOptions options; options.cluster_ops = 0; diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp index e4f019ee5..015bff0d1 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp @@ -38,11 +38,16 @@ static_assert(sizeof(off_t) == sizeof(uint64_t)); using android::base::borrowed_fd; using android::base::unique_fd; -bool ICowWriter::AddCopy(uint64_t new_block, uint64_t old_block) { - if (!ValidateNewBlock(new_block)) { - return false; +bool ICowWriter::AddCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks) { + CHECK(num_blocks != 0); + + for (size_t i = 0; i < num_blocks; i++) { + if (!ValidateNewBlock(new_block + i)) { + return false; + } } - return EmitCopy(new_block, old_block); + + return EmitCopy(new_block, old_block, num_blocks); } bool ICowWriter::AddRawBlocks(uint64_t new_block_start, const void* data, size_t size) { @@ -286,13 +291,20 @@ bool CowWriter::OpenForAppend(uint64_t label) { return EmitClusterIfNeeded(); } -bool CowWriter::EmitCopy(uint64_t new_block, uint64_t old_block) { +bool CowWriter::EmitCopy(uint64_t new_block, uint64_t old_block, uint64_t num_blocks) { CHECK(!merge_in_progress_); - CowOperation op = {}; - op.type = kCowCopyOp; - op.new_block = new_block; - op.source = old_block; - return WriteOperation(op); + + for (size_t i = 0; i < num_blocks; i++) { + CowOperation op = {}; + op.type = kCowCopyOp; + op.new_block = new_block + i; + op.source = old_block + i; + if (!WriteOperation(op)) { + return false; + } + } + + return true; } bool CowWriter::EmitRawBlocks(uint64_t new_block_start, const void* data, size_t size) { diff --git a/fs_mgr/libsnapshot/snapshot_writer.cpp b/fs_mgr/libsnapshot/snapshot_writer.cpp index 48b7d80f7..6aad3d108 100644 --- a/fs_mgr/libsnapshot/snapshot_writer.cpp +++ b/fs_mgr/libsnapshot/snapshot_writer.cpp @@ -111,8 +111,9 @@ std::unique_ptr CompressedSnapshotWriter::OpenReader() { return reader; } -bool CompressedSnapshotWriter::EmitCopy(uint64_t new_block, uint64_t old_block) { - return cow_->AddCopy(new_block, old_block); +bool CompressedSnapshotWriter::EmitCopy(uint64_t new_block, uint64_t old_block, + uint64_t num_blocks) { + return cow_->AddCopy(new_block, old_block, num_blocks); } bool CompressedSnapshotWriter::EmitRawBlocks(uint64_t new_block_start, const void* data, @@ -191,19 +192,29 @@ bool OnlineKernelSnapshotWriter::EmitZeroBlocks(uint64_t new_block_start, uint64 return true; } -bool OnlineKernelSnapshotWriter::EmitCopy(uint64_t new_block, uint64_t old_block) { +bool OnlineKernelSnapshotWriter::EmitCopy(uint64_t new_block, uint64_t old_block, + uint64_t num_blocks) { auto source_fd = GetSourceFd(); if (source_fd < 0) { return false; } - std::string buffer(options_.block_size, 0); - uint64_t offset = old_block * options_.block_size; - if (!android::base::ReadFullyAtOffset(source_fd, buffer.data(), buffer.size(), offset)) { - PLOG(ERROR) << "EmitCopy read"; - return false; + CHECK(num_blocks != 0); + + for (size_t i = 0; i < num_blocks; i++) { + std::string buffer(options_.block_size, 0); + uint64_t offset = (old_block + i) * options_.block_size; + if (!android::base::ReadFullyAtOffset(source_fd, buffer.data(), buffer.size(), offset)) { + PLOG(ERROR) << "EmitCopy read"; + return false; + } + if (!EmitRawBlocks(new_block + i, buffer.data(), buffer.size())) { + PLOG(ERROR) << "EmitRawBlocks failed"; + return false; + } } - return EmitRawBlocks(new_block, buffer.data(), buffer.size()); + + return true; } bool OnlineKernelSnapshotWriter::EmitLabel(uint64_t) {