Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136023002:
arc: Abort booting ARC if the device is critically low on disk space. (Closed)
|
Created:
4 years, 5 months ago by Shuhei Takahashi Modified:
4 years, 5 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hashimoto+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Abort booting ARC if the device is critically low on disk space.
If the device is too low on disk space (<64MB), ARC aborts booting,
and notification is shown to the user.
BUG=chromium:625923
TEST=components_unittests --gtest_filter='ArcBridgeTest.*'
TEST=chromeos_unittests
TEST=Notification shows up on login when free disk space is 40MB.
Committed: https://crrev.com/1821ddc81c38b0fbb42d9e1c0e59e42afa39b327
Cr-Commit-Position: refs/heads/master@{#406218}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Rebased. #
Total comments: 10
Patch Set 4 : Address hidehiko's comments + update mock_cryptohome_client. #Patch Set 5 : . #
Total comments: 3
Patch Set 6 : Call SysInfo::AmountOfFreeDiskSpace() directly. #Patch Set 7 : . #
Total comments: 4
Patch Set 8 : Style fixes as per comments. #
Messages
Total messages: 48 (29 generated)
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. BUG=chromium:625923 TEST=TODO ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ==========
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ==========
nya@chromium.org changed reviewers: + dkrahn@chromium.org, hidehiko@chromium.org
hidehiko: PTAL for ARC changes dkrahn: PTAL for chromeos/dbus Note that this needs roll of third_party/cros_system_api.
On 2016/07/13 08:20:29, Shuhei Takahashi wrote: > hidehiko: PTAL for ARC changes > > dkrahn: PTAL for chromeos/dbus > > > Note that this needs roll of third_party/cros_system_api. chromeos/dbus lgtm
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ==========
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ==========
hidehiko: PTAL PS3: Rebased to ToT. Darren, thanks for quick review!
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
My initial scan. The bot failure looks the real mistake. Could you take a look into it, too? https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_boot_error_notification.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.cc:33: LowDiskSpaceErrorNotificationDelegate() {} = default? https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.cc:42: ~LowDiskSpaceErrorNotificationDelegate() override {} = default? https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_boot_error_notification.h (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.h:14: class ArcBootErrorNotification : public ArcService, Could you briefly summarize what is the responsibility of this class? https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:50: arc_service_manager_->AddService(base::MakeUnique<ArcBootErrorNotification>( I think this is listed in lexicographical order? https://codereview.chromium.org/2136023002/diff/40001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:996: return; isn't it necessary to invoke callback.Run(FAILURE, ...); ? Some methods around here do, and some other don't. I cannot figure out the difference of them, though...
One more request. Could you measure the impact for the boot time?
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ==========
msw: PTAL for mock_cryptohome_client.h (new file) hidehiko: PTAL I measured timing and, to my surprise, I found that this change adds ~100ms to ARC boot time. It is sorely due to D-Bus method call cost; the actual code in cryptohome to check disk usage takes <1ms. We may want to minimize D-Bus method calls by putting disk space checking logic in session_manager side. However, error reporting from session_manager to Chrome is not yet implemented (only success/failure is returned). Considering that we want to merge this change in M53, I propose submitting this change as-is for now, and moving the logic to session_manager later. I filed crbug.com/628124 to track this. https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_boot_error_notification.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.cc:33: LowDiskSpaceErrorNotificationDelegate() {} On 2016/07/14 07:39:40, hidehiko wrote: > = default? Done. https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.cc:42: ~LowDiskSpaceErrorNotificationDelegate() override {} On 2016/07/14 07:39:40, hidehiko wrote: > = default? Done. https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_boot_error_notification.h (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_boot_error_notification.h:14: class ArcBootErrorNotification : public ArcService, On 2016/07/14 07:39:40, hidehiko wrote: > Could you briefly summarize what is the responsibility of this class? Done. https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:50: arc_service_manager_->AddService(base::MakeUnique<ArcBootErrorNotification>( On 2016/07/14 07:39:40, hidehiko wrote: > I think this is listed in lexicographical order? Well, mostly :) Done. https://codereview.chromium.org/2136023002/diff/40001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2136023002/diff/40001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:996: return; On 2016/07/14 07:39:40, hidehiko wrote: > isn't it necessary to invoke callback.Run(FAILURE, ...); ? > Some methods around here do, and some other don't. > I cannot figure out the difference of them, though... Yup, I think that's good to call failure callback. Done.
dkrahn: PTAL for mock_cryptohome_client.h (new file) hidehiko: PTAL Oops, I meant dkrahn, not msw. I'm sorry.
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One more comment (very sorry, I overlooked it in the previous iteration...). https://codereview.chromium.org/2136023002/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2136023002/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:207: DCHECK(thread_checker_.CalledOnValidThread()); Could you check if Stop() is called between start() and OnDiscSpaceChecked()? If so, please not to continue the bootstrap procedure. cf) OnSocketCreated().
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
https://codereview.chromium.org/2136023002/diff/80001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2136023002/diff/80001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:166: virtual void GetFreeDiskSpace(const GetFreeDiskSpaceCallback& callback) = 0; Why do we need a DBus message to cryptohome to call statfs/statvfs ? Is that somehow disallowed? It should also be possible to call that from I/O thread?
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. Depends on following changes in Chromium OS: - cryptohome: DBus method to return free disk space. https://chromium-review.googlesource.com/#/c/359479 - Add a constant for GetFreeDiskSpace(). https://chromium-review.googlesource.com/#/c/359147 And Chromium DEPS roll: - Roll src/third_party/cros_system_api/ 2cf87623e..c74bf51d2 (1 commit). https://codereview.chromium.org/2144733006/ BUG=chromium:625923 TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ==========
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=components_unittests --gtest_filter='ArcBridgeTest.*' TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ==========
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hidehiko: PTAL Thanks for pointing out we can check disk directly in Chrome browser process. I just followed the way current low disk warning logic did (registering to Cryptohome signals) but it was overkill. https://codereview.chromium.org/2136023002/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2136023002/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:207: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/07/14 14:10:26, hidehiko wrote: > Could you check if Stop() is called between start() and OnDiscSpaceChecked()? > If so, please not to continue the bootstrap procedure. > > cf) OnSocketCreated(). Done.
BTW, regarding boot time: I measured the timing between ArcBridgeBootstrapImpl::Start() and ArcBridgeBootstrapImpl::OnDiskSpaceChecked() again, and it turned out it still takes ~100ms. It seems to me this is due to context switch cost on overloaded user session start.
On 2016/07/15 06:09:38, Shuhei Takahashi wrote: > BTW, regarding boot time: I measured the timing between > ArcBridgeBootstrapImpl::Start() and ArcBridgeBootstrapImpl::OnDiskSpaceChecked() > again, and it turned out it still takes ~100ms. It seems to me this is due to > context switch cost on overloaded user session start. More detailed breakdown: - PostTaskAndReplyWithResult -> CheckDiskSpace: 0ms - SysInfo::AmountOfFreeDiskSpace: 1ms - CheckDiskSpace -> OnDiskSpaceChecked: ~100ms So I guess the UI thread is too busy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with minor style comments. https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:135: static int64_t CheckDiskSpace(); Optional: how about "GetFreeDiskSpace()"? "Check" is sometimes used for a function like a validation function, but yours looks a kind of getter? An alternative is just using base::Bind PostTaskAndReplyWithResult( .... base::Bind(AmountOfFreeDiskSpace, FilePath(kDiskCheckPath)), ...); Up to you. https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:221: } else if (disk_free_bytes < kCriticalDiskFreeBytes) { return; } if (...) { : return; } to standardize the style in this file?
Thanks for reviewing! https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:135: static int64_t CheckDiskSpace(); On 2016/07/15 14:47:25, hidehiko wrote: > Optional: how about "GetFreeDiskSpace()"? > "Check" is sometimes used for a function like a validation function, but yours > looks a kind of getter? > > An alternative is just using base::Bind > > PostTaskAndReplyWithResult( > .... > base::Bind(AmountOfFreeDiskSpace, FilePath(kDiskCheckPath)), > ...); > > Up to you. Good point that we can just bind to AmountOfFreeDiskSpace! Changed accordingly. https://codereview.chromium.org/2136023002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:221: } else if (disk_free_bytes < kCriticalDiskFreeBytes) { On 2016/07/15 14:47:25, hidehiko wrote: > return; > } > if (...) { > : > return; > } > > to standardize the style in this file? Done.
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dkrahn@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2136023002/#ps140001 (title: "Style fixes as per comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=components_unittests --gtest_filter='ArcBridgeTest.*' TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=components_unittests --gtest_filter='ArcBridgeTest.*' TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=components_unittests --gtest_filter='ArcBridgeTest.*' TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. ========== to ========== arc: Abort booting ARC if the device is critically low on disk space. If the device is too low on disk space (<64MB), ARC aborts booting, and notification is shown to the user. BUG=chromium:625923 TEST=components_unittests --gtest_filter='ArcBridgeTest.*' TEST=chromeos_unittests TEST=Notification shows up on login when free disk space is 40MB. Committed: https://crrev.com/1821ddc81c38b0fbb42d9e1c0e59e42afa39b327 Cr-Commit-Position: refs/heads/master@{#406218} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1821ddc81c38b0fbb42d9e1c0e59e42afa39b327 Cr-Commit-Position: refs/heads/master@{#406218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
