From c4b46e0ad972cf8f238794349c6e939b268de522 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 16 Jan 2018 15:49:08 -0800 Subject: [PATCH] storaged: use health HAL to read DiskStats. Test: storaged unit tests Bug: 68388678 Change-Id: I03ce3aa71fb54ae40489c7b35973cd4b83b13bfb --- storaged/Android.bp | 5 +- storaged/include/storaged.h | 2 +- storaged/include/storaged_diskstats.h | 41 +++++++----- storaged/storaged.cpp | 7 ++- storaged/storaged_diskstats.cpp | 90 +++++++++++++++++++++++---- storaged/tests/storaged_test.cpp | 14 ++++- 6 files changed, 123 insertions(+), 36 deletions(-) diff --git a/storaged/Android.bp b/storaged/Android.bp index 99491e25f..b478f4a8f 100644 --- a/storaged/Android.bp +++ b/storaged/Android.bp @@ -103,7 +103,10 @@ cc_test { srcs: ["tests/storaged_test.cpp"], - static_libs: ["libstoraged"], + static_libs: [ + "libhealthhalutils", + "libstoraged", + ], } // AIDL interface between storaged and framework.jar diff --git a/storaged/include/storaged.h b/storaged/include/storaged.h index 10175123a..c5cac27e4 100644 --- a/storaged/include/storaged.h +++ b/storaged/include/storaged.h @@ -81,7 +81,7 @@ class storaged_t : public android::hardware::health::V2_0::IHealthInfoCallback, private: time_t mTimer; storaged_config mConfig; - disk_stats_monitor mDsm; + unique_ptr mDsm; uid_monitor mUidm; time_t mStarttime; sp health; diff --git a/storaged/include/storaged_diskstats.h b/storaged/include/storaged_diskstats.h index ff030f640..0b93ba6c5 100644 --- a/storaged/include/storaged_diskstats.h +++ b/storaged/include/storaged_diskstats.h @@ -19,6 +19,8 @@ #include +#include + // number of attributes diskstats has #define DISK_STATS_SIZE ( 11 ) @@ -160,6 +162,7 @@ private: const double mSigma; struct disk_perf mMean; struct disk_perf mStd; + android::sp mHealth; void update_mean(); void update_std(); @@ -170,21 +173,27 @@ private: void update(struct disk_stats* stats); public: - disk_stats_monitor(uint32_t window_size = 5, double sigma = 1.0) : - DISK_STATS_PATH(access(MMC_DISK_STATS_PATH, R_OK) ? - (access(SDA_DISK_STATS_PATH, R_OK) ? - nullptr : - SDA_DISK_STATS_PATH) : - MMC_DISK_STATS_PATH), - mPrevious(), mAccumulate(), mAccumulate_pub(), - mStall(false), mValid(false), - mWindow(window_size), mSigma(sigma), - mMean(), mStd() {} - bool enabled() { - return DISK_STATS_PATH != nullptr; - } - void update(void); - void publish(void); + disk_stats_monitor(const android::sp& healthService, + uint32_t window_size = 5, double sigma = 1.0) + : DISK_STATS_PATH( + healthService != nullptr + ? nullptr + : (access(MMC_DISK_STATS_PATH, R_OK) == 0 + ? MMC_DISK_STATS_PATH + : (access(SDA_DISK_STATS_PATH, R_OK) == 0 ? SDA_DISK_STATS_PATH : nullptr))), + mPrevious(), + mAccumulate(), + mAccumulate_pub(), + mStall(false), + mValid(false), + mWindow(window_size), + mSigma(sigma), + mMean(), + mStd(), + mHealth(healthService) {} + bool enabled() { return mHealth != nullptr || DISK_STATS_PATH != nullptr; } + void update(void); + void publish(void); }; -#endif /* _STORAGED_DISKSTATS_H_ */ \ No newline at end of file +#endif /* _STORAGED_DISKSTATS_H_ */ diff --git a/storaged/storaged.cpp b/storaged/storaged.cpp index 7086887e0..09f2abec1 100644 --- a/storaged/storaged.cpp +++ b/storaged/storaged.cpp @@ -86,6 +86,7 @@ Return storaged_t::healthInfoChanged(const HealthInfo& props) { void storaged_t::init() { init_health_service(); + mDsm = std::make_unique(health); } void storaged_t::init_health_service() { @@ -311,10 +312,10 @@ void storaged_t::flush_protos(unordered_map* protos) { void storaged_t::event(void) { unordered_map protos; - if (mDsm.enabled()) { - mDsm.update(); + if (mDsm->enabled()) { + mDsm->update(); if (!(mTimer % mConfig.periodic_chores_interval_disk_stats_publish)) { - mDsm.publish(); + mDsm->publish(); } } diff --git a/storaged/storaged_diskstats.cpp b/storaged/storaged_diskstats.cpp index 0604e0ad5..105003340 100644 --- a/storaged/storaged_diskstats.cpp +++ b/storaged/storaged_diskstats.cpp @@ -30,6 +30,12 @@ namespace { +using android::sp; +using android::hardware::health::V2_0::DiskStats; +using android::hardware::health::V2_0::IHealth; +using android::hardware::health::V2_0::Result; +using android::hardware::health::V2_0::toString; + #ifdef DEBUG void log_debug_disk_perf(struct disk_perf* perf, const char* type) { // skip if the input structure are all zeros @@ -60,17 +66,30 @@ void log_event_disk_stats(struct disk_stats* stats, const char* type) { } // namespace -bool parse_disk_stats(const char* disk_stats_path, struct disk_stats* stats) -{ - // Get time - struct timespec ts; +bool get_time(struct timespec* ts) { // Use monotonic to exclude suspend time so that we measure IO bytes/sec // when system is running. - int ret = clock_gettime(CLOCK_MONOTONIC, &ts); + int ret = clock_gettime(CLOCK_MONOTONIC, ts); if (ret < 0) { PLOG_TO(SYSTEM, ERROR) << "clock_gettime() failed"; return false; } + return true; +} + +void init_disk_stats_other(const struct timespec& ts, struct disk_stats* stats) { + stats->start_time = 0; + stats->end_time = (uint64_t)ts.tv_sec * SEC_TO_MSEC + ts.tv_nsec / (MSEC_TO_USEC * USEC_TO_NSEC); + stats->counter = 1; + stats->io_avg = (double)stats->io_in_flight; +} + +bool parse_disk_stats(const char* disk_stats_path, struct disk_stats* stats) { + // Get time + struct timespec ts; + if (!get_time(&ts)) { + return false; + } std::string buffer; if (!android::base::ReadFileToString(disk_stats_path, &buffer)) { @@ -84,11 +103,52 @@ bool parse_disk_stats(const char* disk_stats_path, struct disk_stats* stats) ss >> *((uint64_t*)stats + i); } // Other entries - stats->start_time = 0; - stats->end_time = (uint64_t)ts.tv_sec * SEC_TO_MSEC + - ts.tv_nsec / (MSEC_TO_USEC * USEC_TO_NSEC); - stats->counter = 1; - stats->io_avg = (double)stats->io_in_flight; + init_disk_stats_other(ts, stats); + return true; +} + +void convert_hal_disk_stats(struct disk_stats* dst, const DiskStats& src) { + dst->read_ios = src.reads; + dst->read_merges = src.readMerges; + dst->read_sectors = src.readSectors; + dst->read_ticks = src.readTicks; + dst->write_ios = src.writes; + dst->write_merges = src.writeMerges; + dst->write_sectors = src.writeSectors; + dst->write_ticks = src.writeTicks; + dst->io_in_flight = src.ioInFlight; + dst->io_ticks = src.ioTicks; + dst->io_in_queue = src.ioInQueue; +} + +bool get_disk_stats_from_health_hal(const sp& service, struct disk_stats* stats) { + struct timespec ts; + if (!get_time(&ts)) { + return false; + } + + bool success = false; + auto ret = service->getDiskStats([&success, stats](auto result, const auto& halStats) { + if (result != Result::SUCCESS || halStats.size() == 0) { + LOG_TO(SYSTEM, ERROR) << "getDiskStats failed with result " << toString(result) + << " and size " << halStats.size(); + return; + } + + convert_hal_disk_stats(stats, halStats[0]); + success = true; + }); + + if (!ret.isOk()) { + LOG_TO(SYSTEM, ERROR) << "getDiskStats failed with " << ret.description(); + return false; + } + + if (!success) { + return false; + } + + init_disk_stats_other(ts, stats); return true; } @@ -243,8 +303,14 @@ void disk_stats_monitor::update(struct disk_stats* curr) void disk_stats_monitor::update() { disk_stats curr; - if (!parse_disk_stats(DISK_STATS_PATH, &curr)) { - return; + if (mHealth != nullptr) { + if (!get_disk_stats_from_health_hal(mHealth, &curr)) { + return; + } + } else { + if (!parse_disk_stats(DISK_STATS_PATH, &curr)) { + return; + } } update(&curr); diff --git a/storaged/tests/storaged_test.cpp b/storaged/tests/storaged_test.cpp index 6a5fc6138..d1fa9ed21 100644 --- a/storaged/tests/storaged_test.cpp +++ b/storaged/tests/storaged_test.cpp @@ -25,6 +25,7 @@ #include +#include #include // data structures #include // functions to test @@ -234,10 +235,17 @@ void expect_increasing(struct disk_stats stats1, struct disk_stats stats2) { } TEST(storaged_test, disk_stats_monitor) { + using android::hardware::health::V2_0::get_health_service; + + auto healthService = get_health_service(); + // asserting that there is one file for diskstats - ASSERT_TRUE(access(MMC_DISK_STATS_PATH, R_OK) >= 0 || access(SDA_DISK_STATS_PATH, R_OK) >= 0); + ASSERT_TRUE(healthService != nullptr || access(MMC_DISK_STATS_PATH, R_OK) >= 0 || + access(SDA_DISK_STATS_PATH, R_OK) >= 0); + // testing if detect() will return the right value - disk_stats_monitor dsm_detect; + disk_stats_monitor dsm_detect{healthService}; + ASSERT_TRUE(dsm_detect.enabled()); // feed monitor with constant perf data for io perf baseline // using constant perf is reasonable since the functionality of stream_stats // has already been tested @@ -280,7 +288,7 @@ TEST(storaged_test, disk_stats_monitor) { } // testing if stalled disk_stats can be correctly accumulated in the monitor - disk_stats_monitor dsm_acc; + disk_stats_monitor dsm_acc{healthService}; struct disk_stats norm_inc = { .read_ios = 200, .read_merges = 0,