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

Commit d986fefa authored by David Anderson's avatar David Anderson
Browse files

libsnapshot: Eliminate per-snapshot flocks. 

Per-snapshot locks don't solve any problems and add a great deal of
complexity. Instead, refactor the Read/WriteSnapshotStatus methods so
the caller just needs the snapshot name, and is not responsible for
opening a file.

As part of this change, callers of WriteSnapshotStatus must always take
an exclusive flock on the update state file. This is enforced by adding
a helper method to LockedFile to check the lock mode.

Bug: 136678799
Test: libsnapshot_test gtest
Change-Id: Icd580aaec7dfc916b3eed174d86b26688cd2291b
parent 3cb682e3

fs_mgr/libsnapshot/include/libsnapshot/snapshot.h

+7 −6
Original line number Diff line number Diff line
@@ -126,16 +126,18 @@ class SnapshotManager final { 
    // this. It also serves as a proof-of-lock for some functions.

    class LockedFile final {

      public:

        LockedFile(const std::string& path, android::base::unique_fd&& fd)

            : path_(path), fd_(std::move(fd)) {}

        LockedFile(const std::string& path, android::base::unique_fd&& fd, int lock_mode)

            : path_(path), fd_(std::move(fd)), lock_mode_(lock_mode) {}

        ~LockedFile();



        const std::string& path() const { return path_; }

        int fd() const { return fd_; }

        int lock_mode() const { return lock_mode_; }



      private:

        std::string path_;

        android::base::unique_fd fd_;

        int lock_mode_;

    };

    std::unique_ptr<LockedFile> OpenFile(const std::string& file, int open_flags, int lock_flags);

    bool Truncate(LockedFile* file);
@@ -202,10 +204,9 @@ class SnapshotManager final { 
    };



    // Interact with status files under /metadata/ota/snapshots.

    std::unique_ptr<LockedFile> OpenSnapshotStatusFile(const std::string& name, int open_flags,

                                                       int lock_flags);

    bool WriteSnapshotStatus(LockedFile* file, const SnapshotStatus& status);

    bool ReadSnapshotStatus(LockedFile* file, SnapshotStatus* status);

    bool WriteSnapshotStatus(LockedFile* lock, const std::string& name,

                             const SnapshotStatus& status);

    bool ReadSnapshotStatus(LockedFile* lock, const std::string& name, SnapshotStatus* status);

    std::string GetSnapshotStatusFilePath(const std::string& name);



    // Return the name of the device holding the "snapshot" or "snapshot-merge"

fs_mgr/libsnapshot/snapshot.cpp

+40 −39
Original line number Diff line number Diff line
@@ -140,9 +140,6 @@ bool SnapshotManager::CreateSnapshot(LockedFile* lock, const std::string& name, 


    LOG(INFO) << "Snapshot " << name << " will have COW size " << cow_size;



    auto status_file = OpenSnapshotStatusFile(name, O_RDWR | O_CREAT, LOCK_EX);

    if (!status_file) return false;



    // Note, we leave the status file hanging around if we fail to create the

    // actual backing image. This is harmless, since it'll get removed when

    // CancelUpdate is called.
@@ -151,7 +148,7 @@ bool SnapshotManager::CreateSnapshot(LockedFile* lock, const std::string& name, 
            .device_size = device_size,

            .snapshot_size = snapshot_size,

    };

    if (!WriteSnapshotStatus(status_file.get(), status)) {

    if (!WriteSnapshotStatus(lock, name, status)) {

        PLOG(ERROR) << "Could not write snapshot status: " << name;

        return false;

    }
@@ -168,11 +165,8 @@ bool SnapshotManager::MapSnapshot(LockedFile* lock, const std::string& name, 
    CHECK(lock);

    if (!EnsureImageManager()) return false;



    auto status_file = OpenSnapshotStatusFile(name, O_RDWR, LOCK_EX);

    if (!status_file) return false;



    SnapshotStatus status;

    if (!ReadSnapshotStatus(status_file.get(), &status)) {

    if (!ReadSnapshotStatus(lock, name, &status)) {

        return false;

    }
@@ -267,11 +261,8 @@ bool SnapshotManager::UnmapSnapshot(LockedFile* lock, const std::string& name) { 
    CHECK(lock);

    if (!EnsureImageManager()) return false;



    auto status_file = OpenSnapshotStatusFile(name, O_RDWR, LOCK_EX);

    if (!status_file) return false;



    SnapshotStatus status;

    if (!ReadSnapshotStatus(status_file.get(), &status)) {

    if (!ReadSnapshotStatus(lock, name, &status)) {

        return false;

    }
@@ -300,10 +291,6 @@ bool SnapshotManager::DeleteSnapshot(LockedFile* lock, const std::string& name) 
    CHECK(lock);

    if (!EnsureImageManager()) 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;



    auto cow_name = GetCowName(name);

    if (!images_->BackingImageExists(cow_name)) {

        return true;
@@ -312,8 +299,10 @@ bool SnapshotManager::DeleteSnapshot(LockedFile* lock, const std::string& name) 
        return false;

    }



    if (!android::base::RemoveFileIfExists(status_file->path())) {

        LOG(ERROR) << "Failed to remove status file: " << status_file->path();

    std::string error;

    auto file_path = GetSnapshotStatusFilePath(name);

    if (!android::base::RemoveFileIfExists(file_path, &error)) {

        LOG(ERROR) << "Failed to remove status file " << file_path << ": " << error;

        return false;

    }

    return true;
@@ -394,7 +383,10 @@ auto SnapshotManager::OpenFile(const std::string& file, int open_flags, int lock 
        PLOG(ERROR) << "Acquire flock failed: " << file;

        return nullptr;

    }

    return std::make_unique<LockedFile>(file, std::move(fd));

    // For simplicity, we want to CHECK that lock_mode == LOCK_EX, in some

    // calls, so strip extra flags.

    int lock_mode = lock_flags & (LOCK_EX | LOCK_SH);

    return std::make_unique<LockedFile>(file, std::move(fd), lock_mode);

}



SnapshotManager::LockedFile::~LockedFile() {
@@ -486,51 +478,61 @@ std::string SnapshotManager::GetSnapshotStatusFilePath(const std::string& name) 
    return file;

}



auto SnapshotManager::OpenSnapshotStatusFile(const std::string& name, int open_flags,

                                             int lock_flags) -> std::unique_ptr<LockedFile> {

    auto file = GetSnapshotStatusFilePath(name);

    return OpenFile(file, open_flags, lock_flags);

}

bool SnapshotManager::ReadSnapshotStatus(LockedFile* lock, const std::string& name,

                                         SnapshotStatus* status) {

    CHECK(lock);

    auto path = GetSnapshotStatusFilePath(name);



bool SnapshotManager::ReadSnapshotStatus(LockedFile* file, SnapshotStatus* status) {

    // Reset position since some calls read+write.

    if (lseek(file->fd(), 0, SEEK_SET) < 0) {

        PLOG(ERROR) << "lseek status file failed";

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

    if (fd < 0) {

        PLOG(ERROR) << "Open failed: " << path;

        return false;

    }



    std::string contents;

    if (!android::base::ReadFdToString(file->fd(), &contents)) {

        PLOG(ERROR) << "read status file failed";

    if (!android::base::ReadFdToString(fd, &contents)) {

        PLOG(ERROR) << "read failed: " << path;

        return false;

    }

    auto pieces = android::base::Split(contents, " ");

    if (pieces.size() != 5) {

        LOG(ERROR) << "Invalid status line for snapshot: " << file->path();

        LOG(ERROR) << "Invalid status line for snapshot: " << path;

        return false;

    }



    status->state = pieces[0];

    if (!android::base::ParseUint(pieces[1], &status->device_size)) {

        LOG(ERROR) << "Invalid device size in status line for: " << file->path();

        LOG(ERROR) << "Invalid device size in status line for: " << path;

        return false;

    }

    if (!android::base::ParseUint(pieces[2], &status->snapshot_size)) {

        LOG(ERROR) << "Invalid snapshot size in status line for: " << file->path();

        LOG(ERROR) << "Invalid snapshot size in status line for: " << path;

        return false;

    }

    if (!android::base::ParseUint(pieces[3], &status->sectors_allocated)) {

        LOG(ERROR) << "Invalid snapshot size in status line for: " << file->path();

        LOG(ERROR) << "Invalid snapshot size in status line for: " << path;

        return false;

    }

    if (!android::base::ParseUint(pieces[4], &status->metadata_sectors)) {

        LOG(ERROR) << "Invalid snapshot size in status line for: " << file->path();

        LOG(ERROR) << "Invalid snapshot size in status line for: " << path;

        return false;

    }

    return true;

}



bool SnapshotManager::WriteSnapshotStatus(LockedFile* file, const SnapshotStatus& status) {

bool SnapshotManager::WriteSnapshotStatus(LockedFile* lock, const std::string& name,

                                          const SnapshotStatus& status) {

    // The caller must take an exclusive lock to modify snapshots.

    CHECK(lock);

    CHECK(lock->lock_mode() == LOCK_EX);



    auto path = GetSnapshotStatusFilePath(name);

    unique_fd fd(open(path.c_str(), O_RDWR | O_CLOEXEC | O_NOFOLLOW | O_CREAT | O_SYNC, 0660));

    if (fd < 0) {

        PLOG(ERROR) << "Open failed: " << path;

        return false;

    }



    std::vector<std::string> pieces = {

            status.state,

            std::to_string(status.device_size),
@@ -540,9 +542,8 @@ bool SnapshotManager::WriteSnapshotStatus(LockedFile* file, const SnapshotStatus 
    };

    auto contents = android::base::Join(pieces, " ");



    if (!Truncate(file)) return false;

    if (!android::base::WriteStringToFd(contents, file->fd())) {

        PLOG(ERROR) << "write to status file failed: " << file->path();

    if (!android::base::WriteStringToFd(contents, fd)) {

        PLOG(ERROR) << "write failed: " << path;

        return false;

    }

    return true;

fs_mgr/libsnapshot/snapshot_test.cpp

+1 −4
Original line number Diff line number Diff line
@@ -140,11 +140,8 @@ TEST_F(SnapshotTest, CreateSnapshot) { 


    // Scope so delete can re-acquire the snapshot file lock.

    {

        auto file = sm->OpenSnapshotStatusFile("test-snapshot", O_RDONLY, LOCK_SH);

        ASSERT_NE(file, nullptr);



        SnapshotManager::SnapshotStatus status;

        ASSERT_TRUE(sm->ReadSnapshotStatus(file.get(), &status));

        ASSERT_TRUE(sm->ReadSnapshotStatus(lock_.get(), "test-snapshot", &status));

        ASSERT_EQ(status.state, "created");

        ASSERT_EQ(status.device_size, kDeviceSize);

        ASSERT_EQ(status.snapshot_size, kDeviceSize);