install: lazy unmount() in writeFilesystemContent() if needed (#12939)

* install: lazy unmount() in writeFilesystemContent() if needed

The existing code in writeFilesystemContent() will error when
the filesystem cannot be unmounted. However in practise this
is problematic as the live-system can keep the mount point
busy: https://bugs.launchpad.net/snapd/+bug/2025402

As a pragmatic solution this commit unmounts the filesystem
with the `--lazy` option if a normal unmount does not work.

This is what live-editor is doing:
mwhudson/livefs-editor#26

Alternatively we could do a bunch of retries and wait for
the process that keep the filesystem busy to go away.

* install: log unmount errors (thanks to Alfonso!)

* install: also lazy unmount in MountVolumes()

* install: fix silly typo

* install: improve logging on install failure

gadget/install/content.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/gadget"
@@ -82,9 +83,14 @@ func writeFilesystemContent(laidOut *gadget.LaidOutStructure, fsDevice string, o
 		return fmt.Errorf("cannot mount %q at %q: %v", fsDevice, mountpoint, err)
 	}
 	defer func() {
-		errUnmount := sysUnmount(mountpoint, 0)
-		if err == nil {
-			err = errUnmount
+		var errUnmount error
+		if errUnmount = sysUnmount(mountpoint, 0); errUnmount != nil {
+			logger.Noticef("cannot unmount %v after writing filesystem content, trying lazy unmount next: %v", mountpoint, errUnmount)
+			// lazy umount on error, see LP:2025402
+			errUnmount = sysUnmount(mountpoint, syscall.MNT_DETACH)
+		}
+		if err == nil && errUnmount != nil {
+			err = fmt.Errorf("cannot unmount %v after writing filesystem content: %v", fsDevice, errUnmount)
 		}
 	}()
 	fs, err := gadget.NewMountedFilesystemWriter(laidOut, observer)

gadget/install/content_test.go

@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"path/filepath"
+	"syscall"
 
 	. "gopkg.in/check.v1"
 
@@ -33,6 +34,7 @@ import (
 	"github.com/snapcore/snapd/gadget/gadgettest"
 	"github.com/snapcore/snapd/gadget/install"
 	"github.com/snapcore/snapd/gadget/quantity"
+	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -181,7 +183,7 @@ func (s *contentTestSuite) TestWriteFilesystemContent(c *C) {
 		}, {
 			mountErr:   nil,
 			unmountErr: errors.New("unmount error"),
-			err:        "unmount error",
+			err:        "cannot unmount /dev/node2 after writing filesystem content: unmount error",
 		}, {
 			observeErr: errors.New("observe error"),
 			err:        "cannot create filesystem image: cannot write filesystem content of source:grubx64.efi: cannot observe file write: observe error",
@@ -233,6 +235,85 @@ func (s *contentTestSuite) TestWriteFilesystemContent(c *C) {
 	}
 }
 
+func (s *contentTestSuite) TestWriteFilesystemContentUnmountErrHandling(c *C) {
+	dirs.SetRootDir(c.MkDir())
+	defer dirs.SetRootDir(dirs.GlobalRootDir)
+
+	log, restore := logger.MockLogger()
+	defer restore()
+
+	type unmountArgs struct {
+		target string
+		flags  int
+	}
+
+	restore = install.MockSysMount(func(source, target, fstype string, flags uintptr, data string) error {
+		return nil
+	})
+	defer restore()
+
+	// copy existing mock
+	m := mockOnDiskStructureSystemSeed(s.gadgetRoot)
+	obs := &mockWriteObserver{
+		c:            c,
+		observeErr:   nil,
+		expectedRole: m.Role(),
+	}
+
+	for _, tc := range []struct {
+		unmountErr     error
+		lazyUnmountErr error
+
+		expectedErr string
+	}{
+		{
+			nil,
+			nil,
+			"",
+		}, {
+			errors.New("umount error"),
+			nil,
+			"", // no error as lazy unmount succeeded
+		}, {
+			errors.New("umount error"),
+			errors.New("lazy umount err"),
+			`cannot unmount /dev/node2 after writing filesystem content: lazy umount err`},
+	} {
+		log.Reset()
+
+		var unmountCalls []unmountArgs
+		restore = install.MockSysUnmount(func(target string, flags int) error {
+			unmountCalls = append(unmountCalls, unmountArgs{target, flags})
+			switch flags {
+			case 0:
+				return tc.unmountErr
+			case syscall.MNT_DETACH:
+				return tc.lazyUnmountErr
+			default:
+				return fmt.Errorf("unexpected mount flag %v", flags)
+			}
+		})
+		defer restore()
+
+		err := install.WriteFilesystemContent(m, "/dev/node2", obs)
+		if tc.expectedErr == "" {
+			c.Assert(err, IsNil)
+		} else {
+			c.Assert(err, ErrorMatches, tc.expectedErr)
+		}
+		if tc.unmountErr == nil {
+			c.Check(unmountCalls, HasLen, 1)
+			c.Check(unmountCalls[0].flags, Equals, 0)
+			c.Check(log.String(), Equals, "")
+		} else {
+			c.Check(unmountCalls, HasLen, 2)
+			c.Check(unmountCalls[0].flags, Equals, 0)
+			c.Check(unmountCalls[1].flags, Equals, syscall.MNT_DETACH)
+			c.Check(log.String(), Matches, `(?sm).* cannot unmount /.*/run/snapd/gadget-install/dev-node2 after writing filesystem content, trying lazy unmount next: umount error`)
+		}
+	}
+}
+
 func (s *contentTestSuite) TestMakeFilesystem(c *C) {
 	mockUdevadm := testutil.MockCommand(c, "udevadm", "")
 	defer mockUdevadm.Restore()

gadget/install/install.go

@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"syscall"
 
 	"github.com/snapcore/snapd/asserts"
 	"github.com/snapcore/snapd/boot"
@@ -569,7 +570,10 @@ func MountVolumes(onVolumes map[string]*gadget.Volume, encSetupData *EncryptionS
 		for _, mntPt := range mountPoints {
 			errUnmount := sysUnmount(mntPt, 0)
 			if errUnmount != nil {
-				logger.Noticef("cannot unmount %q: %v", mntPt, errUnmount)
+				logger.Noticef("cannot unmount %q: %v (trying lazy unmount next)", mntPt, errUnmount)
+				// lazy umount on error, see LP:2025402
+				errUnmount = sysUnmount(mntPt, syscall.MNT_DETACH)
+				logger.Noticef("cannot lazy unmount %q: %v", mntPt, errUnmount)
 			}
 			// Make sure we do not set err to nil if it had already an error
 			if errUnmount != nil {