Merge fuse unmount paths to reduce fuse unmount total time
Previously, we were waiting unnecessarily for long times trying to unmount
different fuse paths with no luck at the end. Made sure that we merge
these unmount flows so that we don't sleep separately for each unmount.
Additionally, added polling logic to not sleep the whole duration
without trying to unmount. Further more, made sure that we try to kill
apps using storage/emulated when unmounting fuse paths.
More details about the change can be found here go/mount-performance-improvement
Test: atest AdoptableHostTest, atest StorageManagerTest
Bug: 318335297
Bug: 402367661
Bug: 369519866
Flag: android.vold.flags.enhance_fuse_unmount
Change-Id: Id8af00fd1bff1de4078e4acd148a918738b4cea6
diff --git a/Process.cpp b/Process.cpp
index 426a425..0115fb1 100644
--- a/Process.cpp
+++ b/Process.cpp
@@ -46,7 +46,7 @@
namespace android {
namespace vold {
-static bool checkMaps(const std::string& path, const std::string& prefix) {
+static bool checkMaps(const std::string& path, const std::vector<std::string>& prefixes) {
bool found = false;
auto file = std::unique_ptr<FILE, decltype(&fclose)>{fopen(path.c_str(), "re"), fclose};
if (!file) {
@@ -60,9 +60,14 @@
std::string::size_type pos = line.find('/');
if (pos != std::string::npos) {
line = line.substr(pos);
- if (android::base::StartsWith(line, prefix)) {
- LOG(WARNING) << "Found map " << path << " referencing " << line;
- found = true;
+ for (const auto& prefix : prefixes) {
+ if (android::base::StartsWith(line, prefix)) {
+ LOG(WARNING) << "Found map " << path << " referencing " << line;
+ found = true;
+ break;
+ }
+ }
+ if (found) {
break;
}
}
@@ -72,12 +77,14 @@
return found;
}
-static bool checkSymlink(const std::string& path, const std::string& prefix) {
+static bool checkSymlink(const std::string& path, const std::vector<std::string>& prefixes) {
std::string res;
if (android::base::Readlink(path, &res)) {
- if (android::base::StartsWith(res, prefix)) {
- LOG(WARNING) << "Found symlink " << path << " referencing " << res;
- return true;
+ for (const auto& prefix : prefixes) {
+ if (android::base::StartsWith(res, prefix)) {
+ LOG(WARNING) << "Found symlink " << path << " referencing " << res;
+ return true;
+ }
}
}
return false;
@@ -129,7 +136,8 @@
return pids.size();
}
-int KillProcessesWithOpenFiles(const std::string& prefix, int signal, bool killFuseDaemon) {
+int KillProcessesWithOpenFiles(const std::vector<std::string>& prefixes, int signal,
+ bool killFuseDaemon) {
std::unordered_set<pid_t> pids;
auto proc_d = std::unique_ptr<DIR, int (*)(DIR*)>(opendir("/proc"), closedir);
@@ -148,10 +156,10 @@
// Look for references to prefix
bool found = false;
auto path = StringPrintf("/proc/%d", pid);
- found |= checkMaps(path + "/maps", prefix);
- found |= checkSymlink(path + "/cwd", prefix);
- found |= checkSymlink(path + "/root", prefix);
- found |= checkSymlink(path + "/exe", prefix);
+ found |= checkMaps(path + "/maps", prefixes);
+ found |= checkSymlink(path + "/cwd", prefixes);
+ found |= checkSymlink(path + "/root", prefixes);
+ found |= checkSymlink(path + "/exe", prefixes);
auto fd_path = path + "/fd";
auto fd_d = std::unique_ptr<DIR, int (*)(DIR*)>(opendir(fd_path.c_str()), closedir);
@@ -161,7 +169,7 @@
struct dirent* fd_de;
while ((fd_de = readdir(fd_d.get())) != nullptr) {
if (fd_de->d_type != DT_LNK) continue;
- found |= checkSymlink(fd_path + "/" + fd_de->d_name, prefix);
+ found |= checkSymlink(fd_path + "/" + fd_de->d_name, prefixes);
}
}
@@ -198,5 +206,10 @@
return totalKilledPids;
}
+int KillProcessesWithOpenFiles(const std::string& prefix, int signal, bool killFuseDaemon) {
+ return KillProcessesWithOpenFiles(std::vector<std::string>(1, prefix), signal,
+ killFuseDaemon);
+}
+
} // namespace vold
} // namespace android
diff --git a/Process.h b/Process.h
index f3728b5..8a20d1c 100644
--- a/Process.h
+++ b/Process.h
@@ -20,6 +20,8 @@
namespace android {
namespace vold {
+int KillProcessesWithOpenFiles(const std::vector<std::string>& paths, int signal,
+ bool killFuseDaemon = true);
int KillProcessesWithOpenFiles(const std::string& path, int signal, bool killFuseDaemon = true);
int KillProcessesWithTmpfsMounts(const std::string& path, int signal);
diff --git a/Utils.cpp b/Utils.cpp
index c4070d1..9ad828c 100644
--- a/Utils.cpp
+++ b/Utils.cpp
@@ -1724,6 +1724,139 @@
return result;
}
+/* returns list of non unmounted paths */
+std::vector<std::string> UnmountFusePaths(const std::vector<std::string>& paths_to_unmount) {
+ std::vector<std::string> non_unmounted_paths;
+ for (const auto& path : paths_to_unmount) {
+ LOG(INFO) << "Unmounting fuse path " << path;
+ const char* cpath = path.c_str();
+ if (!umount2(cpath, UMOUNT_NOFOLLOW) || errno == EINVAL || errno == ENOENT) {
+ rmdir(cpath);
+ continue;
+ }
+ non_unmounted_paths.push_back(path);
+ }
+ return non_unmounted_paths;
+}
+
+/* returns list of non unmounted paths */
+std::vector<std::string> UnmountFusePathsWithSleepAndPolling(
+ const std::vector<std::string>& paths_to_unmount) {
+ std::vector<std::string> non_unmounted_paths = paths_to_unmount;
+
+ int count = 10;
+ while (count-- > 0) {
+ usleep(500 * 1000);
+ non_unmounted_paths = UnmountFusePaths(non_unmounted_paths);
+ if (non_unmounted_paths.empty()) {
+ return non_unmounted_paths;
+ }
+ }
+ return non_unmounted_paths;
+}
+
+/* returns list of non unmounted paths */
+std::vector<std::string> KillProcessesWithFuseOpenFilesAndUnmount(
+ const std::vector<std::string>& paths_to_unmount, const std::string& absolute_upper_path,
+ int signal, bool kill_fuse_daemon, bool force_sleep_if_no_processes_killed) {
+
+ // In addition to killing apps using paths to unmount, we need to kill aps using
+ // the upper path (e.g storage/emulated) As they would prevent unmounting fuse
+ std::vector<std::string> paths_to_kill(paths_to_unmount);
+ paths_to_kill.push_back(absolute_upper_path);
+
+ int total_killed_pids = KillProcessesWithOpenFiles(paths_to_kill, signal, kill_fuse_daemon);
+
+ if (sSleepOnUnmount && (force_sleep_if_no_processes_killed || total_killed_pids)) {
+ return UnmountFusePathsWithSleepAndPolling(paths_to_unmount);
+ }
+ return UnmountFusePaths(paths_to_unmount);
+}
+
+status_t UnmountUserFuseEnhanced(userid_t user_id, const std::string& absolute_lower_path,
+ const std::string& relative_upper_path,
+ const std::string& absolute_upper_path,
+ const std::vector<std::string>& bind_mount_paths) {
+ std::vector<std::string> paths_to_unmount(bind_mount_paths);
+
+ std::string fuse_path(StringPrintf("/mnt/user/%d/%s", user_id, relative_upper_path.c_str()));
+ paths_to_unmount.push_back(fuse_path);
+
+ std::string pass_through_path(
+ StringPrintf("/mnt/pass_through/%d/%s", user_id, relative_upper_path.c_str()));
+ paths_to_unmount.push_back(pass_through_path);
+
+ auto start_time = std::chrono::steady_clock::now();
+ LOG(INFO) << "Unmounting fuse paths";
+
+ // Try unmounting without killing any processes
+ paths_to_unmount = UnmountFusePaths(paths_to_unmount);
+ if (paths_to_unmount.empty()) {
+ return android::OK;
+ }
+
+ // Kill processes except for FuseDaemon holding references to fuse paths with SIGINT
+ // And try to unmount afterwards with sleep and polling mechanism
+ paths_to_unmount = KillProcessesWithFuseOpenFilesAndUnmount(
+ paths_to_unmount, absolute_upper_path, SIGINT, /*kill_fuse_daemon*/ false,
+ /*force_sleep_if_no_processes_killed*/false);
+ if (paths_to_unmount.empty()) {
+ return android::OK;
+ }
+
+ // Kill processes except for FuseDaemon holding references to fuse paths with SIGTERM
+ // And try to unmount afterwards with sleep and polling mechanism
+ paths_to_unmount = KillProcessesWithFuseOpenFilesAndUnmount(
+ paths_to_unmount, absolute_upper_path, SIGTERM, /*kill_fuse_daemon*/ false,
+ /*force_sleep_if_no_processes_killed*/false);
+
+ if (paths_to_unmount.empty()) {
+ return android::OK;
+ }
+
+ auto now = std::chrono::steady_clock::now();
+ auto time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - start_time);
+ bool force_sleep_if_no_process_killed = time_elapsed < std::chrono::milliseconds(5000);
+ // Kill processes except for FuseDaemon holding references to fuse paths with SIGKILL
+ // And try to unmount afterwards with sleep and polling mechanism
+ // intentionally force sleep if sSleepOnUnmount isn't set to false
+ // and if we haven't slept in previous retries so that we give MediaProvider time
+ // to release FDs prior to try killing it in the next step
+ paths_to_unmount = KillProcessesWithFuseOpenFilesAndUnmount(
+ paths_to_unmount, absolute_upper_path, SIGKILL, /*kill_fuse_daemon*/ false,
+ force_sleep_if_no_process_killed);
+
+ if (paths_to_unmount.empty()) {
+ return android::OK;
+ }
+
+ // Kill processes including FuseDaemon holding references to fuse paths with SIGKILL
+ // And try to unmount afterwards with sleep and polling mechanism
+ paths_to_unmount = KillProcessesWithFuseOpenFilesAndUnmount(
+ paths_to_unmount, absolute_upper_path, SIGKILL, /*kill_fuse_daemon*/ true,
+ /*force_sleep_if_no_processes_killed*/false);
+ if (paths_to_unmount.empty()) {
+ return android::OK;
+ }
+
+ // If we reached here, then it means that previous kill and unmount retries didn't succeed
+ // Try to unmount with MNT_DETACH so we try lazily unmount
+ android::status_t result = android::OK;
+ for (const auto& path : paths_to_unmount) {
+ LOG(ERROR) << "Failed to unmount. Trying MNT_DETACH " << path;
+ const char* cpath = path.c_str();
+ if (!umount2(cpath, UMOUNT_NOFOLLOW | MNT_DETACH) || errno == EINVAL || errno == ENOENT) {
+ rmdir(cpath);
+ continue;
+ }
+ PLOG(ERROR) << "Failed to unmount with MNT_DETACH " << path;
+ if (path == fuse_path) {
+ result = -errno;
+ }
+ }
+ return result;
+}
+
status_t PrepareAndroidDirs(const std::string& volumeRoot) {
std::string androidDir = volumeRoot + kAndroidDir;
std::string androidDataDir = volumeRoot + kAppDataDir;
diff --git a/Utils.h b/Utils.h
index 0eca902..8296ef8 100644
--- a/Utils.h
+++ b/Utils.h
@@ -205,6 +205,10 @@
status_t UnmountUserFuse(userid_t userId, const std::string& absolute_lower_path,
const std::string& relative_upper_path);
+status_t UnmountUserFuseEnhanced(userid_t userId, const std::string& absolute_lower_path,
+ const std::string& relative_upper_path,
+ const std::string& absolute_upper_path,
+ const std::vector<std::string>& bind_mount_paths = {});
status_t PrepareAndroidDirs(const std::string& volumeRoot);
diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp
index 270dcd4..519453d 100644
--- a/model/EmulatedVolume.cpp
+++ b/model/EmulatedVolume.cpp
@@ -36,8 +36,11 @@
#include <sys/sysmacros.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <android_vold_flags.h>
using android::base::StringPrintf;
+namespace flags = android::vold::flags;
+
namespace android {
namespace vold {
@@ -428,9 +431,17 @@
auto fuse_unmounter = [&]() {
LOG(INFO) << "fuse_unmounter scope_guard running";
fd.reset();
- if (UnmountUserFuse(user_id, getInternalPath(), label) != OK) {
- PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
+ if (flags::enhance_fuse_unmount()) {
+ std::string user_path(StringPrintf("%s/%d", getPath().c_str(), getMountUserId()));
+ if (UnmountUserFuseEnhanced(user_id, getInternalPath(), label, user_path) != OK) {
+ PLOG(INFO) << "UnmountUserFuseEnhanced failed on emulated fuse volume";
+ }
+ } else {
+ if (UnmountUserFuse(user_id, getInternalPath(), label) != OK) {
+ PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
+ }
}
+
mFuseMounted = false;
};
auto fuse_guard = android::base::make_scope_guard(fuse_unmounter);
@@ -486,21 +497,22 @@
status_t EmulatedVolume::doUnmount() {
int userId = getMountUserId();
- // Kill all processes using the filesystem before we unmount it. If we
- // unmount the filesystem first, most file system operations will return
- // ENOTCONN until the unmount completes. This is an exotic and unusual
- // error code and might cause broken behaviour in applications.
if (mFuseMounted) {
- // For FUSE specifically, we have an emulated volume per user, so only kill
- // processes using files from this particular user.
std::string user_path(StringPrintf("%s/%d", getPath().c_str(), getMountUserId()));
- LOG(INFO) << "Killing all processes referencing " << user_path;
- KillProcessesUsingPath(user_path);
- } else {
- KillProcessesUsingPath(getPath());
- }
- if (mFuseMounted) {
+ // We don't kill processes before trying to unmount in case enhance_fuse_unmount enabled
+ // As we make sure to kill processes if needed if unmounting failed
+ if (!flags::enhance_fuse_unmount()) {
+ // Kill all processes using the filesystem before we unmount it. If we
+ // unmount the filesystem first, most file system operations will return
+ // ENOTCONN until the unmount completes. This is an exotic and unusual
+ // error code and might cause broken behaviour in applications.
+ // For FUSE specifically, we have an emulated volume per user, so only kill
+ // processes using files from this particular user.
+ LOG(INFO) << "Killing all processes referencing " << user_path;
+ KillProcessesUsingPath(user_path);
+ }
+
std::string label = getLabel();
if (!IsFuseBpfEnabled()) {
@@ -509,12 +521,24 @@
unmountFuseBindMounts();
}
- if (UnmountUserFuse(userId, getInternalPath(), label) != OK) {
- PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
- return -errno;
+ if (flags::enhance_fuse_unmount()) {
+ status_t result = UnmountUserFuseEnhanced(userId, getInternalPath(), label, user_path);
+ if (result != OK) {
+ PLOG(INFO) << "UnmountUserFuseEnhanced failed on emulated fuse volume";
+ return result;
+ }
+ } else {
+ if (UnmountUserFuse(userId, getInternalPath(), label) != OK) {
+ PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
+ return -errno;
+ }
}
mFuseMounted = false;
+ } else {
+ // This branch is needed to help with unmounting private volumes that aren't set to primary
+ // and don't have fuse mounted but have stacked emulated volumes
+ KillProcessesUsingPath(getPath());
}
return unmountSdcardFs();
diff --git a/model/PublicVolume.cpp b/model/PublicVolume.cpp
index 747598a..5a30fca 100644
--- a/model/PublicVolume.cpp
+++ b/model/PublicVolume.cpp
@@ -36,9 +36,11 @@
#include <sys/sysmacros.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <android_vold_flags.h>
using android::base::GetBoolProperty;
using android::base::StringPrintf;
+namespace flags = android::vold::flags;
namespace android {
namespace vold {
@@ -309,30 +311,57 @@
// the FUSE process first, most file system operations will return
// ENOTCONN until the unmount completes. This is an exotic and unusual
// error code and might cause broken behaviour in applications.
- KillProcessesUsingPath(getPath());
+
+ // We don't kill processes here if enhance_fuse_unmount as we make sure that we kill processes
+ // only if unmounting failed
+ if (!mFuseMounted || !flags::enhance_fuse_unmount()) {
+ KillProcessesUsingPath(getPath());
+ }
if (mFuseMounted) {
std::string stableName = getStableName();
// Unmount bind mounts for running users
auto vol_manager = VolumeManager::Instance();
- int user_id = getMountUserId();
- for (int started_user : vol_manager->getStartedUsers()) {
+ userid_t user_id = getMountUserId();
+ std::vector<std::string> bind_mount_paths;
+ for (userid_t started_user : vol_manager->getStartedUsers()) {
if (started_user == user_id) {
// No need to remove bind mount for the user that owns the mount
continue;
}
- LOG(INFO) << "Removing Public Volume Bind Mount for: " << started_user;
+ if (user_id != VolumeManager::Instance()->getSharedStorageUser(started_user)) {
+ // No need to remove bind mount
+ // if the user does not share storage with the mount owner
+ continue;
+ }
+
auto mountPath = GetFuseMountPathForUser(started_user, stableName);
- ForceUnmount(mountPath);
- rmdir(mountPath.c_str());
+ if (flags::enhance_fuse_unmount()) {
+ // Add it to list so that we unmount it as part of UnmountUserFuseEnhanced
+ bind_mount_paths.push_back(mountPath);
+ } else {
+ LOG(INFO) << "Removing Public Volume Bind Mount for: " << started_user;
+ ForceUnmount(mountPath);
+ rmdir(mountPath.c_str());
+ }
}
- if (UnmountUserFuse(getMountUserId(), getInternalPath(), stableName) != OK) {
- PLOG(INFO) << "UnmountUserFuse failed on public fuse volume";
- return -errno;
+ if (flags::enhance_fuse_unmount()) {
+ status_t result = UnmountUserFuseEnhanced(getMountUserId(), getInternalPath(),
+ stableName, getPath(), bind_mount_paths);
+ if (result != OK) {
+ PLOG(INFO) << "UnmountUserFuseEnhanced failed on public fuse volume";
+ return result;
+ }
+ } else {
+ if (UnmountUserFuse(getMountUserId(), getInternalPath(), stableName) != OK) {
+ PLOG(INFO) << "UnmountUserFuse failed on public fuse volume";
+ return -errno;
+ }
}
+
mFuseMounted = false;
}