fix: make unmount handling robust for busy/disconnected mounts

Author: qinfustu

pkg/utils/mount/mount.go

@@ -13,6 +13,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/containerd/log"
 	"github.com/pkg/errors"
 
 	"github.com/containerd/nydus-snapshotter/pkg/errdefs"
@@ -26,17 +27,61 @@ type Interface interface {
 type Mounter struct {
 }
 
+// These indirections are test seams so unit tests can stub out the real
+// syscall / filesystem check without touching real mountpoints.
+var (
+	syscallUnmount = syscall.Unmount
+	isMountpoint   = IsMountpoint
+)
+
+// isDisconnected reports whether err indicates a broken/stale mountpoint whose
+// backing server (e.g. a dead nydusd behind a FUSE mount) is gone. Such a
+// mountpoint still needs to be unmounted, so callers must not bail out on it.
+func isDisconnected(err error) bool {
+	return errors.Is(err, syscall.ENOTCONN) || errors.Is(err, syscall.ESTALE)
+}
+
+// unmountWithFallback tries a plain unmount first and, on failure, degrades to
+// force then lazy detach so that busy or disconnected mountpoints are always
+// torn down instead of being left behind.
+func unmountWithFallback(target string) error {
+	err := syscallUnmount(target, 0)
+	if err == nil || errors.Is(err, syscall.EINVAL) {
+		// EINVAL means the target is not a mountpoint (already unmounted).
+		return nil
+	}
+
+	// umountForce aborts in-flight requests, which is what a disconnected FUSE
+	// mount needs; try it first.
+	if ferr := syscallUnmount(target, umountForce); ferr == nil {
+		log.L.Warnf("force umount %s after plain umount failed: %v", target, err)
+		return nil
+	}
+
+	// umountDetach (lazy) detaches from the namespace even while busy; last resort.
+	if lerr := syscallUnmount(target, umountDetach); lerr != nil {
+		return errors.Wrapf(lerr, "lazy umount %s (plain umount error: %v)", target, err)
+	}
+	log.L.Warnf("lazy-detached %s after plain umount failed: %v", target, err)
+	return nil
+}
+
 func (m *Mounter) Umount(target string) error {
-	if mounted, err := IsMountpoint(target); err == nil {
-		if !mounted {
-			return errors.New("not mounted")
+	mounted, err := isMountpoint(target)
+	if err != nil {
+		// A disconnected/stale mountpoint fails IsMountpoint (stat returns
+		// ENOTCONN) but still needs unmounting; proceed instead of bailing out.
+		if !isDisconnected(err) {
+			return err
 		}
-	} else {
-		return err
+		mounted = true
+	}
+
+	if !mounted {
+		return nil
 	}
 
-	// return syscall.Unmount(target, syscall.MNT_FORCE)
-	return syscall.Unmount(target, 0)
+	return unmountWithFallback(target)
 }
 
 func NormalizePath(path string) (realPath string, err error) {
@@ -83,8 +128,11 @@ func IsMountpoint(path string) (bool, error) {
 
 func WaitUntilUnmounted(path string) error {
 	return retry.Do(func() error {
-		mounted, err := IsMountpoint(path)
+		mounted, err := isMountpoint(path)
 		if err != nil {
+			if isDisconnected(err) {
+				return nil
+			}
 			return err
 		}
 

pkg/utils/mount/mount_darwin.go

@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2022. Nydus Developers. All rights reserved.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package mount
+
+import "golang.org/x/sys/unix"
+
+// darwin has no lazy (MNT_DETACH) unmount; degrade the last resort to a forced
+// unmount. This build exists mainly for local development on macOS; nydusd
+// mounts are only managed at runtime on Linux.
+const (
+	umountForce  = unix.MNT_FORCE
+	umountDetach = unix.MNT_FORCE
+)

pkg/utils/mount/mount_linux.go

@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2022. Nydus Developers. All rights reserved.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package mount
+
+import "golang.org/x/sys/unix"
+
+const (
+	// umountForce aborts in-flight requests (needed for disconnected FUSE mounts).
+	umountForce = unix.MNT_FORCE
+	// umountDetach performs a lazy unmount, detaching a busy mountpoint from the
+	// namespace and cleaning it up once it is no longer referenced.
+	umountDetach = unix.MNT_DETACH
+)

pkg/utils/mount/mount_test.go

@@ -0,0 +1,139 @@
+/*
+ * Copyright (c) 2022. Nydus Developers. All rights reserved.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package mount
+
+import (
+	"syscall"
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIsDisconnected(t *testing.T) {
+	assert.True(t, isDisconnected(syscall.ENOTCONN))
+	assert.True(t, isDisconnected(syscall.ESTALE))
+	// Must see through pkg/errors wrapping (IsMountpoint wraps stat errors).
+	assert.True(t, isDisconnected(errors.Wrapf(syscall.ENOTCONN, "stat target of %s", "/foo")))
+	assert.False(t, isDisconnected(syscall.EBUSY))
+	assert.False(t, isDisconnected(errors.New("some other error")))
+	assert.False(t, isDisconnected(nil))
+}
+
+func TestUnmountWithFallback(t *testing.T) {
+	origUnmount := syscallUnmount
+	t.Cleanup(func() { syscallUnmount = origUnmount })
+
+	type call struct {
+		flags int
+	}
+
+	tests := []struct {
+		name         string
+		errByAttempt []error // error returned for the Nth syscallUnmount call
+		wantErr      bool
+		wantCalls    []call
+	}{
+		{
+			name:         "plain umount succeeds",
+			errByAttempt: []error{nil},
+			wantCalls:    []call{{0}},
+		},
+		{
+			name:         "EINVAL treated as already unmounted",
+			errByAttempt: []error{syscall.EINVAL},
+			wantCalls:    []call{{0}},
+		},
+		{
+			name:         "EBUSY falls back to force",
+			errByAttempt: []error{syscall.EBUSY, nil},
+			wantCalls:    []call{{0}, {umountForce}},
+		},
+		{
+			name:         "force fails then lazy detach succeeds",
+			errByAttempt: []error{syscall.EBUSY, syscall.EBUSY, nil},
+			wantCalls:    []call{{0}, {umountForce}, {umountDetach}},
+		},
+		{
+			name:         "all attempts fail returns error",
+			errByAttempt: []error{syscall.EBUSY, syscall.EBUSY, syscall.EBUSY},
+			wantErr:      true,
+			wantCalls:    []call{{0}, {umountForce}, {umountDetach}},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var gotCalls []call
+			idx := 0
+			syscallUnmount = func(_ string, flags int) error {
+				gotCalls = append(gotCalls, call{flags})
+				err := tc.errByAttempt[idx]
+				idx++
+				return err
+			}
+
+			err := unmountWithFallback("/mnt/target")
+			if tc.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tc.wantCalls, gotCalls)
+		})
+	}
+}
+
+func TestUmount(t *testing.T) {
+	origIsMountpoint := isMountpoint
+	origUnmount := syscallUnmount
+	t.Cleanup(func() {
+		isMountpoint = origIsMountpoint
+		syscallUnmount = origUnmount
+	})
+
+	t.Run("disconnected mountpoint still gets unmounted", func(t *testing.T) {
+		isMountpoint = func(string) (bool, error) { return false, syscall.ENOTCONN }
+		unmounted := false
+		syscallUnmount = func(string, int) error { unmounted = true; return nil }
+
+		require.NoError(t, (&Mounter{}).Umount("/mnt/target"))
+		assert.True(t, unmounted, "a disconnected mountpoint must still be unmounted")
+	})
+
+	t.Run("not mounted returns nil without unmounting", func(t *testing.T) {
+		isMountpoint = func(string) (bool, error) { return false, nil }
+		called := false
+		syscallUnmount = func(string, int) error { called = true; return nil }
+
+		require.NoError(t, (&Mounter{}).Umount("/mnt/target"))
+		assert.False(t, called, "must not attempt to unmount a non-mountpoint")
+	})
+
+	t.Run("other IsMountpoint error is propagated", func(t *testing.T) {
+		wantErr := errors.New("boom")
+		isMountpoint = func(string) (bool, error) { return false, wantErr }
+		syscallUnmount = func(string, int) error { return nil }
+
+		require.ErrorIs(t, (&Mounter{}).Umount("/mnt/target"), wantErr)
+	})
+}
+
+func TestWaitUntilUnmountedIgnoresDisconnectedMountpoint(t *testing.T) {
+	origIsMountpoint := isMountpoint
+	t.Cleanup(func() { isMountpoint = origIsMountpoint })
+
+	calls := 0
+	isMountpoint = func(string) (bool, error) {
+		calls++
+		return false, syscall.ENOTCONN
+	}
+
+	require.NoError(t, WaitUntilUnmounted("/mnt/target"))
+	assert.Equal(t, 1, calls)
+}