Commit 3fd2c265 authored by Jarryd's avatar Jarryd Committed by Commit Bot

Quota: Fix precision mistakes for storage pressure.

There have been a number of reports about the storage pressure
notification being shown, even when there is an absence of
storage pressure.

This is due to a numerical precision issue, where division of two
ints would almost always return 0. This 0 meant that a condition
that checked for storage pressure would always pass, and QuotaManger
would fire storage pressure notifications for all write errors,
regardless of disk availability.

This change removes the division operation entirely with a little
trick:

 - Given percent_available = 100 * available_space / total,
 - percent_available < kStoragePressureThresholdRatio  is equivalent
     to (substitution):
 - 100 * available_space / total < kStoragePressureThresholdRatio
     is equivalent to (multiply both sides by total):
 - 100 * available_space < kStoragePressureThresholdRatio * total

TBR=pwnall@chromium.org

Bug: 1127237
Change-Id: I79c5ff47f27824d25ff8e47dc4aed859c8d3cd7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412758Reviewed-by: default avatarJarryd Goodman <jarrydg@chromium.org>
Commit-Queue: Jarryd Goodman <jarrydg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807356}
parent fdc59761
...@@ -38,8 +38,9 @@ const base::TimeDelta GetThrottlingInterval() { ...@@ -38,8 +38,9 @@ const base::TimeDelta GetThrottlingInterval() {
void StorageNotificationServiceImpl::MaybeShowStoragePressureNotification( void StorageNotificationServiceImpl::MaybeShowStoragePressureNotification(
const url::Origin origin) { const url::Origin origin) {
if (base::TimeTicks::Now() - disk_pressure_notification_last_sent_at_ < if (!disk_pressure_notification_last_sent_at_.is_null() &&
GetThrottlingInterval()) { base::TimeTicks::Now() - disk_pressure_notification_last_sent_at_ <
GetThrottlingInterval()) {
return; return;
} }
......
...@@ -53,7 +53,7 @@ constexpr int64_t kReportHistogramInterval = 60 * 60 * 1000; // 1 hour ...@@ -53,7 +53,7 @@ constexpr int64_t kReportHistogramInterval = 60 * 60 * 1000; // 1 hour
// Take action on write errors if there is <= 2% disk space // Take action on write errors if there is <= 2% disk space
// available. // available.
constexpr double kStoragePressureThresholdRatio = 2; constexpr double kStoragePressureThresholdPercent = 2;
// Limit how frequently QuotaManager polls for free disk space when // Limit how frequently QuotaManager polls for free disk space when
// only using that information to identify storage pressure. // only using that information to identify storage pressure.
...@@ -1486,7 +1486,8 @@ void QuotaManager::MaybeRunStoragePressureCallback(const url::Origin& origin, ...@@ -1486,7 +1486,8 @@ void QuotaManager::MaybeRunStoragePressureCallback(const url::Origin& origin,
origin_for_pending_storage_pressure_callback_ = std::move(origin); origin_for_pending_storage_pressure_callback_ = std::move(origin);
return; return;
} }
if (100 * (available_space / total_space) < kStoragePressureThresholdRatio) {
if (100 * available_space < kStoragePressureThresholdPercent * total_space) {
storage_pressure_callback_.Run(std::move(origin)); storage_pressure_callback_.Run(std::move(origin));
} }
} }
......
...@@ -420,6 +420,17 @@ class QuotaManagerTest : public testing::Test { ...@@ -420,6 +420,17 @@ class QuotaManagerTest : public testing::Test {
void GetUsage_WithModifyTestBody(const StorageType type); void GetUsage_WithModifyTestBody(const StorageType type);
void SetStoragePressureCallback(
base::RepeatingCallback<void(url::Origin)> callback) {
quota_manager_->SetStoragePressureCallback(std::move(callback));
}
void MaybeRunStoragePressureCallback(const url::Origin& origin,
int64_t total,
int64_t available) {
quota_manager_->MaybeRunStoragePressureCallback(origin, total, available);
}
void set_additional_callback_count(int c) { additional_callback_count_ = c; } void set_additional_callback_count(int c) { additional_callback_count_ = c; }
int additional_callback_count() const { return additional_callback_count_; } int additional_callback_count() const { return additional_callback_count_; }
void DidGetUsageAndQuotaAdditional(QuotaStatusCode status, void DidGetUsageAndQuotaAdditional(QuotaStatusCode status,
...@@ -2670,4 +2681,22 @@ TEST_F(QuotaManagerTest, GetUsageAndQuota_SessionOnly) { ...@@ -2670,4 +2681,22 @@ TEST_F(QuotaManagerTest, GetUsageAndQuota_SessionOnly) {
EXPECT_EQ(0, quota()); EXPECT_EQ(0, quota());
} }
TEST_F(QuotaManagerTest, MaybeRunStoragePressureCallback) {
bool callback_ran = false;
auto cb = base::BindRepeating(
[](bool* callback_ran, url::Origin origin) { *callback_ran = true; },
&callback_ran);
SetStoragePressureCallback(std::move(cb));
int64_t kGBytes = QuotaManager::kMBytes * 1024;
MaybeRunStoragePressureCallback(url::Origin(), 100 * kGBytes, 2 * kGBytes);
task_environment_.RunUntilIdle();
EXPECT_FALSE(callback_ran);
MaybeRunStoragePressureCallback(url::Origin(), 100 * kGBytes, kGBytes);
task_environment_.RunUntilIdle();
EXPECT_TRUE(callback_ran);
}
} // namespace storage } // namespace storage
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment