From dccfdca1e19d27bac2c8f20df431fd36d0f87d53 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 10 Dec 2018 12:51:42 -0800 Subject: [PATCH] liblp: Reclaim wasted space from unaligned partitions. When allocating a partition with a size that is unaligned (to the optimal alignment), the remaining sectors are wasted since they are never reallocated. This is because the free list is guaranteed to only contain optimally-aligned regions. Unfortunately this means when a partition is resized, we are wasting a small amount of space each time. On a non-A/B device, this could wind up being significant. For example, with an alignment of 512KiB, a 4KiB partition at offset 0 will waste 508KiB of space. The next extent to be allocated by any partition will start at the next 512KiB. To address this, we round up extents to the optimal alignment. This means partitions may wind up slightly over-allocated, versus before, where they would waste space by making it unavailable. Bug: 120434950 Test: liblp_test gtest Change-Id: I555209b301058555526cc4309f7049ae81cf877d --- fs_mgr/liblp/builder.cpp | 13 +++++++-- fs_mgr/liblp/builder_test.cpp | 50 ++++++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/fs_mgr/liblp/builder.cpp b/fs_mgr/liblp/builder.cpp index 699b9e701..07f9d66d0 100644 --- a/fs_mgr/liblp/builder.cpp +++ b/fs_mgr/liblp/builder.cpp @@ -606,14 +606,23 @@ bool MetadataBuilder::GrowPartition(Partition* partition, uint64_t aligned_size) } uint64_t sectors = std::min(sectors_needed, region.length()); + if (sectors < region.length()) { + const auto& block_device = block_devices_[region.device_index]; + if (block_device.alignment) { + const uint64_t alignment = block_device.alignment / LP_SECTOR_SIZE; + sectors = AlignTo(sectors, alignment); + sectors = std::min(sectors, region.length()); + } + } CHECK(sectors % sectors_per_block == 0); auto extent = std::make_unique(sectors, region.device_index, region.start); new_extents.push_back(std::move(extent)); - sectors_needed -= sectors; - if (!sectors_needed) { + if (sectors >= sectors_needed) { + sectors_needed = 0; break; } + sectors_needed -= sectors; } if (sectors_needed) { LERROR << "Not enough free space to expand partition: " << partition->name(); diff --git a/fs_mgr/liblp/builder_test.cpp b/fs_mgr/liblp/builder_test.cpp index 7833a2529..37939646c 100644 --- a/fs_mgr/liblp/builder_test.cpp +++ b/fs_mgr/liblp/builder_test.cpp @@ -209,8 +209,8 @@ TEST_F(BuilderTest, InternalPartitionAlignment) { ASSERT_TRUE(builder->ResizePartition(a, a->size() + 4096)); ASSERT_TRUE(builder->ResizePartition(b, b->size() + 4096)); } - EXPECT_EQ(a->size(), 40960); - EXPECT_EQ(b->size(), 40960); + EXPECT_EQ(a->size(), 7864320); + EXPECT_EQ(b->size(), 7864320); unique_ptr exported = builder->Export(); ASSERT_NE(exported, nullptr); @@ -218,7 +218,7 @@ TEST_F(BuilderTest, InternalPartitionAlignment) { // Check that each starting sector is aligned. for (const auto& extent : exported->extents) { ASSERT_EQ(extent.target_type, LP_TARGET_TYPE_LINEAR); - EXPECT_EQ(extent.num_sectors, 8); + EXPECT_EQ(extent.num_sectors, 1536); uint64_t lba = extent.target_data * LP_SECTOR_SIZE; uint64_t aligned_lba = AlignTo(lba, device_info.alignment, device_info.alignment_offset); @@ -645,7 +645,7 @@ TEST_F(BuilderTest, MultipleBlockDevices) { EXPECT_EQ(metadata->extents[1].target_type, LP_TARGET_TYPE_LINEAR); EXPECT_EQ(metadata->extents[1].target_data, 1472); EXPECT_EQ(metadata->extents[1].target_source, 1); - EXPECT_EQ(metadata->extents[2].num_sectors, 129088); + EXPECT_EQ(metadata->extents[2].num_sectors, 129600); EXPECT_EQ(metadata->extents[2].target_type, LP_TARGET_TYPE_LINEAR); EXPECT_EQ(metadata->extents[2].target_data, 1472); EXPECT_EQ(metadata->extents[2].target_source, 2); @@ -744,17 +744,41 @@ TEST_F(BuilderTest, ABExtents) { EXPECT_EQ(system_a->extents().size(), static_cast(1)); EXPECT_EQ(system_b->extents().size(), static_cast(1)); ASSERT_TRUE(builder->ResizePartition(system_b, 6_GiB)); - EXPECT_EQ(system_b->extents().size(), static_cast(3)); + EXPECT_EQ(system_b->extents().size(), static_cast(2)); unique_ptr exported = builder->Export(); ASSERT_NE(exported, nullptr); - ASSERT_EQ(exported->extents.size(), static_cast(4)); + ASSERT_EQ(exported->extents.size(), static_cast(3)); EXPECT_EQ(exported->extents[0].target_data, 10487808); - EXPECT_EQ(exported->extents[0].num_sectors, 4194304); - EXPECT_EQ(exported->extents[1].target_data, 14682624); - EXPECT_EQ(exported->extents[1].num_sectors, 6288896); - EXPECT_EQ(exported->extents[2].target_data, 6292992); - EXPECT_EQ(exported->extents[2].num_sectors, 2099712); - EXPECT_EQ(exported->extents[3].target_data, 1536); - EXPECT_EQ(exported->extents[3].num_sectors, 6291456); + EXPECT_EQ(exported->extents[0].num_sectors, 10483712); + EXPECT_EQ(exported->extents[1].target_data, 6292992); + EXPECT_EQ(exported->extents[1].num_sectors, 2099712); + EXPECT_EQ(exported->extents[2].target_data, 1536); + EXPECT_EQ(exported->extents[2].num_sectors, 6291456); +} + +TEST_F(BuilderTest, PartialExtents) { + // super has a minimum extent size of 768KiB. + BlockDeviceInfo device_info("super", 1_GiB, 768 * 1024, 0, 4096); + auto builder = MetadataBuilder::New(device_info, 65536, 1); + ASSERT_NE(builder, nullptr); + Partition* system = builder->AddPartition("system", 0); + ASSERT_NE(system, nullptr); + Partition* vendor = builder->AddPartition("vendor", 0); + ASSERT_NE(vendor, nullptr); + ASSERT_TRUE(builder->ResizePartition(system, device_info.alignment + 4096)); + ASSERT_TRUE(builder->ResizePartition(vendor, device_info.alignment)); + ASSERT_EQ(system->size(), device_info.alignment * 2); + ASSERT_EQ(vendor->size(), device_info.alignment); + + ASSERT_TRUE(builder->ResizePartition(system, device_info.alignment * 2)); + ASSERT_EQ(system->extents().size(), static_cast(1)); + + unique_ptr exported = builder->Export(); + ASSERT_NE(exported, nullptr); + ASSERT_EQ(exported->extents.size(), static_cast(2)); + EXPECT_EQ(exported->extents[0].target_data, 1536); + EXPECT_EQ(exported->extents[0].num_sectors, 3072); + EXPECT_EQ(exported->extents[1].target_data, 4608); + EXPECT_EQ(exported->extents[1].num_sectors, 1536); }