commit ae0783f4c7a9263450c2bc92b69583220f479753
author Akilesh Kailash <akailash@google.com> Thu Jan 21 06:14:04 2021 +0000
committer Akilesh Kailash <akailash@google.com> Thu Jan 21 06:27:55 2021 +0000
tree 7c8fa2c8290a5dd2791edee3b88be7b11bc6239d
parent 5e9fb35ba880cd0bba417b544e2564280c089a06

libsnapshot: Merge completion for sector 0

Snapuserd daemon parses the merge completion request based on
how the dm-snapshot merge is done. dm-snapshot marks the merge as
complete by zeroing out the metadata viz old-chunk and new-chunk id's.

If we have a sector 0 operation such as copy/replace op,
then old-chunk id will be 0 and new-chunk id will be a non-zero
pseudo number. Once the merge is complete, then old-chunk and new-chunk will be 0.

The problem is that daemon used to track the merge completion just by checking
if old-chunk was non-zero. This check is not sufficient and ends up
tripping the assert in the daemon.

Bug: 178061207
Test: Modify cow_snapuserd_test to test this case and validate the
      IO path.
Reported-by: Kelvin Zhang <zhangkelvin@google.com>
Signed-off-by: Akilesh Kailash <akailash@google.com>
Change-Id: I6603af1c7b55e487dc3aec0c30c0a9dea0fedb56

fs_mgr/libsnapshot/cow_snapuserd_test.cpp

diff --git a/fs_mgr/libsnapshot/cow_snapuserd_test.cpp b/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
index 4cc0fd3..0addba3 100644
--- a/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
+++ b/fs_mgr/libsnapshot/cow_snapuserd_test.cpp
@@ -255,17 +255,23 @@
     ASSERT_TRUE(writer.Initialize(cow_system_->fd));
 
     size_t num_blocks = size_ / options.block_size;
-    size_t blk_src_copy = num_blocks;
-    size_t blk_end_copy = blk_src_copy + num_blocks;
-    size_t source_blk = 0;
+    size_t blk_end_copy = num_blocks * 2;
+    size_t source_blk = num_blocks - 1;
+    size_t blk_src_copy = blk_end_copy - 1;
 
-    while (source_blk < num_blocks) {
+    size_t x = num_blocks;
+    while (1) {
         ASSERT_TRUE(writer.AddCopy(source_blk, blk_src_copy));
-        source_blk += 1;
-        blk_src_copy += 1;
+        x -= 1;
+        if (x == 0) {
+            break;
+        }
+        source_blk -= 1;
+        blk_src_copy -= 1;
     }
 
-    ASSERT_EQ(blk_src_copy, blk_end_copy);
+    source_blk = num_blocks;
+    blk_src_copy = blk_end_copy;
 
     ASSERT_TRUE(writer.AddRawBlocks(source_blk, random_buffer_1_.get(), size_));
 
@@ -280,11 +286,9 @@
 
     // Flush operations
     ASSERT_TRUE(writer.Finalize());
-
     // Construct the buffer required for validation
     orig_buffer_ = std::make_unique<uint8_t[]>(total_base_size_);
     std::string zero_buffer(size_, 0);
-
     ASSERT_EQ(android::base::ReadFullyAtOffset(base_fd_, orig_buffer_.get(), size_, size_), true);
     memcpy((char*)orig_buffer_.get() + size_, random_buffer_1_.get(), size_);
     memcpy((char*)orig_buffer_.get() + (size_ * 2), (void*)zero_buffer.c_str(), size_);

fs_mgr/libsnapshot/snapuserd.cpp

diff --git a/fs_mgr/libsnapshot/snapuserd.cpp b/fs_mgr/libsnapshot/snapuserd.cpp
index 573b6a5..9ad4a1a 100644
--- a/fs_mgr/libsnapshot/snapuserd.cpp
+++ b/fs_mgr/libsnapshot/snapuserd.cpp
@@ -324,8 +324,7 @@
                 reinterpret_cast<struct disk_exception*>((char*)unmerged_buffer + offset);
 
         // Unmerged op by the kernel
-        if (merged_de->old_chunk != 0) {
-            CHECK(merged_de->new_chunk != 0);
+        if (merged_de->old_chunk != 0 || merged_de->new_chunk != 0) {
             CHECK(merged_de->old_chunk == cow_de->old_chunk);
             CHECK(merged_de->new_chunk == cow_de->new_chunk);
 
@@ -334,11 +333,6 @@
             continue;
         }
 
-        // Merge complete on this exception. However, we don't know how many
-        // merged in this cycle; hence break here.
-        CHECK(merged_de->new_chunk == 0);
-        CHECK(merged_de->old_chunk == 0);
-
         break;
     }