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

Commit 9ea1acb8 authored by Akilesh Kailash's avatar Akilesh Kailash
Browse files

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: default avatarAkilesh Kailash <akailash@google.com>
Change-Id: Ic0f1ddd390f79966aabfbeadb7d64bc5bb86e83b
parent 57b9a537

fs_mgr/libsnapshot/cow_snapuserd_test.cpp

+83 −11
Original line number Original line Diff line number Diff line
@@ -96,7 +96,8 @@ class TempDevice { 
class CowSnapuserdTest final {

class CowSnapuserdTest final {

  public:

  public:

    bool Setup();

    bool Setup();

    bool SetupCopyOverlap();

    bool SetupCopyOverlap_1();

    bool SetupCopyOverlap_2();

    bool Merge();

    bool Merge();

    void ValidateMerge();

    void ValidateMerge();

    void ReadSnapshotDeviceAndValidate();

    void ReadSnapshotDeviceAndValidate();
@@ -115,7 +116,9 @@ class CowSnapuserdTest final { 
    void StartMerge();

    void StartMerge();





    void CreateCowDevice();

    void CreateCowDevice();

    void CreateCowDeviceWithCopyOverlap();

    void CreateCowDeviceWithCopyOverlap_1();

    void CreateCowDeviceWithCopyOverlap_2();

    bool SetupDaemon();

    void CreateBaseDevice();

    void CreateBaseDevice();

    void InitCowDevice();

    void InitCowDevice();

    void SetDeviceControlName();

    void SetDeviceControlName();
@@ -193,10 +196,19 @@ bool CowSnapuserdTest::Setup() { 
    return setup_ok_;

    return setup_ok_;

}

}





bool CowSnapuserdTest::SetupCopyOverlap() {

bool CowSnapuserdTest::SetupCopyOverlap_1() {

    CreateBaseDevice();

    CreateBaseDevice();

    CreateCowDeviceWithCopyOverlap();

    CreateCowDeviceWithCopyOverlap_1();

    return SetupDaemon();

}



bool CowSnapuserdTest::SetupCopyOverlap_2() {

    CreateBaseDevice();

    CreateCowDeviceWithCopyOverlap_2();

    return SetupDaemon();

}





bool CowSnapuserdTest::SetupDaemon() {

    SetDeviceControlName();

    SetDeviceControlName();





    StartSnapuserdDaemon();

    StartSnapuserdDaemon();
@@ -275,7 +287,59 @@ void CowSnapuserdTest::ReadSnapshotDeviceAndValidate() { 
    ASSERT_EQ(memcmp(snapuserd_buffer.get(), (char*)orig_buffer_.get() + (size_ * 3), size_), 0);

    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();

    std::string path = android::base::GetExecutableDirectory();

    cow_system_ = std::make_unique<TemporaryFile>(path);

    cow_system_ = std::make_unique<TemporaryFile>(path);
@@ -770,17 +834,17 @@ void CowSnapuserdMetadataTest::ValidateMetadata() { 




            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            ASSERT_EQ(de->old_chunk, 21);

            ASSERT_EQ(de->old_chunk, 21);

            ASSERT_EQ(de->new_chunk, 536);

            ASSERT_EQ(de->new_chunk, 537);

            offset += sizeof(struct disk_exception);

            offset += sizeof(struct disk_exception);





            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            ASSERT_EQ(de->old_chunk, 22);

            ASSERT_EQ(de->old_chunk, 22);

            ASSERT_EQ(de->new_chunk, 537);

            ASSERT_EQ(de->new_chunk, 538);

            offset += sizeof(struct disk_exception);

            offset += sizeof(struct disk_exception);





            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            de = reinterpret_cast<struct disk_exception*>((char*)buffer + offset);

            ASSERT_EQ(de->old_chunk, 23);

            ASSERT_EQ(de->old_chunk, 23);

            ASSERT_EQ(de->new_chunk, 538);

            ASSERT_EQ(de->new_chunk, 539);

            offset += sizeof(struct disk_exception);

            offset += sizeof(struct disk_exception);





            // End of metadata

            // End of metadata
@@ -821,9 +885,17 @@ TEST(Snapuserd_Test, Snapshot_IO_TEST) { 
    harness.Shutdown();

    harness.Shutdown();

}

}





TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST) {

TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST_1) {

    CowSnapuserdTest harness;

    ASSERT_TRUE(harness.SetupCopyOverlap_1());

    ASSERT_TRUE(harness.Merge());

    harness.ValidateMerge();

    harness.Shutdown();

}



TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST_2) {

    CowSnapuserdTest harness;

    CowSnapuserdTest harness;

    ASSERT_TRUE(harness.SetupCopyOverlap());

    ASSERT_TRUE(harness.SetupCopyOverlap_2());

    ASSERT_TRUE(harness.Merge());

    ASSERT_TRUE(harness.Merge());

    harness.ValidateMerge();

    harness.ValidateMerge();

    harness.Shutdown();

    harness.Shutdown();
@@ -831,7 +903,7 @@ TEST(Snapuserd_Test, Snapshot_COPY_Overlap_TEST) { 




TEST(Snapuserd_Test, Snapshot_COPY_Overlap_Merge_Resume_TEST) {

TEST(Snapuserd_Test, Snapshot_COPY_Overlap_Merge_Resume_TEST) {

    CowSnapuserdTest harness;

    CowSnapuserdTest harness;

    ASSERT_TRUE(harness.SetupCopyOverlap());

    ASSERT_TRUE(harness.SetupCopyOverlap_1());

    harness.MergeInterrupt();

    harness.MergeInterrupt();

    harness.ValidateMerge();

    harness.ValidateMerge();

    harness.Shutdown();

    harness.Shutdown();

fs_mgr/libsnapshot/snapuserd.cpp

+7 −0
Original line number Original line Diff line number Diff line
@@ -437,6 +437,7 @@ bool Snapuserd::ReadMetadata() { 
    int num_ra_ops_per_iter = ((GetBufferDataSize()) / BLOCK_SZ);

    int num_ra_ops_per_iter = ((GetBufferDataSize()) / BLOCK_SZ);

    std::optional<chunk_t> prev_id = {};

    std::optional<chunk_t> prev_id = {};

    std::map<uint64_t, const CowOperation*> map;

    std::map<uint64_t, const CowOperation*> map;

    std::set<uint64_t> dest_blocks;

    size_t pending_copy_ops = exceptions_per_area_ - num_ops;

    size_t pending_copy_ops = exceptions_per_area_ - num_ops;

    uint64_t total_copy_ops = reader_->total_copy_ops();

    uint64_t total_copy_ops = reader_->total_copy_ops();
@@ -555,10 +556,15 @@ bool Snapuserd::ReadMetadata() { 
                if (diff != 1) {

                if (diff != 1) {

                    break;

                    break;

                }

                }



                if (dest_blocks.count(cow_op->new_block) || map.count(cow_op->source) > 0) {

                    break;

                }

            }

            }

            metadata_found = true;

            metadata_found = true;

            pending_copy_ops -= 1;

            pending_copy_ops -= 1;

            map[cow_op->new_block] = cow_op;

            map[cow_op->new_block] = cow_op;

            dest_blocks.insert(cow_op->source);

            prev_id = cow_op->new_block;

            prev_id = cow_op->new_block;

            cowop_riter_->Next();

            cowop_riter_->Next();

        } while (!cowop_riter_->Done() && pending_copy_ops);

        } while (!cowop_riter_->Done() && pending_copy_ops);
@@ -620,6 +626,7 @@ bool Snapuserd::ReadMetadata() { 
            }

            }

        }

        }

        map.clear();

        map.clear();

        dest_blocks.clear();

        prev_id.reset();

        prev_id.reset();

    }

    }

fs_mgr/libsnapshot/snapuserd_server.cpp

+1 −1
Original line number Original line Diff line number Diff line
@@ -191,7 +191,7 @@ bool SnapuserdServer::Receivemsg(android::base::borrowed_fd fd, const std::strin 
        }

        }

        case DaemonOperations::DETACH: {

        case DaemonOperations::DETACH: {

            terminating_ = true;

            terminating_ = true;

            return Sendmsg(fd, "success");

            return true;

        }

        }

        default: {

        default: {

            LOG(ERROR) << "Received unknown message type from client";

            LOG(ERROR) << "Received unknown message type from client";