New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 741019 link

Starred by 7 users

Issue metadata

Status: Verified
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 760007



Sign in to add a comment

session_manager: Don't write policy to disk at shutdown

Project Member Reported by tnagel@chromium.org, Jul 11 2017

Issue description

session_manager writes policy to disk upon shutdown. (Look for "Persisted policy to disk." log messages.)

These writes are unnecessary (policy is written already when it is received) and slow (using fsync()). This is slowing down Chrome OS shutdown and should be removed.
 
Owner: igorcov@chromium.org
Status: Assigned (was: Untriaged)
I'm working on making writes to disk only when the policy change is detected.
Status: Started (was: Assigned)
Labels: M-61
Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Blocking: 760007
That will also reduce unenrollement when we have a fs corruption, if the policy is not updated while the corruption happens.

Comment 6 by puthik@chromium.org, Sep 11 2017

Labels: -Pri-3 M-60 Merge-Request-62 Merge-Request-61 M-62 Merge-Request-60 Pri-1
I think this should mitigate  Issue 760007 

Add merge request.
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 11 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 8 by puthik@chromium.org, Sep 11 2017

Cc: gwendal@chromium.org bhthompson@chromium.org josa...@chromium.org puthik@chromium.org
Labels: -M-62 -Merge-Review-61 -Merge-Request-62
This actually already in M61

Change tag to just 60.

The CL is here
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/567510
Labels: -Merge-Request-60 Merge-Approved-60
Approved for M60
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2c31d5d9edcbdf3434a41b776e29128b1570aab0

commit 2c31d5d9edcbdf3434a41b776e29128b1570aab0
Author: Igor <igorcov@chromium.org>
Date: Fri Jul 14 02:46:19 2017

login: Avoid persisting policy data when no change detected.

When device is shut down, SessionManagerImpl::Finalize is called
as the session is destroyed. The method calls PolicyStore::Persist
to save the policy data.

In this CL the policy data is cached and is persisted to disk
only in case the data is changed. This doesn't avoid persisting
when policy fetch is done because the timestamp is included in
the data, but allows to avoid the writing to disk at shut down.

BUG= chromium:741019 
TEST=Manual

Change-Id: Idde3cb666fe6e18b778dad4b5d732c88d32d4fac
Reviewed-on: https://chromium-review.googlesource.com/567510
Commit-Ready: Igor <igorcov@chromium.org>
Tested-by: Igor <igorcov@chromium.org>
Reviewed-by: Thiemo Nagel <tnagel@chromium.org>

[modify] https://crrev.com/2c31d5d9edcbdf3434a41b776e29128b1570aab0/login_manager/policy_service.cc
[modify] https://crrev.com/2c31d5d9edcbdf3434a41b776e29128b1570aab0/login_manager/policy_store.h
[modify] https://crrev.com/2c31d5d9edcbdf3434a41b776e29128b1570aab0/login_manager/policy_store.cc

Labels: -Merge-Approved-60 Merge-Merged
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 11 2017

Labels: merge-merged-release-R60-9592.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2e5303c448021b96bf4b281524194339e401c4a7

commit 2e5303c448021b96bf4b281524194339e401c4a7
Author: Igor <igorcov@chromium.org>
Date: Mon Sep 11 18:06:19 2017

login: Avoid persisting policy data when no change detected.

When device is shut down, SessionManagerImpl::Finalize is called
as the session is destroyed. The method calls PolicyStore::Persist
to save the policy data.

In this CL the policy data is cached and is persisted to disk
only in case the data is changed. This doesn't avoid persisting
when policy fetch is done because the timestamp is included in
the data, but allows to avoid the writing to disk at shut down.

BUG= chromium:741019 
TEST=Manual

Change-Id: Idde3cb666fe6e18b778dad4b5d732c88d32d4fac
Reviewed-on: https://chromium-review.googlesource.com/567510
Commit-Ready: Igor <igorcov@chromium.org>
Tested-by: Igor <igorcov@chromium.org>
Reviewed-by: Thiemo Nagel <tnagel@chromium.org>
(cherry picked from commit 2c31d5d9edcbdf3434a41b776e29128b1570aab0)
Reviewed-on: https://chromium-review.googlesource.com/661021
Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org>
Commit-Queue: Puthikorn Voravootivat <puthik@chromium.org>
Tested-by: Puthikorn Voravootivat <puthik@chromium.org>

[modify] https://crrev.com/2e5303c448021b96bf4b281524194339e401c4a7/login_manager/policy_service.cc
[modify] https://crrev.com/2e5303c448021b96bf4b281524194339e401c4a7/login_manager/policy_store.h
[modify] https://crrev.com/2e5303c448021b96bf4b281524194339e401c4a7/login_manager/policy_store.cc

As verified in M60.0.3112.114 9592.94.0 stable caroline, "Persisted policy to disk" was not seen during shutdown event in /var/log/messages.

Sign in to add a comment