snapuserd: Fix merging of overlapping copy blocks
As part of r.android.com/c/1678745/7, overlapping
copy operations was allowed to batch merge which
is not right. The intention of that CL was
to avoid un-necessary write traffic involved
in flushing data to scratch space. However,
as part of the optimization, copy operations
were merged. More specifically, the problem
comes into play when the number of overlapping
copy operations is more than the read-ahead
window size (2MB).
I have added a new test case to test this
specific code path to avoid future regressions.
Additionally, remove un-necessary "send()"
as part of "detach" response from snapuserd server.
Client is not waiting for any response. It
just creates a race window which is harmless
but error log will be misleading.
Bug: 187506548
Test: cow_snapuserd which tests the similar
configuration as seen in the COW file in the bug
report.
Signed-off-by: Akilesh Kailash <akailash@google.com>
Change-Id: Ic0f1ddd390f79966aabfbeadb7d64bc5bb86e83b
diff --git a/fs_mgr/libsnapshot/cow_snapuserd_test.cpp b/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
index 313eb64..3888eb1 100644
--- a/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
+++ b/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
@@ -96,7 +96,8 @@
class CowSnapuserdTest final {
public:
bool Setup();
- bool SetupCopyOverlap();
+ bool SetupCopyOverlap_1();
+ bool SetupCopyOverlap_2();
bool Merge();
void ValidateMerge();
void ReadSnapshotDeviceAndValidate();
@@ -115,7 +116,9 @@
void StartMerge();
void CreateCowDevice();
- void CreateCowDeviceWithCopyOverlap();
+ void CreateCowDeviceWithCopyOverlap_1();
+ void CreateCowDeviceWithCopyOverlap_2();
+ bool SetupDaemon();
void CreateBaseDevice();
void InitCowDevice();
void SetDeviceControlName();
@@ -193,10 +196,19 @@
return setup_ok_;
}
-bool CowSnapuserdTest::SetupCopyOverlap() {
+bool CowSnapuserdTest::SetupCopyOverlap_1() {
CreateBaseDevice();
- CreateCowDeviceWithCopyOverlap();
+ CreateCowDeviceWithCopyOverlap_1();
+ return SetupDaemon();
+}
+bool CowSnapuserdTest::SetupCopyOverlap_2() {
+ CreateBaseDevice();
+ CreateCowDeviceWithCopyOverlap_2();
+ return SetupDaemon();
+}
+
+bool CowSnapuserdTest::SetupDaemon() {
SetDeviceControlName();
StartSnapuserdDaemon();
@@ -275,7 +287,59 @@
ASSERT_EQ(memcmp(snapuserd_buffer.get(), (char*)orig_buffer_.get() + (size_ * 3), size_), 0);
}
-void CowSnapuserdTest::CreateCowDeviceWithCopyOverlap() {
+void CowSnapuserdTest::CreateCowDeviceWithCopyOverlap_2() {
+ std::string path = android::base::GetExecutableDirectory();
+ cow_system_ = std::make_unique<TemporaryFile>(path);
+
+ CowOptions options;
+ options.compression = "gz";
+ CowWriter writer(options);
+
+ ASSERT_TRUE(writer.Initialize(cow_system_->fd));
+
+ size_t num_blocks = size_ / options.block_size;
+ size_t x = num_blocks;
+ size_t blk_src_copy = 0;
+
+ // Create overlapping copy operations
+ while (1) {
+ ASSERT_TRUE(writer.AddCopy(blk_src_copy, blk_src_copy + 1));
+ x -= 1;
+ if (x == 1) {
+ break;
+ }
+ blk_src_copy += 1;
+ }
+
+ // Flush operations
+ ASSERT_TRUE(writer.Finalize());
+
+ // Construct the buffer required for validation
+ orig_buffer_ = std::make_unique<uint8_t[]>(total_base_size_);
+
+ // Read the entire base device
+ ASSERT_EQ(android::base::ReadFullyAtOffset(base_fd_, orig_buffer_.get(), total_base_size_, 0),
+ true);
+
+ // Merged operations required for validation
+ int block_size = 4096;
+ x = num_blocks;
+ loff_t src_offset = block_size;
+ loff_t dest_offset = 0;
+
+ while (1) {
+ memmove((char*)orig_buffer_.get() + dest_offset, (char*)orig_buffer_.get() + src_offset,
+ block_size);
+ x -= 1;
+ if (x == 1) {
+ break;
+ }
+ src_offset += block_size;
+ dest_offset += block_size;
+ }
+}
+
+void CowSnapuserdTest::CreateCowDeviceWithCopyOverlap_1() {
std::string path = android::base::GetExecutableDirectory();
cow_system_ = std::make_unique<TemporaryFile>(path);
@@ -770,19 +834,19 @@
de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);
ASSERT_EQ(de->old_chunk, 21);
- ASSERT_EQ(de->new_chunk, 536);
- offset += sizeof(struct disk_exception);
-
- de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);
- ASSERT_EQ(de->old_chunk, 22);
ASSERT_EQ(de->new_chunk, 537);
offset += sizeof(struct disk_exception);
de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);
- ASSERT_EQ(de->old_chunk, 23);
+ ASSERT_EQ(de->old_chunk, 22);
ASSERT_EQ(de->new_chunk, 538);
offset += sizeof(struct disk_exception);
+ de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);
+ ASSERT_EQ(de->old_chunk, 23);
+ ASSERT_EQ(de->new_chunk, 539);
+ offset += sizeof(struct disk_exception);
+
// End of metadata
de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);
ASSERT_EQ(de->old_chunk, 0);
@@ -821,9 +885,17 @@
harness.Shutdown();
}
-TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST) {
+TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST_1) {
CowSnapuserdTest harness;
- ASSERT_TRUE(harness.SetupCopyOverlap());
+ ASSERT_TRUE(harness.SetupCopyOverlap_1());
+ ASSERT_TRUE(harness.Merge());
+ harness.ValidateMerge();
+ harness.Shutdown();
+}
+
+TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST_2) {
+ CowSnapuserdTest harness;
+ ASSERT_TRUE(harness.SetupCopyOverlap_2());
ASSERT_TRUE(harness.Merge());
harness.ValidateMerge();
harness.Shutdown();
@@ -831,7 +903,7 @@
TEST(Snapuserd_Test, Snapshot_COPY_Overlap_Merge_Resume_TEST) {
CowSnapuserdTest harness;
- ASSERT_TRUE(harness.SetupCopyOverlap());
+ ASSERT_TRUE(harness.SetupCopyOverlap_1());
harness.MergeInterrupt();
harness.ValidateMerge();
harness.Shutdown();
diff --git a/fs_mgr/libsnapshot/snapuserd.cpp b/fs_mgr/libsnapshot/snapuserd.cpp
index 2ccc750..3210983 100644
--- a/fs_mgr/libsnapshot/snapuserd.cpp
+++ b/fs_mgr/libsnapshot/snapuserd.cpp
@@ -437,6 +437,7 @@
int num_ra_ops_per_iter = ((GetBufferDataSize()) / BLOCK_SZ);
std::optional<chunk_t> prev_id = {};
std::map<uint64_t, const CowOperation*> map;
+ std::set<uint64_t> dest_blocks;
size_t pending_copy_ops = exceptions_per_area_ - num_ops;
uint64_t total_copy_ops = reader_->total_copy_ops();
@@ -555,10 +556,15 @@
if (diff != 1) {
break;
}
+
+ if (dest_blocks.count(cow_op->new_block) || map.count(cow_op->source) > 0) {
+ break;
+ }
}
metadata_found = true;
pending_copy_ops -= 1;
map[cow_op->new_block] = cow_op;
+ dest_blocks.insert(cow_op->source);
prev_id = cow_op->new_block;
cowop_riter_->Next();
} while (!cowop_riter_->Done() && pending_copy_ops);
@@ -620,6 +626,7 @@
}
}
map.clear();
+ dest_blocks.clear();
prev_id.reset();
}
diff --git a/fs_mgr/libsnapshot/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd_server.cpp
index ff8a259..3b0af3e 100644
--- a/fs_mgr/libsnapshot/snapuserd_server.cpp
+++ b/fs_mgr/libsnapshot/snapuserd_server.cpp
@@ -191,7 +191,7 @@
}
case DaemonOperations::DETACH: {
terminating_ = true;
- return Sendmsg(fd, "success");
+ return true;
}
default: {
LOG(ERROR) << "Received unknown message type from client";