Change how the quota system computes the total poolsize for temporary storage

storage/browser/quota/quota_manager.cc

Index: storage/browser/quota/quota_manager.cc
diff --git a/storage/browser/quota/quota_manager.cc b/storage/browser/quota/quota_manager.cc
index cb1add8c6d984301c9bba020561a5adce77fbeb2..2cc5a02d3f5fe884af5a17c199b333c65e462fef 100644
--- a/storage/browser/quota/quota_manager.cc
+++ b/storage/browser/quota/quota_manager.cc
@@ -23,6 +23,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "net/base/url_util.h"
@@ -58,7 +59,15 @@ const int64_t kMBytes = 1024 * 1024;
 const int kMinutesInMilliSeconds = 60 * 1000;
 
 const int64_t kReportHistogramInterval = 60 * 60 * 1000;  // 1 hour
-const double kTemporaryQuotaRatioToAvail = 1.0 / 3.0;  // 33%
+const double kTemporaryQuotaRatio = 1.0 / 3.0;  // 33%

[jsbell, 2016/03/23 18:08:28]

While you're here, can you comment on what these are used for?

[michaeln, 2016/04/07 00:32:34]

Done, also moved these values to where they're used so it's more obvious how
they're used

+
+#if defined(OS_ANDROID)
+const uint64_t kOsAccomodation = 250 * kMBytes;

[michaeln, 2016/03/22 22:23:55]

these specific numbers are just placeholders, this should probably be bigger for
android and we probably need something specific for chromecast too, and blimp???

how can we tell if the build target is chromecast

+#elif defined(OS_CHROMEOS)
+const uint64_t kOsAccomodation = 1000 * kMBytes;
+#else
+const uint64_t kOsAccomodation = 10000 * kMBytes;
+#endif
 
 }  // namespace
 
@@ -243,36 +252,6 @@ bool UpdateModifiedTimeOnDBThread(const GURL& origin,
   return database->SetOriginLastModifiedTime(origin, type, modified_time);
 }
 
