From aefb3b17fb3de87c859101cb551628f6d9fd8e63 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 6 Aug 2019 14:55:54 -0700 Subject: [PATCH] libsnapshot: Don't force-unmap in DeleteSnapshot(). It is hard to re-use this function in the merge code when it forcefully unmaps the snapshot, because the snapshot may have been rewritten to be a dm-linear device. Instead, leave the decision up to the caller. Bug: 136678799 Test: libsnapshot_test gtest Change-Id: I03c027c0781696885a5a5654d3049287cc16ecd0 --- fs_mgr/libsnapshot/include/libsnapshot/snapshot.h | 5 ++--- fs_mgr/libsnapshot/snapshot.cpp | 5 ----- fs_mgr/libsnapshot/snapshot_test.cpp | 3 ++- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index ce979cf20..062e00b67 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -170,9 +170,8 @@ class SnapshotManager final { bool MapSnapshot(LockedFile* lock, const std::string& name, const std::string& base_device, const std::chrono::milliseconds& timeout_ms, std::string* dev_path); - // Remove the backing copy-on-write image for the named snapshot. If the - // device is still mapped, this will attempt an Unmap, and fail if the - // unmap fails. + // Remove the backing copy-on-write image for the named snapshot. The + // caller is responsible for ensuring that the snapshot is unmapped. bool DeleteSnapshot(LockedFile* lock, const std::string& name); // Unmap a snapshot device previously mapped with MapSnapshotDevice(). diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index c982dd10a..ef561797f 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -300,11 +300,6 @@ bool SnapshotManager::DeleteSnapshot(LockedFile* lock, const std::string& name) CHECK(lock); if (!EnsureImageManager()) return false; - if (!UnmapSnapshot(lock, name)) { - LOG(ERROR) << "Snapshot could not be unmapped for deletion: " << name; - return false; - } - // Take the snapshot's lock after Unmap, since it will also try to lock. auto status_file = OpenSnapshotStatusFile(name, O_RDONLY, LOCK_EX); if (!status_file) return false; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 1b3289b4d..9cc9bd7d1 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -104,7 +104,7 @@ TEST_F(SnapshotTest, CreateSnapshot) { ASSERT_EQ(snapshots.size(), 1); ASSERT_EQ(snapshots[0], "test-snapshot"); - // Scope so delete can re-acquire the status file lock. + // Scope so delete can re-acquire the snapshot file lock. { auto file = sm->OpenSnapshotStatusFile("test-snapshot", O_RDONLY, LOCK_SH); ASSERT_NE(file, nullptr); @@ -116,6 +116,7 @@ TEST_F(SnapshotTest, CreateSnapshot) { ASSERT_EQ(status.snapshot_size, kDeviceSize); } + ASSERT_TRUE(sm->UnmapSnapshot(lock_.get(), "test-snapshot")); ASSERT_TRUE(sm->DeleteSnapshot(lock_.get(), "test-snapshot")); }