Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1145c01e authored by David Anderson's avatar David Anderson
Browse files

libsnapshot: Improve how devices are collapsed after merging. 

Currently, we replace snapshot-merge with a linear device wrapping the
base device. This is not efficient. This patch reads LpMetadata for the
underlying partition, and duplicates its table into the snapshot-merge
device. This removes a layer of stacking and also allows removing the
base device.

Note that snapshot_test is growing a bit unwiedly, because it's starting
to implement pieces of libsnapshot that will be filled in later for
update_engine. (MapUpdatePartitions is a good example of this.) When
those pieces land in libsnapshot, snapshot_test will be cleaned up to
remove much of this manual fiddling.

Bug: 139090440
Test: libsnapshot_test gtest
Change-Id: I3872dc51d9e5980803303806f42a5c7e74b0b78a
parent ad970fc0

fs_mgr/fs_mgr_dm_linear.cpp

+3 −3
Original line number Diff line number Diff line
@@ -79,7 +79,7 @@ static bool GetPhysicalPartitionDevicePath(const IPartitionOpener& opener, 
    return true;

}



static bool CreateDmTable(const IPartitionOpener& opener, const LpMetadata& metadata,

bool CreateDmTable(const IPartitionOpener& opener, const LpMetadata& metadata,

                   const LpMetadataPartition& partition, const std::string& super_device,

                   DmTable* table) {

    uint64_t sector = 0;

fs_mgr/include/fs_mgr_dm_linear.h

+5 −0
Original line number Diff line number Diff line
@@ -89,6 +89,11 @@ bool CreateLogicalPartition(const CreateLogicalPartitionParams& params, std::str 
// is non-zero, then this will block until the device path has been unlinked.

bool DestroyLogicalPartition(const std::string& name);



// Helper for populating a DmTable for a logical partition.

bool CreateDmTable(const IPartitionOpener& opener, const LpMetadata& metadata,

                   const LpMetadataPartition& partition, const std::string& super_device,

                   android::dm::DmTable* table);



}  // namespace fs_mgr

}  // namespace android

fs_mgr/libsnapshot/snapshot.cpp

+41 −27
Original line number Diff line number Diff line
@@ -47,6 +47,7 @@ using android::dm::DmTargetSnapshot; 
using android::dm::kSectorSize;

using android::dm::SnapshotStorageMode;

using android::fiemap::IImageManager;

using android::fs_mgr::CreateDmTable;

using android::fs_mgr::CreateLogicalPartition;

using android::fs_mgr::CreateLogicalPartitionParams;

using android::fs_mgr::GetPartitionName;
@@ -851,25 +852,16 @@ bool SnapshotManager::OnSnapshotMergeComplete(LockedFile* lock, const std::strin 


bool SnapshotManager::CollapseSnapshotDevice(const std::string& name,

                                             const SnapshotStatus& status) {

    // Ideally, we would complete the following steps to collapse the device:

    //  (1) Rewrite the snapshot table to be identical to the base device table.

    //  (2) Rewrite the verity table to use the "snapshot" (now linear) device.

    //  (3) Delete the base device.

    //

    // This should be possible once libsnapshot understands LpMetadata. In the

    // meantime, we implement a simpler solution: rewriting the snapshot table

    // to be a single dm-linear segment against the base device. While not as

    // ideal, it still lets us remove the COW device. We can remove this

    // implementation once the new method has been tested.

    auto& dm = DeviceMapper::Instance();

    auto dm_name = GetSnapshotDeviceName(name, status);



    // Verify we have a snapshot-merge device.

    DeviceMapper::TargetInfo target;

    if (!GetSingleTarget(dm_name, TableQuery::Table, &target)) {

        return false;

    }

    if (DeviceMapper::GetTargetType(target.spec) != "snapshot-merge") {

        // This should be impossible, it was checked above.

        // This should be impossible, it was checked earlier.

        LOG(ERROR) << "Snapshot device has invalid target type: " << dm_name;

        return false;

    }
@@ -898,7 +890,7 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, 
            return false;

        }

        if (outer_table.size() != 2) {

            LOG(ERROR) << "Expected 2 dm-linear targets for tabble " << name

            LOG(ERROR) << "Expected 2 dm-linear targets for table " << name

                       << ", got: " << outer_table.size();

            return false;

        }
@@ -918,27 +910,49 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, 
        }

    }



    // Note: we are replacing the OUTER table here, so we do not use dm_name.

    DmTargetLinear new_target(0, num_sectors, base_device, 0);

    LOG(INFO) << "Replacing snapshot device " << name

              << " table with: " << new_target.GetParameterString();

    // Grab the partition metadata for the snapshot.

    uint32_t slot = SlotNumberForSlotSuffix(device_->GetSlotSuffix());

    auto super_device = device_->GetSuperDevice(slot);

    const auto& opener = device_->GetPartitionOpener();

    auto metadata = android::fs_mgr::ReadMetadata(opener, super_device, slot);

    if (!metadata) {

        LOG(ERROR) << "Could not read super partition metadata.";

        return false;

    }

    auto partition = android::fs_mgr::FindPartition(*metadata.get(), name);

    if (!partition) {

        LOG(ERROR) << "Snapshot does not have a partition in super: " << name;

        return false;

    }



    // Create a DmTable that is identical to the base device.

    DmTable table;

    table.Emplace<DmTargetLinear>(new_target);

    if (!CreateDmTable(opener, *metadata.get(), *partition, super_device, &table)) {

        LOG(ERROR) << "Could not create a DmTable for partition: " << name;

        return false;

    }



    // Note: we are replacing the *outer* table here, so we do not use dm_name.

    if (!dm.LoadTableAndActivate(name, table)) {

        return false;

    }



    if (dm_name != name) {

        // Attempt to delete the snapshot device. Nothing should be depending on

        // the device, and device-mapper should have flushed remaining I/O. We

        // could in theory replace with dm-zero (or re-use the table above), but

        // for now it's better to know why this would fail.

    // Attempt to delete the snapshot device if one still exists. Nothing

    // should be depending on the device, and device-mapper should have

    // flushed remaining I/O. We could in theory replace with dm-zero (or

    // re-use the table above), but for now it's better to know why this

    // would fail.

    if (!dm.DeleteDeviceIfExists(dm_name)) {

        LOG(ERROR) << "Unable to delete snapshot device " << dm_name << ", COW cannot be "

                   << "reclaimed until after reboot.";

        return false;

    }



    // Cleanup the base device as well, since it is no longer used. This does

    // not block cleanup.

    auto base_name = GetBaseDeviceName(name);

    if (!dm.DeleteDeviceIfExists(base_name)) {

        LOG(ERROR) << "Unable to delete base device for snapshot: " << base_name;

    }

    return true;

}