-int64_t CalculateTemporaryGlobalQuota(int64_t global_limited_usage,
-                                      int64_t available_space) {
-  DCHECK_GE(global_limited_usage, 0);
-  int64_t avail_space = available_space;
-  if (avail_space <
-      std::numeric_limits<int64_t>::max() - global_limited_usage) {
-    // We basically calculate the temporary quota by
-    // [available_space + space_used_for_temp] * kTempQuotaRatio,
-    // but make sure we'll have no overflow.
-    avail_space += global_limited_usage;
-  }
-  int64_t pool_size = avail_space * kTemporaryQuotaRatioToAvail;
-  UMA_HISTOGRAM_MBYTES("Quota.GlobalTemporaryPoolSize", pool_size);
-  return pool_size;
-}
-
-void DispatchTemporaryGlobalQuotaCallback(
-    const QuotaCallback& callback,
-    QuotaStatusCode status,
-    const UsageAndQuota& usage_and_quota) {
-  if (status != kQuotaStatusOk) {
-    callback.Run(status, 0);
-    return;
-  }
-
-  callback.Run(status, CalculateTemporaryGlobalQuota(
-      usage_and_quota.global_limited_usage,
-      usage_and_quota.available_disk_space));
-}
-
 int64_t CalculateQuotaWithDiskSpace(int64_t available_disk_space,
                                     int64_t usage,
                                     int64_t quota) {
@@ -297,12 +276,26 @@ int64_t CalculateQuotaWithDiskSpace(int64_t available_disk_space,
 
 int64_t CalculateTemporaryHostQuota(int64_t host_usage,
                                     int64_t global_quota,
-                                    int64_t global_limited_usage) {
-  DCHECK_GE(global_limited_usage, 0);
-  int64_t host_quota = global_quota / QuotaManager::kPerHostTemporaryPortion;
-  if (global_limited_usage > global_quota)

[michaeln, 2016/03/22 21:40:15]

This used to defend against filling the device.

-    host_quota = std::min(host_quota, host_usage);
-  return host_quota;
+                                    int64_t available_disk_space) {
+  const int64_t desired_quota = global_quota /
+                                    QuotaManager::kPerHostTemporaryPortion;
+
+  // We define "too low" as not enough room for another hosts
+  // nominal desired amount of data. (TODO: what should this be)

[michaeln, 2016/03/22 21:40:15]

~66 MBytes for small andoid devcices and ~66GB for PCs with terrabyte drives,
maybe add clamp this to no more than 10G?

too_low = std::min(computed_too_low, 10GB)

[jsbell, 2016/03/23 18:08:29]

Would the "save a bit of room for non-Chrome usage" calc go in here as well?

[michaeln, 2016/04/07 00:32:34]

This is one of the two places where logic to "save a bit for others" comes into
play. The other place is around |must_remain_available_space| in the
QuotaTemporaryStorageEvictor, it uses 1/10 the volume size for that value.

If chrome is run on an approaching full disk, this check will prevent chrome
from writing data to disk.

+  const int64_t too_low_threshold = desired_quota;
+
+  // If disk space is too low, cap usage at current levels.
+  if (available_disk_space < too_low_threshold)

[michaeln, 2016/03/22 21:40:15]

That defense is replaced with this.

+    return std::min(desired_quota, host_usage);
+
+  // If its close to being too low, cap growth to avoid it getting too low.
+  int64_t remaining_quota = std::min(INT64_C(0), desired_quota - host_usage);

[jsbell, 2016/03/23 18:08:28]

I think this might be clearer as:

return std::min(
  desired_quota,
  host_usage + (available_disk_space - too_low_threshold));

(sanity check that this is the same?)

[michaeln, 2016/04/07 00:32:34]

I think your right. I had thought a variable named 'remaining_quota' might help
explain what its doing but smushing it into a single line helps more. Your
equation looks right, I added the inner std::max to prevent returning a negative
number.

return std::min(
    desired_quota,
    host_usage + std::max(0, available_disk_space - too_low_threshold));

+  if (available_disk_space < remaining_quota + too_low_threshold) {
+    remaining_quota = available_disk_space - too_low_threshold;
+    return host_usage + remaining_quota;
+  }
+
+  return desired_quota;
 }
 
 void DispatchUsageAndQuotaForWebApps(
@@ -318,17 +311,19 @@ void DispatchUsageAndQuotaForWebApps(
     return;
   }
 
-  int64_t usage = usage_and_quota.usage;
-  int64_t quota = usage_and_quota.quota;
+  int64_t host_usage = usage_and_quota.usage;
+  int64_t global_quota = usage_and_quota.quota;
+  int64_t host_quota = global_quota;
 
   if (type == kStorageTypeTemporary && !is_unlimited) {
-    quota = CalculateTemporaryHostQuota(
-        usage, quota, usage_and_quota.global_limited_usage);
+    host_quota = CalculateTemporaryHostQuota(
+        host_usage, global_quota, usage_and_quota.available_disk_space);
   }
 
   if (is_incognito) {
-    quota = std::min(quota, QuotaManager::kIncognitoDefaultQuotaLimit);
-    callback.Run(status, usage, quota);
+    host_quota = std::min(host_quota,
+                          QuotaManager::kIncognitoDefaultQuotaLimit);
+    callback.Run(status, host_usage, host_quota);
     return;
   }
 
@@ -338,14 +333,14 @@ void DispatchUsageAndQuotaForWebApps(
   // the available disk space.
   if (is_unlimited || can_query_disk_size) {
     callback.Run(
-        status, usage,
+        status, host_usage,
         CalculateQuotaWithDiskSpace(
             usage_and_quota.available_disk_space,
-            usage, quota));
+            host_usage, host_quota));
     return;
   }
 
-  callback.Run(status, usage, quota);
+  callback.Run(status, host_usage, host_quota);
 }
 
 }  // namespace
