From de1daa72aaa25de8b61f004457a60b8f4527dec6 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 31 Jul 2018 16:43:26 -0700 Subject: [PATCH 1/2] liblp: Simplify GrowPartition(). The partition resize algorithm duplicates a lot of logic because it handles the final free interval separately from other free intervals. This is unnecessary and makes it harder to change the actual algorithm. This change makes GrowPartition() treat the final free space region the same as free gaps in between partitions. It does this by converting the extent list into a gap list, and then adds a final gap for the remainder of the free space. The resize function no longer has to treat the end of the disk separately. This patch does not change the way partitions are allocated, it is purely a refactoring. Bug: 79173901 Test: liblp_test gtest Change-Id: I4780f20b23fe021eac62de874b061857712c04fe --- fs_mgr/liblp/builder.cpp | 88 ++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/fs_mgr/liblp/builder.cpp b/fs_mgr/liblp/builder.cpp index d6eee6b32..d3c785d33 100644 --- a/fs_mgr/liblp/builder.cpp +++ b/fs_mgr/liblp/builder.cpp @@ -299,49 +299,59 @@ bool MetadataBuilder::GrowPartition(Partition* partition, uint64_t aligned_size) Interval(uint64_t start, uint64_t end) : start(start), end(end) {} bool operator<(const Interval& other) const { return start < other.start; } }; - std::vector intervals; - // Collect all extents in the partition table. + // Collect all extents in the partition table, then sort them by starting + // sector. + std::vector extents; for (const auto& partition : partitions_) { for (const auto& extent : partition->extents()) { LinearExtent* linear = extent->AsLinearExtent(); if (!linear) { continue; } - intervals.emplace_back(linear->physical_sector(), - linear->physical_sector() + extent->num_sectors()); + extents.emplace_back(linear->physical_sector(), + linear->physical_sector() + extent->num_sectors()); } } + std::sort(extents.begin(), extents.end()); - // Sort extents by starting sector. - std::sort(intervals.begin(), intervals.end()); + // Convert the extent list into a list of gaps between the extents; i.e., + // the list of ranges that are free on the disk. + std::vector free_regions; + for (size_t i = 1; i < extents.size(); i++) { + const Interval& previous = extents[i - 1]; + const Interval& current = extents[i]; + + uint64_t aligned = AlignSector(previous.end); + if (aligned >= current.start) { + // There is no gap between these two extents, try the next one. + // Note that we check with >= instead of >, since alignment may + // bump the ending sector past the beginning of the next extent. + continue; + } + + // The new interval represents the free space starting at the end of + // the previous interval, and ending at the start of the next interval. + free_regions.emplace_back(aligned, current.start); + } + + // Add a final interval representing the remainder of the free space. + uint64_t last_free_extent_start = + extents.empty() ? geometry_.first_logical_sector : extents.back().end; + last_free_extent_start = AlignSector(last_free_extent_start); + if (last_free_extent_start <= geometry_.last_logical_sector) { + free_regions.emplace_back(last_free_extent_start, geometry_.last_logical_sector + 1); + } // Find gaps that we can use for new extents. Note we store new extents in a // temporary vector, and only commit them if we are guaranteed enough free // space. std::vector> new_extents; - for (size_t i = 1; i < intervals.size(); i++) { - const Interval& previous = intervals[i - 1]; - const Interval& current = intervals[i]; - - if (previous.end >= current.start) { - // There is no gap between these two extents, try the next one. Note that - // extents may never overlap, but just for safety, we ignore them if they - // do. - DCHECK(previous.end == current.start); - continue; - } - - uint64_t aligned = AlignSector(previous.end); - if (aligned >= current.start) { - // After alignment, this extent is not usable. - continue; - } - + for (const auto& region : free_regions) { // This gap is enough to hold the remainder of the space requested, so we // can allocate what we need and return. - if (current.start - aligned >= sectors_needed) { - auto extent = std::make_unique(sectors_needed, aligned); + if (region.end - region.start >= sectors_needed) { + auto extent = std::make_unique(sectors_needed, region.start); sectors_needed -= extent->num_sectors(); new_extents.push_back(std::move(extent)); break; @@ -349,33 +359,13 @@ bool MetadataBuilder::GrowPartition(Partition* partition, uint64_t aligned_size) // This gap is not big enough to fit the remainder of the space requested, // so consume the whole thing and keep looking for more. - auto extent = std::make_unique(current.start - aligned, aligned); + auto extent = std::make_unique(region.end - region.start, region.start); sectors_needed -= extent->num_sectors(); new_extents.push_back(std::move(extent)); } - - // If we still have more to allocate, take it from the remaining free space - // in the allocatable region. if (sectors_needed) { - uint64_t first_sector; - if (intervals.empty()) { - first_sector = geometry_.first_logical_sector; - } else { - first_sector = intervals.back().end; - } - DCHECK(first_sector <= geometry_.last_logical_sector); - - // Note: After alignment, |first_sector| may be > the last usable sector. - first_sector = AlignSector(first_sector); - - // Note: the last usable sector is inclusive. - if (first_sector > geometry_.last_logical_sector || - geometry_.last_logical_sector + 1 - first_sector < sectors_needed) { - LERROR << "Not enough free space to expand partition: " << partition->name(); - return false; - } - auto extent = std::make_unique(sectors_needed, first_sector); - new_extents.push_back(std::move(extent)); + LERROR << "Not enough free space to expand partition: " << partition->name(); + return false; } for (auto& extent : new_extents) { From 4d9c7459c4fa7baaa61fb3cb930039096f844fe3 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 1 Aug 2018 14:14:37 -0700 Subject: [PATCH 2/2] liblp: Require block-aligned partition sizes. DM_TABLE_LOAD will reject dm-linear entries if their size is not a multiple of the backing device's logical block size. For example, a partition of 10GiB+512 bytes will fail to map in device-mapper if the logical block size is 4096 bytes. To address this, this patch adds a few changes to liblp: The block size given to lpmake is now recorded in LpGeometryMetadata. The block size must be a multiple of the sector size. In addition, partiton sizes are now aligned to the block size, and the super partition must have enough free space to allocate at least one block (in addition to storing metadata). GrowPartition now has multiple checks that the block-size invariant is not violated, to ensure that no invalid partition tables will be created. Bug: 79173901 Test: liblp_test gtest Change-Id: I484aac1f9b90ebd92dc1c89ce1e09cd89bbb441e --- fs_mgr/liblp/builder.cpp | 78 ++++++++++++++++---- fs_mgr/liblp/builder_test.cpp | 50 ++++++++++--- fs_mgr/liblp/include/liblp/builder.h | 23 ++++-- fs_mgr/liblp/include/liblp/metadata_format.h | 6 ++ fs_mgr/liblp/utility_test.cpp | 3 +- 5 files changed, 125 insertions(+), 35 deletions(-) diff --git a/fs_mgr/liblp/builder.cpp b/fs_mgr/liblp/builder.cpp index d3c785d33..eb429b930 100644 --- a/fs_mgr/liblp/builder.cpp +++ b/fs_mgr/liblp/builder.cpp @@ -48,10 +48,20 @@ bool GetBlockDeviceInfo(const std::string& block_device, BlockDeviceInfo* device PERROR << __PRETTY_FUNCTION__ << "BLKIOMIN failed"; return false; } - if (ioctl(fd, BLKALIGNOFF, &device_info->alignment_offset) < 0) { + + int alignment_offset; + if (ioctl(fd, BLKALIGNOFF, &alignment_offset) < 0) { PERROR << __PRETTY_FUNCTION__ << "BLKIOMIN failed"; return false; } + int logical_block_size; + if (ioctl(fd, BLKSSZGET, &logical_block_size) < 0) { + PERROR << __PRETTY_FUNCTION__ << "BLKSSZGET failed"; + return false; + } + + device_info->alignment_offset = static_cast(alignment_offset); + device_info->logical_block_size = static_cast(logical_block_size); return true; #else (void)block_device; @@ -178,6 +188,7 @@ bool MetadataBuilder::Init(const LpMetadata& metadata) { device_info_.alignment = geometry_.alignment; device_info_.alignment_offset = geometry_.alignment_offset; + device_info_.logical_block_size = geometry_.logical_block_size; return true; } @@ -201,6 +212,10 @@ bool MetadataBuilder::Init(const BlockDeviceInfo& device_info, uint32_t metadata LERROR << "Block device size must be a multiple of 512."; return false; } + if (device_info_.logical_block_size % LP_SECTOR_SIZE != 0) { + LERROR << "Logical block size must be a multiple of 512."; + return false; + } if (device_info_.alignment_offset % LP_SECTOR_SIZE != 0) { LERROR << "Alignment offset is not sector-aligned."; return false; @@ -244,6 +259,18 @@ bool MetadataBuilder::Init(const BlockDeviceInfo& device_info, uint32_t metadata return false; } + // Finally, the size of the allocatable space must be a multiple of the + // logical block size. If we have no more free space after this + // computation, then we abort. Note that the last sector is inclusive, + // so we have to account for that. + uint64_t num_free_sectors = last_sector - first_sector + 1; + uint64_t sectors_per_block = device_info_.logical_block_size / LP_SECTOR_SIZE; + if (num_free_sectors < sectors_per_block) { + LERROR << "Not enough space to allocate any partition tables."; + return false; + } + last_sector = first_sector + (num_free_sectors / sectors_per_block) * sectors_per_block - 1; + geometry_.first_logical_sector = first_sector; geometry_.last_logical_sector = last_sector; geometry_.metadata_max_size = metadata_max_size; @@ -251,6 +278,7 @@ bool MetadataBuilder::Init(const BlockDeviceInfo& device_info, uint32_t metadata geometry_.alignment = device_info_.alignment; geometry_.alignment_offset = device_info_.alignment_offset; geometry_.block_device_size = device_info_.size; + geometry_.logical_block_size = device_info.logical_block_size; return true; } @@ -297,6 +325,7 @@ bool MetadataBuilder::GrowPartition(Partition* partition, uint64_t aligned_size) uint64_t end; Interval(uint64_t start, uint64_t end) : start(start), end(end) {} + uint64_t length() const { return end - start; } bool operator<(const Interval& other) const { return start < other.start; } }; @@ -343,31 +372,46 @@ bool MetadataBuilder::GrowPartition(Partition* partition, uint64_t aligned_size) free_regions.emplace_back(last_free_extent_start, geometry_.last_logical_sector + 1); } + const uint64_t sectors_per_block = device_info_.logical_block_size / LP_SECTOR_SIZE; + CHECK(sectors_needed % sectors_per_block == 0); + // Find gaps that we can use for new extents. Note we store new extents in a // temporary vector, and only commit them if we are guaranteed enough free // space. std::vector> new_extents; - for (const auto& region : free_regions) { - // This gap is enough to hold the remainder of the space requested, so we - // can allocate what we need and return. - if (region.end - region.start >= sectors_needed) { - auto extent = std::make_unique(sectors_needed, region.start); - sectors_needed -= extent->num_sectors(); - new_extents.push_back(std::move(extent)); - break; + for (auto& region : free_regions) { + if (region.length() % sectors_per_block != 0) { + // This should never happen, because it would imply that we + // once allocated an extent that was not a multiple of the + // block size. That extent would be rejected by DM_TABLE_LOAD. + LERROR << "Region " << region.start << ".." << region.end + << " is not a multiple of the block size, " << sectors_per_block; + + // If for some reason the final region is mis-sized we still want + // to be able to grow partitions. So just to be safe, round the + // region down to the nearest block. + region.end = region.start + (region.length() / sectors_per_block) * sectors_per_block; + if (!region.length()) { + continue; + } } - // This gap is not big enough to fit the remainder of the space requested, - // so consume the whole thing and keep looking for more. - auto extent = std::make_unique(region.end - region.start, region.start); - sectors_needed -= extent->num_sectors(); + uint64_t sectors = std::min(sectors_needed, region.length()); + CHECK(sectors % sectors_per_block == 0); + + auto extent = std::make_unique(sectors, region.start); new_extents.push_back(std::move(extent)); + sectors_needed -= sectors; + if (!sectors_needed) { + break; + } } if (sectors_needed) { LERROR << "Not enough free space to expand partition: " << partition->name(); return false; } + // Everything succeeded, so commit the new extents. for (auto& extent : new_extents) { partition->AddExtent(std::move(extent)); } @@ -435,6 +479,12 @@ uint64_t MetadataBuilder::AlignSector(uint64_t sector) { void MetadataBuilder::set_block_device_info(const BlockDeviceInfo& device_info) { device_info_.size = device_info.size; + // Note that if the logical block size changes, we're probably in trouble: + // we could have already built extents that will only work on the previous + // size. + DCHECK(partitions_.empty() || + device_info_.logical_block_size == device_info.logical_block_size); + // The kernel does not guarantee these values are present, so we only // replace existing values if the new values are non-zero. if (device_info.alignment) { @@ -447,7 +497,7 @@ void MetadataBuilder::set_block_device_info(const BlockDeviceInfo& device_info) bool MetadataBuilder::ResizePartition(Partition* partition, uint64_t requested_size) { // Align the space needed up to the nearest sector. - uint64_t aligned_size = AlignTo(requested_size, LP_SECTOR_SIZE); + uint64_t aligned_size = AlignTo(requested_size, device_info_.logical_block_size); if (aligned_size > partition->size()) { return GrowPartition(partition, aligned_size); diff --git a/fs_mgr/liblp/builder_test.cpp b/fs_mgr/liblp/builder_test.cpp index 4334d51a5..f1a91c479 100644 --- a/fs_mgr/liblp/builder_test.cpp +++ b/fs_mgr/liblp/builder_test.cpp @@ -92,11 +92,11 @@ TEST(liblp, PartitionAlignment) { Partition* system = builder->AddPartition("system", TEST_GUID, LP_PARTITION_ATTR_READONLY); ASSERT_NE(system, nullptr); EXPECT_EQ(builder->ResizePartition(system, 10000), true); - EXPECT_EQ(system->size(), 10240); + EXPECT_EQ(system->size(), 12288); EXPECT_EQ(system->extents().size(), 1); - builder->ResizePartition(system, 9000); - EXPECT_EQ(system->size(), 9216); + builder->ResizePartition(system, 7000); + EXPECT_EQ(system->size(), 8192); EXPECT_EQ(system->extents().size(), 1); } @@ -120,13 +120,13 @@ TEST(liblp, MetadataAlignment) { TEST(liblp, InternalAlignment) { // Test the metadata fitting within alignment. - BlockDeviceInfo device_info(1024 * 1024, 768 * 1024, 0); + BlockDeviceInfo device_info(1024 * 1024, 768 * 1024, 0, 4096); unique_ptr builder = MetadataBuilder::New(device_info, 1024, 2); ASSERT_NE(builder, nullptr); unique_ptr exported = builder->Export(); ASSERT_NE(exported, nullptr); EXPECT_EQ(exported->geometry.first_logical_sector, 1536); - EXPECT_EQ(exported->geometry.last_logical_sector, 2035); + EXPECT_EQ(exported->geometry.last_logical_sector, 2031); // Test a large alignment offset thrown in. device_info.alignment_offset = 753664; @@ -135,7 +135,7 @@ TEST(liblp, InternalAlignment) { exported = builder->Export(); ASSERT_NE(exported, nullptr); EXPECT_EQ(exported->geometry.first_logical_sector, 1472); - EXPECT_EQ(exported->geometry.last_logical_sector, 2035); + EXPECT_EQ(exported->geometry.last_logical_sector, 2031); // Alignment offset without alignment doesn't mean anything. device_info.alignment = 0; @@ -150,7 +150,7 @@ TEST(liblp, InternalAlignment) { exported = builder->Export(); ASSERT_NE(exported, nullptr); EXPECT_EQ(exported->geometry.first_logical_sector, 78); - EXPECT_EQ(exported->geometry.last_logical_sector, 1975); + EXPECT_EQ(exported->geometry.last_logical_sector, 1973); // Test a small alignment with no alignment offset. device_info.alignment = 11 * 1024; @@ -163,7 +163,7 @@ TEST(liblp, InternalAlignment) { } TEST(liblp, InternalPartitionAlignment) { - BlockDeviceInfo device_info(512 * 1024 * 1024, 768 * 1024, 753664); + BlockDeviceInfo device_info(512 * 1024 * 1024, 768 * 1024, 753664, 4096); unique_ptr builder = MetadataBuilder::New(device_info, 32 * 1024, 2); Partition* a = builder->AddPartition("a", TEST_GUID, 0); @@ -381,7 +381,7 @@ TEST(liblp, MetadataTooLarge) { static const size_t kMetadataSize = 64 * 1024; // No space to store metadata + geometry. - BlockDeviceInfo device_info(kDiskSize, 0, 0); + BlockDeviceInfo device_info(kDiskSize, 0, 0, 4096); unique_ptr builder = MetadataBuilder::New(device_info, kMetadataSize, 1); EXPECT_EQ(builder, nullptr); @@ -390,8 +390,8 @@ TEST(liblp, MetadataTooLarge) { builder = MetadataBuilder::New(device_info, kMetadataSize, 1); EXPECT_EQ(builder, nullptr); - // Space for metadata + geometry + one free sector. - device_info.size += LP_SECTOR_SIZE; + // Space for metadata + geometry + one free block. + device_info.size += device_info.logical_block_size; builder = MetadataBuilder::New(device_info, kMetadataSize, 1); EXPECT_NE(builder, nullptr); @@ -424,19 +424,21 @@ TEST(liblp, block_device_info) { ASSERT_EQ(device_info.alignment % LP_SECTOR_SIZE, 0); ASSERT_EQ(device_info.alignment_offset % LP_SECTOR_SIZE, 0); ASSERT_LE(device_info.alignment_offset, INT_MAX); + ASSERT_EQ(device_info.logical_block_size % LP_SECTOR_SIZE, 0); // Having an alignment offset > alignment doesn't really make sense. ASSERT_LT(device_info.alignment_offset, device_info.alignment); } TEST(liblp, UpdateBlockDeviceInfo) { - BlockDeviceInfo device_info(1024 * 1024, 4096, 1024); + BlockDeviceInfo device_info(1024 * 1024, 4096, 1024, 4096); unique_ptr builder = MetadataBuilder::New(device_info, 1024, 1); ASSERT_NE(builder, nullptr); EXPECT_EQ(builder->block_device_info().size, device_info.size); EXPECT_EQ(builder->block_device_info().alignment, device_info.alignment); EXPECT_EQ(builder->block_device_info().alignment_offset, device_info.alignment_offset); + EXPECT_EQ(builder->block_device_info().logical_block_size, device_info.logical_block_size); device_info.alignment = 0; device_info.alignment_offset = 2048; @@ -450,3 +452,27 @@ TEST(liblp, UpdateBlockDeviceInfo) { EXPECT_EQ(builder->block_device_info().alignment, 8192); EXPECT_EQ(builder->block_device_info().alignment_offset, 2048); } + +TEST(liblp, InvalidBlockSize) { + BlockDeviceInfo device_info(1024 * 1024, 0, 0, 513); + unique_ptr builder = MetadataBuilder::New(device_info, 1024, 1); + EXPECT_EQ(builder, nullptr); +} + +TEST(liblp, AlignedExtentSize) { + BlockDeviceInfo device_info(1024 * 1024, 0, 0, 4096); + unique_ptr builder = MetadataBuilder::New(device_info, 1024, 1); + ASSERT_NE(builder, nullptr); + + Partition* partition = builder->AddPartition("system", TEST_GUID, 0); + ASSERT_NE(partition, nullptr); + ASSERT_TRUE(builder->ResizePartition(partition, 512)); + EXPECT_EQ(partition->size(), 4096); +} + +TEST(liblp, AlignedFreeSpace) { + // Only one sector free - at least one block is required. + BlockDeviceInfo device_info(10240, 0, 0, 4096); + unique_ptr builder = MetadataBuilder::New(device_info, 512, 1); + ASSERT_EQ(builder, nullptr); +} diff --git a/fs_mgr/liblp/include/liblp/builder.h b/fs_mgr/liblp/include/liblp/builder.h index 0f96e3a60..e83b92297 100644 --- a/fs_mgr/liblp/include/liblp/builder.h +++ b/fs_mgr/liblp/include/liblp/builder.h @@ -32,11 +32,16 @@ class LinearExtent; // By default, partitions are aligned on a 1MiB boundary. static const uint32_t kDefaultPartitionAlignment = 1024 * 1024; +static const uint32_t kDefaultBlockSize = 4096; struct BlockDeviceInfo { - BlockDeviceInfo() : size(0), alignment(0), alignment_offset(0) {} - BlockDeviceInfo(uint64_t size, uint32_t alignment, uint32_t alignment_offset) - : size(size), alignment(alignment), alignment_offset(alignment_offset) {} + BlockDeviceInfo() : size(0), alignment(0), alignment_offset(0), logical_block_size(0) {} + BlockDeviceInfo(uint64_t size, uint32_t alignment, uint32_t alignment_offset, + uint32_t logical_block_size) + : size(size), + alignment(alignment), + alignment_offset(alignment_offset), + logical_block_size(logical_block_size) {} // Size of the block device, in bytes. uint64_t size; // Optimal target alignment, in bytes. Partition extents will be aligned to @@ -46,6 +51,8 @@ struct BlockDeviceInfo { // |alignment_offset| on the target device is correctly aligned on its // parent device. This value must be 0 or a multiple of 512. uint32_t alignment_offset; + // Block size, for aligning extent sizes and partition sizes. + uint32_t logical_block_size; }; // Abstraction around dm-targets that can be encoded into logical partition tables. @@ -88,6 +95,8 @@ class ZeroExtent final : public Extent { }; class Partition final { + friend class MetadataBuilder; + public: Partition(const std::string& name, const std::string& guid, uint32_t attributes); @@ -97,10 +106,6 @@ class Partition final { // Remove all extents from this partition. void RemoveExtents(); - // Remove and/or shrink extents until the partition is the requested size. - // See MetadataBuilder::ShrinkPartition for more information. - void ShrinkTo(uint64_t requested_size); - const std::string& name() const { return name_; } uint32_t attributes() const { return attributes_; } const std::string& guid() const { return guid_; } @@ -108,6 +113,8 @@ class Partition final { uint64_t size() const { return size_; } private: + void ShrinkTo(uint64_t aligned_size); + std::string name_; std::string guid_; std::vector> extents_; @@ -144,7 +151,7 @@ class MetadataBuilder { // size. This is a convenience method for tests. static std::unique_ptr New(uint64_t blockdev_size, uint32_t metadata_max_size, uint32_t metadata_slot_count) { - BlockDeviceInfo device_info(blockdev_size, 0, 0); + BlockDeviceInfo device_info(blockdev_size, 0, 0, kDefaultBlockSize); return New(device_info, metadata_max_size, metadata_slot_count); } diff --git a/fs_mgr/liblp/include/liblp/metadata_format.h b/fs_mgr/liblp/include/liblp/metadata_format.h index e1323e11e..52c80f7f4 100644 --- a/fs_mgr/liblp/include/liblp/metadata_format.h +++ b/fs_mgr/liblp/include/liblp/metadata_format.h @@ -136,6 +136,12 @@ typedef struct LpMetadataGeometry { * can be used to verify the geometry against a target device. */ uint64_t block_device_size; + + /* 76: Logical block size of the super partition block device. This is the + * minimal alignment for partition and extent sizes, and it must be a + * multiple of LP_SECTOR_SIZE. + */ + uint32_t logical_block_size; } __attribute__((packed)) LpMetadataGeometry; /* The logical partition metadata has a number of tables; they are described diff --git a/fs_mgr/liblp/utility_test.cpp b/fs_mgr/liblp/utility_test.cpp index 092dbf11c..7bf42aed9 100644 --- a/fs_mgr/liblp/utility_test.cpp +++ b/fs_mgr/liblp/utility_test.cpp @@ -40,7 +40,8 @@ TEST(liblp, GetMetadataOffset) { 80000, 0, 0, - 1024 * 1024}; + 1024 * 1024, + 4096}; EXPECT_EQ(GetPrimaryMetadataOffset(geometry, 0), 4096); EXPECT_EQ(GetPrimaryMetadataOffset(geometry, 1), 4096 + 16384); EXPECT_EQ(GetPrimaryMetadataOffset(geometry, 2), 4096 + 16384 * 2);