Allow to shrink ext2 filesystems.
There was a function a long time ago that didn't handle shrinking an
ext2 filesystem when computing the diff of the blocks that were not
part of a file nor metadata (mostly empty space blocks). This is now
handled in a different way, which is not related to how the filesystems
are parsed in the ABGenerator.
This patch removes the check that prevents the delta generator from
generating those payloads and fixes the inplace generator for partitions
that shrink in size.
Bug: 28846535
TEST=Generated a payload to a smaller ext2 system image, both for
minor version 1 and 2+.
Change-Id: Ide408c48b0032dbe478c8e348c40e1a0b5665ea8
diff --git a/payload_generator/delta_diff_generator.cc b/payload_generator/delta_diff_generator.cc
index 91d212e..a140d21 100644
--- a/payload_generator/delta_diff_generator.cc
+++ b/payload_generator/delta_diff_generator.cc
@@ -97,15 +97,6 @@
if (!old_part.path.empty() &&
!diff_utils::IsSquashfs4Filesystem(new_part.path)) {
// Delta update.
- if (diff_utils::IsExtFilesystem(new_part.path)) {
- LOG_IF(WARNING, old_part.size != new_part.size)
- << "Old and new filesystems have different size.";
- // TODO(deymo): Our tools only support growing the filesystem size
- // during an update. Remove this check when that's fixed.
- // crbug.com/192136
- LOG_IF(FATAL, old_part.size > new_part.size)
- << "Shirking the filesystem size is not supported at the moment.";
- }
if (config.version.minor == kInPlaceMinorPayloadVersion) {
LOG(INFO) << "Using generator InplaceGenerator().";
strategy.reset(new InplaceGenerator());
diff --git a/payload_generator/inplace_generator.cc b/payload_generator/inplace_generator.cc
index 30dbafc..be8b487 100644
--- a/payload_generator/inplace_generator.cc
+++ b/payload_generator/inplace_generator.cc
@@ -677,17 +677,13 @@
enum BlockField { READER = 0, WRITER, BLOCK_FIELD_COUNT };
for (int field = READER; field < BLOCK_FIELD_COUNT; field++) {
- const int extents_size =
- (field == READER) ? operation.src_extents_size() :
- operation.dst_extents_size();
const char* past_participle = (field == READER) ? "read" : "written";
const google::protobuf::RepeatedPtrField<Extent>& extents =
(field == READER) ? operation.src_extents() : operation.dst_extents();
Vertex::Index Block::*access_type = (field == READER) ?
&Block::reader : &Block::writer;
- for (int i = 0; i < extents_size; i++) {
- const Extent& extent = extents.Get(i);
+ for (const Extent& extent : extents) {
for (uint64_t block = extent.start_block();
block < (extent.start_block() + extent.num_blocks()); block++) {
if ((*blocks)[block].*access_type != Vertex::kInvalidIndex) {
@@ -738,6 +734,7 @@
}
bool InplaceGenerator::ResolveReadAfterWriteDependencies(
+ const PartitionConfig& old_part,
const PartitionConfig& new_part,
uint64_t partition_size,
size_t block_size,
@@ -746,7 +743,7 @@
// Convert the operations to the graph.
Graph graph;
CheckGraph(graph);
- vector<Block> blocks(new_part.size / block_size);
+ vector<Block> blocks(std::max(old_part.size, new_part.size) / block_size);
for (const auto& aop : *aops) {
AddInstallOpToGraph(
&graph, Vertex::kInvalidIndex, &blocks, aop.op, aop.name);
@@ -815,12 +812,8 @@
blob_file));
LOG(INFO) << "Done reading " << new_part.name;
- TEST_AND_RETURN_FALSE(
- ResolveReadAfterWriteDependencies(new_part,
- partition_size,
- config.block_size,
- blob_file,
- aops));
+ TEST_AND_RETURN_FALSE(ResolveReadAfterWriteDependencies(
+ old_part, new_part, partition_size, config.block_size, blob_file, aops));
LOG(INFO) << "Done reordering " << new_part.name;
return true;
}
diff --git a/payload_generator/inplace_generator.h b/payload_generator/inplace_generator.h
index 4839824..48a1fac 100644
--- a/payload_generator/inplace_generator.h
+++ b/payload_generator/inplace_generator.h
@@ -212,6 +212,7 @@
// On success, stores the new operations in |aops| in the right order and
// returns true.
static bool ResolveReadAfterWriteDependencies(
+ const PartitionConfig& old_part,
const PartitionConfig& new_part,
uint64_t partition_size,
size_t block_size,
diff --git a/payload_generator/inplace_generator_unittest.cc b/payload_generator/inplace_generator_unittest.cc
index cde4bfc..20ac50b 100644
--- a/payload_generator/inplace_generator_unittest.cc
+++ b/payload_generator/inplace_generator_unittest.cc
@@ -33,6 +33,7 @@
#include "update_engine/common/utils.h"
#include "update_engine/payload_generator/cycle_breaker.h"
#include "update_engine/payload_generator/delta_diff_generator.h"
+#include "update_engine/payload_generator/delta_diff_utils.h"
#include "update_engine/payload_generator/extent_ranges.h"
#include "update_engine/payload_generator/graph_types.h"
#include "update_engine/payload_generator/graph_utils.h"
@@ -118,6 +119,16 @@
blob_file_.reset(new BlobFileWriter(blob_fd_, &blob_file_size_));
}
+ // Dump the list of operations |aops| in case of test failure.
+ void DumpAopsOnFailure(const vector<AnnotatedOperation>& aops) {
+ if (HasNonfatalFailure()) {
+ LOG(INFO) << "Result operation list:";
+ for (const auto& aop : aops) {
+ LOG(INFO) << aop;
+ }
+ }
+ }
+
// Blob file name, file descriptor and file size used to store operation
// blobs.
string blob_path_;
@@ -618,19 +629,21 @@
// space, forcing it to create a new full operation and the second case with
// one extra block in the partition that can be used for the move operation.
for (const auto part_blocks : vector<uint64_t>{num_blocks, num_blocks + 1}) {
- SCOPED_TRACE(base::StringPrintf("Using partition_blocs=%" PRIu64,
- part_blocks));
+ SCOPED_TRACE(
+ base::StringPrintf("Using partition_blocks=%" PRIu64, part_blocks));
vector<AnnotatedOperation> result_aops = aops;
EXPECT_TRUE(InplaceGenerator::ResolveReadAfterWriteDependencies(
- part, part_blocks * block_size, block_size, blob_file_.get(),
- &result_aops));
+ part,
+ part,
+ part_blocks * block_size,
+ block_size,
+ blob_file_.get(),
+ &result_aops));
size_t full_ops = 0;
for (const auto& aop : result_aops) {
- if (aop.op.type() == InstallOperation::REPLACE ||
- aop.op.type() == InstallOperation::REPLACE_BZ) {
+ if (diff_utils::IsAReplaceOperation(aop.op.type()))
full_ops++;
- }
if (aop.op.type() != InstallOperation::MOVE)
continue;
@@ -648,13 +661,90 @@
// operation for it.
EXPECT_EQ(part_blocks == num_blocks ? 2U : 1U, full_ops);
- if (HasNonfatalFailure()) {
- LOG(INFO) << "Result operation list:";
- for (const auto& aop : result_aops) {
- LOG(INFO) << aop;
+ DumpAopsOnFailure(result_aops);
+ }
+}
+
+// Test that we can shrink a filesystem and break cycles.
+TEST_F(InplaceGeneratorTest, ResolveReadAfterWriteDependenciesShrinkData) {
+ size_t block_size = 4096;
+ size_t old_blocks = 10;
+ size_t new_blocks = 8;
+ vector<AnnotatedOperation> aops;
+
+ // Create a loop using the blocks 1-6 and one other operation writing to the
+ // block 7 from outside the new partition. The loop in the blocks 1-6 uses
+ // two-block operations, so it needs two blocks of scratch space. It can't use
+ // the block 0 as scratch space (see previous test) and it can't use the
+ // blocks 7 or 8 due the last move operation.
+
+ aops.emplace_back();
+ aops.back().name = base::StringPrintf("<bz-block-0>");
+ aops.back().op.set_type(InstallOperation::REPLACE_BZ);
+ StoreExtents({ExtentForRange(0, 1)}, aops.back().op.mutable_dst_extents());
+
+ const size_t num_ops = 3;
+ for (size_t i = 0; i < num_ops; i++) {
+ AnnotatedOperation aop;
+ aop.name = base::StringPrintf("<op-%" PRIuS ">", i);
+ aop.op.set_type(InstallOperation::BSDIFF);
+ StoreExtents({ExtentForRange(1 + 2 * i, 2)}, aop.op.mutable_src_extents());
+ StoreExtents({ExtentForRange(1 + 2 * ((i + 1) % num_ops), 2)},
+ aop.op.mutable_dst_extents());
+ aops.push_back(aop);
+ }
+
+ {
+ AnnotatedOperation aop;
+ aop.name = "<op-shrink>";
+ aop.op.set_type(InstallOperation::BSDIFF);
+ StoreExtents({ExtentForRange(8, 1)}, aop.op.mutable_src_extents());
+ StoreExtents({ExtentForRange(7, 1)}, aop.op.mutable_dst_extents());
+ aops.push_back(aop);
+ }
+
+ PartitionConfig old_part("part");
+ old_part.path = "/dev/zero";
+ old_part.size = old_blocks * block_size;
+
+ PartitionConfig new_part("part");
+ new_part.path = "/dev/zero";
+ new_part.size = new_blocks * block_size;
+
+ CreateBlobFile();
+
+ EXPECT_TRUE(InplaceGenerator::ResolveReadAfterWriteDependencies(
+ old_part,
+ new_part,
+ (old_blocks + 2) * block_size, // enough scratch space.
+ block_size,
+ blob_file_.get(),
+ &aops));
+
+ size_t full_ops = 0;
+ for (const auto& aop : aops) {
+ if (diff_utils::IsAReplaceOperation(aop.op.type()))
+ full_ops++;
+ }
+ // There should be only one REPLACE* operation, the one we added for block 0.
+ EXPECT_EQ(1U, full_ops);
+
+ // There should be only one MOVE operation, the one used to break the loop
+ // which should write to scratch space past the block 7 (the last block of the
+ // new partition) which is being written later.
+ size_t move_ops = 0;
+ for (const auto& aop : aops) {
+ if (aop.op.type() == InstallOperation::MOVE) {
+ move_ops++;
+ for (const Extent& extent : aop.op.dst_extents()) {
+ EXPECT_LE(7U, extent.start_block()) << "On dst extents for aop: "
+ << aop;
}
}
}
+ EXPECT_EQ(1U, move_ops);
+
+ DumpAopsOnFailure(aops);
}
} // namespace chromeos_update_engine