Open partitions with O_DSYNC flag only if the update is periodic.
Currently when updating we always open the target partition with flag O_DSYNC
(CL:562552), but this makes all infrastructure operations like 'cros flash',
provisioning, force update, paygen, etc much slower. This changes the update
engine to only add O_DSYNC flag if an update is triggered by periodic checks
(not interactively forced). This means if the user clicks on 'check for update'
it will be an interactive update and O_DSYNC will not be used. This change keeps
the AOSP partitions open without O_DSYNC flag. This CL uses non-interactive mode
for all unit tests but currently there are no integration test like provisioning
for triggering periodic updates.
Currently 'parrot' board canaries (only board with rotating HDD) is failing due
to timeouts related to slow updates. This CL potentially will clear that problem.
TEST=cros_workon_make --test, installed an image with/out the O_DSYCN flag and
measured the 'cros flash' time.
BUG=chromium:738027
Change-Id: If45fcf5e798b9c9353e09021ad812c859d983a65
Reviewed-on: https://chromium-review.googlesource.com/567360
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 01db96a..a21a9b0 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -355,7 +355,14 @@
target_path_ = install_plan_->partitions[current_partition_].target_path;
int err;
- target_fd_ = OpenFile(target_path_.c_str(), O_RDWR | O_DSYNC, &err);
+ int flags = O_RDWR;
+ if (!is_interactive_)
+ flags |= O_DSYNC;
+
+ LOG(INFO) << "Opening " << target_path_ << " partition with"
+ << (is_interactive_ ? "out" : "") << " O_DSYNC";
+
+ target_fd_ = OpenFile(target_path_.c_str(), flags, &err);
if (!target_fd_) {
LOG(ERROR) << "Unable to open target partition "
<< partition.partition_name() << " on slot "
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 71d7178..038b260 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -78,12 +78,14 @@
BootControlInterface* boot_control,
HardwareInterface* hardware,
DownloadActionDelegate* download_delegate,
- InstallPlan* install_plan)
+ InstallPlan* install_plan,
+ bool is_interactive)
: prefs_(prefs),
boot_control_(boot_control),
hardware_(hardware),
download_delegate_(download_delegate),
- install_plan_(install_plan) {}
+ install_plan_(install_plan),
+ is_interactive_(is_interactive) {}
// FileWriter's Write implementation where caller doesn't care about
// error codes.
@@ -385,6 +387,9 @@
// The last progress chunk recorded.
unsigned last_progress_chunk_{0};
+ // If |true|, the update is user initiated (vs. periodic update checks).
+ bool is_interactive_{false};
+
// The timeout after which we should force emitting a progress log (constant),
// and the actual point in time for the next forced log to be emitted.
const base::TimeDelta forced_progress_log_wait_{
diff --git a/payload_consumer/delta_performer_integration_test.cc b/payload_consumer/delta_performer_integration_test.cc
index afbb8dc..6fdaba5 100644
--- a/payload_consumer/delta_performer_integration_test.cc
+++ b/payload_consumer/delta_performer_integration_test.cc
@@ -746,7 +746,8 @@
&state->fake_boot_control_,
&state->fake_hardware_,
&state->mock_delegate_,
- install_plan);
+ install_plan,
+ false /* is_interactive */);
string public_key_path = GetBuildArtifactsPath(kUnittestPublicKeyPath);
EXPECT_TRUE(utils::FileExists(public_key_path.c_str()));
(*performer)->set_public_key_path(public_key_path);
diff --git a/payload_consumer/delta_performer_unittest.cc b/payload_consumer/delta_performer_unittest.cc
index 12d0708..a326ae4 100644
--- a/payload_consumer/delta_performer_unittest.cc
+++ b/payload_consumer/delta_performer_unittest.cc
@@ -328,8 +328,12 @@
FakeBootControl fake_boot_control_;
FakeHardware fake_hardware_;
MockDownloadActionDelegate mock_delegate_;
- DeltaPerformer performer_{
- &prefs_, &fake_boot_control_, &fake_hardware_, &mock_delegate_, &install_plan_};
+ DeltaPerformer performer_{&prefs_,
+ &fake_boot_control_,
+ &fake_hardware_,
+ &mock_delegate_,
+ &install_plan_,
+ false /* is_interactive*/};
};
TEST_F(DeltaPerformerTest, FullPayloadWriteTest) {
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index b2405ef..43812d7 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -44,12 +44,14 @@
BootControlInterface* boot_control,
HardwareInterface* hardware,
SystemState* system_state,
- HttpFetcher* http_fetcher)
+ HttpFetcher* http_fetcher,
+ bool is_interactive)
: prefs_(prefs),
boot_control_(boot_control),
hardware_(hardware),
system_state_(system_state),
http_fetcher_(http_fetcher),
+ is_interactive_(is_interactive),
writer_(nullptr),
code_(ErrorCode::kSuccess),
delegate_(nullptr),
@@ -188,8 +190,12 @@
if (writer_) {
LOG(INFO) << "Using writer for test.";
} else {
- delta_performer_.reset(new DeltaPerformer(
- prefs_, boot_control_, hardware_, delegate_, &install_plan_));
+ delta_performer_.reset(new DeltaPerformer(prefs_,
+ boot_control_,
+ hardware_,
+ delegate_,
+ &install_plan_,
+ is_interactive_));
writer_ = delta_performer_.get();
}
download_active_ = true;
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index 285930a..618a5b9 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -72,12 +72,13 @@
// Takes ownership of the passed in HttpFetcher. Useful for testing.
// A good calling pattern is:
// DownloadAction(prefs, boot_contol, hardware, system_state,
- // new WhateverHttpFetcher);
+ // new WhateverHttpFetcher, false);
DownloadAction(PrefsInterface* prefs,
BootControlInterface* boot_control,
HardwareInterface* hardware,
SystemState* system_state,
- HttpFetcher* http_fetcher);
+ HttpFetcher* http_fetcher,
+ bool is_interactive);
~DownloadAction() override;
// InstallPlanAction overrides.
@@ -145,6 +146,11 @@
// Pointer to the HttpFetcher that does the http work.
std::unique_ptr<HttpFetcher> http_fetcher_;
+ // If |true|, the update is user initiated (vs. periodic update checks). Hence
+ // the |delta_performer_| can decide not to use O_DSYNC flag for faster
+ // update.
+ bool is_interactive_;
+
// The FileWriter that downloaded data should be written to. It will
// either point to *decompressing_file_writer_ or *delta_performer_.
FileWriter* writer_;
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 5e9ef5c..ae85cb5 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -165,7 +165,8 @@
fake_system_state.boot_control(),
fake_system_state.hardware(),
&fake_system_state,
- http_fetcher);
+ http_fetcher,
+ false /* is_interactive */);
download_action.SetTestFileWriter(&writer);
BondActions(&feeder_action, &download_action);
MockDownloadActionDelegate download_delegate;
@@ -280,7 +281,8 @@
fake_system_state_.boot_control(),
fake_system_state_.hardware(),
&fake_system_state_,
- new MockHttpFetcher(data.data(), data.size(), nullptr));
+ new MockHttpFetcher(data.data(), data.size(), nullptr),
+ false /* is_interactive */);
download_action.SetTestFileWriter(&writer);
MockDownloadActionDelegate download_delegate;
if (use_download_delegate) {
@@ -380,7 +382,8 @@
fake_system_state_.boot_control(),
fake_system_state_.hardware(),
&fake_system_state_,
- new MockHttpFetcher("x", 1, nullptr));
+ new MockHttpFetcher("x", 1, nullptr),
+ false /* is_interactive */);
download_action.SetTestFileWriter(&writer);
DownloadActionTestAction test_action;
@@ -468,7 +471,8 @@
fake_system_state_.boot_control(),
fake_system_state_.hardware(),
&fake_system_state_,
- http_fetcher_));
+ http_fetcher_,
+ false /* is_interactive */));
download_action_->SetTestFileWriter(&writer);
BondActions(&feeder_action, download_action_.get());
DownloadActionTestProcessorDelegate delegate(ErrorCode::kSuccess);