Commit d0312acf authored by Derek Basehore's avatar Derek Basehore Committed by Commit Bot

Revert "Disable unmount-on-suspend for FUSE"

This reverts commit 436c119d.

Reason for revert: An edge case for platforms without ARC++ was not handled correctly.

Original change's description:
> Disable unmount-on-suspend for FUSE
>
> This removes the code for unmounting the FUSEs (filesystem in userspace)
> on suspend. Instead of unmounting to resolve the FUSE/freeze on suspend
> race condition, we will be using powerd code that orders freeze using
> freezer cgroups.
>
> Crostini still has some work left to do before this can be done for it,
> so unmount on suspend for its FUSE, sshfs, will be done in a later
> patch.
>
> Test: "suspend_stress_test -c 2500" on device
> Bug: b/134792837
> Change-Id: Icb8f9e309f6780e6f935be550c6c4e048a44b1fd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2336869
> Commit-Queue: Derek Basehore <dbasehore@chromium.org>
> Reviewed-by: Joel Hockey <joelhockey@chromium.org>
> Reviewed-by: Sergei Datsenko <dats@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Reviewed-by: David Munro <davidmunro@google.com>
> Cr-Commit-Position: refs/heads/master@{#811950}

TBR=jamescook@chromium.org,achuith@chromium.org,dbasehore@chromium.org,joelhockey@chromium.org,dats@chromium.org,davidmunro@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: b/134792837
Change-Id: Ia50e43dceeb0eea448d6fdf7232af328a9a1fd1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2451400Reviewed-by: default avatarDavid Munro <davidmunro@google.com>
Reviewed-by: default avatarSergei Datsenko <dats@chromium.org>
Reviewed-by: default avatarJoel Hockey <joelhockey@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Commit-Queue: Derek Basehore <dbasehore@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814910}
parent db2d0af3
......@@ -566,7 +566,8 @@ DriveIntegrationService::DriveIntegrationService(
drivefs_holder_(std::make_unique<DriveFsHolder>(
profile_,
this,
std::move(test_drivefs_mojo_listener_factory))) {
std::move(test_drivefs_mojo_listener_factory))),
power_manager_observer_(this) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(profile && !profile->IsOffTheRecord());
......@@ -591,6 +592,10 @@ DriveIntegrationService::DriveIntegrationService(
blocking_task_runner_.get()));
}
// PowerManagerClient is unset in unit tests.
if (chromeos::PowerManagerClient::Get()) {
power_manager_observer_.Add(chromeos::PowerManagerClient::Get());
}
SetEnabled(drive::util::IsDriveEnabledForProfile(profile));
}
......@@ -1023,6 +1028,21 @@ void DriveIntegrationService::PinFiles(
profile_->GetPrefs()->SetBoolean(prefs::kDriveFsPinnedMigrated, true);
}
void DriveIntegrationService::SuspendImminent(
power_manager::SuspendImminent::Reason reason) {
// This may a bit racy since it doesn't prevent suspend until the unmount is
// completed, instead relying on something else to defer suspending long
// enough.
RemoveDriveMountPoint();
}
void DriveIntegrationService::SuspendDone(
const base::TimeDelta& sleep_duration) {
if (is_enabled()) {
AddDriveMountPoint();
}
}
void DriveIntegrationService::GetQuickAccessItems(
int max_number,
GetQuickAccessItemsCallback callback) {
......
......@@ -18,6 +18,7 @@
#include "base/observer_list_types.h"
#include "base/scoped_observer.h"
#include "chromeos/components/drivefs/drivefs_host.h"
#include "chromeos/dbus/power/power_manager_client.h"
#include "components/drive/drive_notification_observer.h"
#include "components/drive/file_errors.h"
#include "components/drive/file_system_core_util.h"
......@@ -90,7 +91,8 @@ class DriveIntegrationServiceObserver : public base::CheckedObserver {
// that are used to integrate Drive to Chrome. The object of this class is
// created per-profile.
class DriveIntegrationService : public KeyedService,
public drivefs::DriveFsHost::MountObserver {
public drivefs::DriveFsHost::MountObserver,
public chromeos::PowerManagerClient::Observer {
public:
class PreferenceWatcher;
using DriveFsMojoListenerFactory = base::RepeatingCallback<
......@@ -269,6 +271,10 @@ class DriveIntegrationService : public KeyedService,
// Pin all the files in |files_to_pin| with DriveFS.
void PinFiles(const std::vector<base::FilePath>& files_to_pin);
// chromeos::PowerManagerClient::Observer overrides:
void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
void SuspendDone(const base::TimeDelta& sleep_duration) override;
void OnGetQuickAccessItems(
GetQuickAccessItemsCallback callback,
drive::FileError error,
......@@ -300,6 +306,10 @@ class DriveIntegrationService : public KeyedService,
base::TimeTicks mount_start_;
ScopedObserver<chromeos::PowerManagerClient,
chromeos::PowerManagerClient::Observer>
power_manager_observer_{this};
// Note: This should remain the last member so it'll be destroyed and
// invalidate its weak pointers before any other members are destroyed.
base::WeakPtrFactory<DriveIntegrationService> weak_ptr_factory_{this};
......
......@@ -182,6 +182,9 @@ SmbService::SmbService(Profile* profile,
SmbService::~SmbService() {
net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
if (chromeos::PowerManagerClient::Get()) {
chromeos::PowerManagerClient::Get()->RemoveObserver(this);
}
}
void SmbService::Shutdown() {
......@@ -814,6 +817,9 @@ void SmbService::CompleteSetup() {
base::Unretained(this))));
RestoreMounts();
net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
if (chromeos::PowerManagerClient::Get()) {
chromeos::PowerManagerClient::Get()->AddObserver(this);
}
if (setup_complete_callback_) {
std::move(setup_complete_callback_).Run();
......@@ -1017,5 +1023,53 @@ void SmbService::RecordMountCount() const {
file_systems.size() + smbfs_shares_.size());
}
void SmbService::SuspendImminent(
power_manager::SuspendImminent::Reason reason) {
for (auto it = smbfs_shares_.begin(); it != smbfs_shares_.end(); ++it) {
SmbFsShare* share = it->second.get();
// For each share, block suspend until the unmount has completed, to ensure
// that no smbfs instances are active when the system goes to sleep.
auto token = base::UnguessableToken::Create();
chromeos::PowerManagerClient::Get()->BlockSuspend(token, "SmbService");
share->Unmount(
base::BindOnce(&SmbService::OnSuspendUnmountDone, AsWeakPtr(), token));
}
}
void SmbService::OnSuspendUnmountDone(
base::UnguessableToken power_manager_suspend_token,
chromeos::MountError result) {
LOG_IF(ERROR, result != chromeos::MountError::MOUNT_ERROR_NONE)
<< "Could not unmount smbfs share during suspension: "
<< static_cast<int>(result);
// Regardless of the outcome, unblock suspension for this share.
chromeos::PowerManagerClient::Get()->UnblockSuspend(
power_manager_suspend_token);
}
void SmbService::SuspendDone(const base::TimeDelta& sleep_duration) {
for (auto it = smbfs_shares_.begin(); it != smbfs_shares_.end(); ++it) {
SmbFsShare* share = it->second.get();
const std::string mount_id = share->mount_id();
// Don't try to reconnect as we race the network stack in getting an IP
// address.
SmbFsShare::MountOptions options = share->options();
options.skip_connect = true;
// Observing power management changes from SmbService allows us to remove
// the share in OnSmbfsMountDone if remount fails.
share->Remount(
options, base::BindOnce(
&SmbService::OnSmbfsMountDone, AsWeakPtr(), mount_id,
base::BindOnce([](SmbMountResult result,
const base::FilePath& mount_path) {
LOG_IF(ERROR, result != SmbMountResult::kSuccess)
<< "Error remounting smbfs share after suspension: "
<< static_cast<int>(result);
})));
}
}
} // namespace smb_client
} // namespace chromeos
......@@ -25,6 +25,7 @@
#include "chrome/browser/chromeos/smb_client/smb_task_queue.h"
#include "chrome/browser/chromeos/smb_client/smbfs_share.h"
#include "chrome/browser/profiles/profile.h"
#include "chromeos/dbus/power/power_manager_client.h"
#include "chromeos/dbus/smb_provider_client.h"
#include "components/keyed_service/core/keyed_service.h"
#include "net/base/network_change_notifier.h"
......@@ -48,6 +49,7 @@ class SmbShareInfo;
// Creates and manages an smb file system.
class SmbService : public KeyedService,
public net::NetworkChangeNotifier::NetworkChangeObserver,
public chromeos::PowerManagerClient::Observer,
public base::SupportsWeakPtr<SmbService> {
public:
using MountResponse = base::OnceCallback<void(SmbMountResult result)>;
......@@ -124,6 +126,10 @@ class SmbService : public KeyedService,
void SetSmbFsMounterCreationCallbackForTesting(
SmbFsShare::MounterCreationCallback callback);
// chromeos::PowerManagerClient::Observer overrides
void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
void SuspendDone(const base::TimeDelta& sleep_duration) override;
private:
friend class SmbServiceTest;
......@@ -164,6 +170,11 @@ class SmbService : public KeyedService,
MountInternalCallback callback,
SmbMountResult result);
// Callback passed to SmbFsShare::Unmount() during a power management
// suspension. Ensures that suspension is blocked until the unmount completes.
void OnSuspendUnmountDone(base::UnguessableToken power_manager_suspend_token,
chromeos::MountError result);
// Retrieves the mount_id for |file_system_info|.
int32_t GetMountId(const ProvidedFileSystemInfo& info) const;
......
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