commit d5c120ed8f5b37d9091f71b8fc2d0d762bcd7971
author Alex Deymo <deymo@google.com> Mon Aug 22 19:51:59 2016 -0700
committer Alex Deymo <deymo@google.com> Tue Aug 23 13:05:26 2016 -0700
tree f87fc148772d281bb9343660538fad5edc1ec55f
parent 4d5fe6401fe4a0fbe342cf9c00cf703649fe9d08

Lazy unmount postinstall if it fails to unmount.

If postinstall forks a new child that's not killed when killing the main
postinstall process or if postinstall makes any other running process to
hold a file descriptor open in the mounted filesystem, the filesystem is
busy an we can't unmount /postinstall. Since the postinstall mountpoint is
fix in Android, we need to force a lazy unmount in order for the process
to succeed in a future run.

This case can only occur when canceling an update during postinstall (for
example if the update was retried from the server) and would otherwise
clear itself after a few unmount retries if the process using the fd
stops using it.

Bug: 31021934
Test: Added native tests to excersice this case.

(cherry picked from commit 5ba93ad42fad1efc40ae2e7abcda9c8793363508)

Change-Id: I3464377b43e2ee79cd7318dbc9d3a62706c6ea6c

common/utils.cc

diff --git a/common/utils.cc b/common/utils.cc
index 166cfb4..1338268 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -705,14 +705,24 @@
 }
 
 bool UnmountFilesystem(const string& mountpoint) {
-  for (int num_retries = 0; ; ++num_retries) {
+  int num_retries = 1;
+  for (;; ++num_retries) {
     if (umount(mountpoint.c_str()) == 0)
+      return true;
+    if (errno != EBUSY || num_retries >= kUnmountMaxNumOfRetries)
       break;
-
-    TEST_AND_RETURN_FALSE_ERRNO(errno == EBUSY &&
-                                num_retries < kUnmountMaxNumOfRetries);
     usleep(kUnmountRetryIntervalInMicroseconds);
   }
+  if (errno == EINVAL) {
+    LOG(INFO) << "Not a mountpoint: " << mountpoint;
+    return false;
+  }
+  PLOG(WARNING) << "Error unmounting " << mountpoint << " after " << num_retries
+                << " attempts. Lazy unmounting instead, error was";
+  if (umount2(mountpoint.c_str(), MNT_DETACH) != 0) {
+    PLOG(ERROR) << "Lazy unmount failed";
+    return false;
+  }
   return true;
 }
 

common/utils_unittest.cc

diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
index f840a75..b0beff9 100644
--- a/common/utils_unittest.cc
+++ b/common/utils_unittest.cc
@@ -17,7 +17,9 @@
 #include "update_engine/common/utils.h"
 
 #include <errno.h>
+#include <fcntl.h>
 #include <stdint.h>
+#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
@@ -545,4 +547,37 @@
   EXPECT_TRUE(BoolMacroTestHelper());
 }
 
+TEST(UtilsTest, UnmountFilesystemFailureTest) {
+  EXPECT_FALSE(utils::UnmountFilesystem("/path/to/non-existing-dir"));
+}
+
+TEST(UtilsTest, UnmountFilesystemBusyFailureTest) {
+  string tmp_image;
+  EXPECT_TRUE(utils::MakeTempFile("img.XXXXXX", &tmp_image, nullptr));
+  ScopedPathUnlinker tmp_image_unlinker(tmp_image);
+
+  EXPECT_TRUE(base::CopyFile(
+      test_utils::GetBuildArtifactsPath().Append("gen/disk_ext2_4k.img"),
+      base::FilePath(tmp_image)));
+
+  base::ScopedTempDir mnt_dir;
+  EXPECT_TRUE(mnt_dir.CreateUniqueTempDir());
+
+  string loop_dev;
+  test_utils::ScopedLoopbackDeviceBinder loop_binder(
+      tmp_image, true, &loop_dev);
+
+  // This is the actual test part. While we hold a file descriptor open for the
+  // mounted filesystem, umount should still succeed.
+  EXPECT_TRUE(utils::MountFilesystem(
+      loop_dev, mnt_dir.path().value(), MS_RDONLY, "ext4", ""));
+  string target_file = mnt_dir.path().Append("empty-file").value();
+  int fd = HANDLE_EINTR(open(target_file.c_str(), O_RDONLY));
+  EXPECT_GE(fd, 0);
+  EXPECT_TRUE(utils::UnmountFilesystem(mnt_dir.path().value()));
+  IGNORE_EINTR(close(fd));
+  // The filesystem was already unmounted so this call should fail.
+  EXPECT_FALSE(utils::UnmountFilesystem(mnt_dir.path().value()));
+}
+
 }  // namespace chromeos_update_engine