fs_mgr/libsnapshot/snapshot_test.cpp

+23 −14
Original line number Diff line number Diff line
@@ -45,6 +45,7 @@ using android::fiemap::IImageManager; 
using android::fs_mgr::BlockDeviceInfo;

using android::fs_mgr::CreateLogicalPartitionParams;

using android::fs_mgr::DestroyLogicalPartition;

using android::fs_mgr::GetPartitionName;

using android::fs_mgr::MetadataBuilder;

using namespace ::testing;

using namespace android::fs_mgr::testing;
@@ -198,6 +199,7 @@ class SnapshotTest : public ::testing::Test { 
                    .block_device = fake_super,

                    .metadata = metadata.get(),

                    .partition = &partition,

                    .device_name = GetPartitionName(partition) + "-base",

                    .force_writable = true,

                    .timeout_ms = 10s,

            };
@@ -308,15 +310,20 @@ TEST_F(SnapshotTest, FirstStageMountAfterRollback) { 
}



TEST_F(SnapshotTest, Merge) {

    ON_CALL(*GetMockedPropertyFetcher(), GetBoolProperty("ro.virtual_ab.enabled", _))

            .WillByDefault(Return(true));



    ASSERT_TRUE(AcquireLock());



    static const uint64_t kDeviceSize = 1024 * 1024;

    ASSERT_TRUE(sm->CreateSnapshot(lock_.get(), "test-snapshot", kDeviceSize, kDeviceSize,

                                   kDeviceSize));



    std::string base_device, snap_device;

    ASSERT_TRUE(CreatePartition("base-device", kDeviceSize, &base_device));

    ASSERT_TRUE(sm->MapSnapshot(lock_.get(), "test-snapshot", base_device, 10s, &snap_device));

    ASSERT_TRUE(CreatePartition("test_partition_a", kDeviceSize));

    ASSERT_TRUE(MapUpdatePartitions());

    ASSERT_TRUE(dm_.GetDmDevicePathByName("test_partition_b-base", &base_device));

    ASSERT_TRUE(sm->CreateSnapshot(lock_.get(), "test_partition_b", kDeviceSize, kDeviceSize,

                                   kDeviceSize));

    ASSERT_TRUE(sm->MapSnapshot(lock_.get(), "test_partition_b", base_device, 10s, &snap_device));



    std::string test_string = "This is a test string.";

    {
@@ -325,10 +332,10 @@ TEST_F(SnapshotTest, Merge) { 
        ASSERT_TRUE(android::base::WriteFully(fd, test_string.data(), test_string.size()));

    }



    // Note: we know the name of the device is test-snapshot because we didn't

    // request a linear segment.

    // Note: we know there is no inner/outer dm device since we didn't request

    // a linear segment.

    DeviceMapper::TargetInfo target;

    ASSERT_TRUE(sm->IsSnapshotDevice("test-snapshot", &target));

    ASSERT_TRUE(sm->IsSnapshotDevice("test_partition_b", &target));

    ASSERT_EQ(DeviceMapper::GetTargetType(target.spec), "snapshot");



    // Release the lock.
@@ -341,7 +348,7 @@ TEST_F(SnapshotTest, Merge) { 
    ASSERT_TRUE(sm->InitiateMerge());



    // The device should have been switched to a snapshot-merge target.

    ASSERT_TRUE(sm->IsSnapshotDevice("test-snapshot", &target));

    ASSERT_TRUE(sm->IsSnapshotDevice("test_partition_b", &target));

    ASSERT_EQ(DeviceMapper::GetTargetType(target.spec), "snapshot-merge");



    // We should not be able to cancel an update now.
@@ -351,10 +358,12 @@ TEST_F(SnapshotTest, Merge) { 
    ASSERT_EQ(sm->GetUpdateState(), UpdateState::None);



    // The device should no longer be a snapshot or snapshot-merge.

    ASSERT_FALSE(sm->IsSnapshotDevice("test-snapshot"));

    ASSERT_FALSE(sm->IsSnapshotDevice("test_partition_b"));



    // Test that we can read back the string we wrote to the snapshot.

    unique_fd fd(open(base_device.c_str(), O_RDONLY | O_CLOEXEC));

    // Test that we can read back the string we wrote to the snapshot. Note

    // that the base device is gone now. |snap_device| contains the correct

    // partition.

    unique_fd fd(open(snap_device.c_str(), O_RDONLY | O_CLOEXEC));

    ASSERT_GE(fd, 0);



    std::string buffer(test_string.size(), '\0');
@@ -420,7 +429,7 @@ TEST_F(SnapshotTest, FirstStageMountAndMerge) { 
    // Simulate a reboot into the new slot.

    lock_ = nullptr;

    ASSERT_TRUE(sm->FinishedSnapshotWrites());

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b"));

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b-base"));



    auto rebooted = new TestDeviceInfo(fake_super);

    rebooted->set_slot_suffix("_b");
@@ -459,7 +468,7 @@ TEST_F(SnapshotTest, FlashSuperDuringUpdate) { 
    // Simulate a reboot into the new slot.

    lock_ = nullptr;

    ASSERT_TRUE(sm->FinishedSnapshotWrites());

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b"));

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b-base"));



    // Reflash the super partition.

    FormatFakeSuper();
@@ -504,7 +513,7 @@ TEST_F(SnapshotTest, FlashSuperDuringMerge) { 
    // Simulate a reboot into the new slot.

    lock_ = nullptr;

    ASSERT_TRUE(sm->FinishedSnapshotWrites());

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b"));

    ASSERT_TRUE(DestroyLogicalPartition("test_partition_b-base"));



    auto rebooted = new TestDeviceInfo(fake_super);

    rebooted->set_slot_suffix("_b");