@@ -925,8 +920,6 @@ void QuotaManager::GetUsageAndQuotaForWebApps(
     dispatcher->set_quota(kNoLimit);
   } else {
     if (type == kStorageTypeTemporary) {
-      GetUsageTracker(type)->GetGlobalLimitedUsage(
-          dispatcher->GetGlobalLimitedUsageCallback());
       GetTemporaryGlobalQuota(dispatcher->GetQuotaCallback());
     } else if (type == kStorageTypePersistent) {
       GetPersistentHostQuota(net::GetHostOrSpecFromURL(origin),
@@ -940,8 +933,7 @@ void QuotaManager::GetUsageAndQuotaForWebApps(
   GetUsageTracker(type)->GetHostUsage(net::GetHostOrSpecFromURL(origin),
                                       dispatcher->GetHostUsageCallback());
 
-  if (!is_incognito_ && (unlimited || can_query_disk_size))
-    GetAvailableSpace(dispatcher->GetAvailableSpaceCallback());
+  GetAvailableSpace(dispatcher->GetAvailableSpaceCallback());

[jsbell, 2016/03/23 18:08:29]

Can we avoid this if is_incognito_ like we used to?

[michaeln, 2016/04/07 00:32:34]

that's a good question, the system is currently using GetAvailableSpace() for
the incognito case for some things, i'll have to dig some more? maybe we can use
constants for volume info in the incog case?

 
   dispatcher->WaitForResults(base::Bind(
       &DispatchUsageAndQuotaForWebApps,
@@ -1057,13 +1049,11 @@ void QuotaManager::GetTemporaryGlobalQuota(const QuotaCallback& callback) {
     return;
   }
 
-  UsageAndQuotaCallbackDispatcher* dispatcher =
-      new UsageAndQuotaCallbackDispatcher(this);
-  GetUsageTracker(kStorageTypeTemporary)->
-      GetGlobalLimitedUsage(dispatcher->GetGlobalLimitedUsageCallback());
-  GetAvailableSpace(dispatcher->GetAvailableSpaceCallback());
-  dispatcher->WaitForResults(
-      base::Bind(&DispatchTemporaryGlobalQuotaCallback, callback));
+  db_thread_->PostTask(
+      FROM_HERE,
+      base::Bind(&QuotaManager::CalculateTemporaryPoolSize,
+                 get_volume_info_fn_, profile_path_, callback,
+                 base::ThreadTaskRunnerHandle::Get()));
 }
 
 void QuotaManager::SetTemporaryGlobalOverrideQuota(
@@ -1796,6 +1786,26 @@ void QuotaManager::PostTaskAndReplyWithResultForDBThread(
 }
 
 // static
+void QuotaManager::CalculateTemporaryPoolSize(
+    GetVolumeInfoFn get_volume_info_fn,
+    const base::FilePath& profile_path,
+    const QuotaCallback& callback,
+    const scoped_refptr<base::TaskRunner>& reply_runner) {
+  QuotaStatusCode status_code = kQuotaStatusUnknown;
+  int64_t pool_size = 0;
+  uint64_t available, total;
+  if (get_volume_info_fn(profile_path, &available, &total)) {
+    status_code = kQuotaStatusOk;
+    if (total > kOsAccomodation)
+      total -= kOsAccomodation; // todo: what if total is super small???

[jsbell, 2016/03/23 18:08:28]

Maybe we want a threshold, e.g. os_reserve = min(kOsAccomodation, total * 0.75)
so that on a newfangled device that e.g. we treat as desktop we assume there's
*some* storage?

Also, maybe we should have a related DCHECK so we catch new configurations
during development, e.g. if you're running on a Windows VM with only 8GB disk ?
Then we'd know we need to adjust the calcs/variables.

[michaeln, 2016/04/07 00:32:34]

Done

+    pool_size = static_cast<int64_t>(total * kTemporaryQuotaRatio);
+    UMA_HISTOGRAM_MBYTES("Quota.GlobalTemporaryPoolSize", pool_size);
+  }
+  reply_runner->PostTask(
+      FROM_HERE, base::Bind(callback, status_code, pool_size));
+}
+
+// static
 int64_t QuotaManager::CallGetAmountOfFreeDiskSpace(
     GetVolumeInfoFn get_volume_info_fn,
     const base::FilePath& profile_path) {