From 64c15f010c9165af4e0a160bfd58373b0a8d6294 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Tue, 2 Nov 2021 21:51:50 -0700 Subject: [PATCH] Add retries when removing device mapper target (#1200) Add retries when removing device mapper target Additionally ignore the error from device mapper if target removal still fails. The corresponding layer path is already unmounted at that point and this avoids having an inconsistent state. Signed-off-by: Maksim An --- guest/storage/devicemapper/devicemapper.go | 41 +++++++-- .../storage/devicemapper/devicemapper_test.go | 83 +++++++++++++++++++ guest/storage/pmem/pmem.go | 6 +- 3 files changed, 119 insertions(+), 11 deletions(-) diff --git a/guest/storage/devicemapper/devicemapper.go b/guest/storage/devicemapper/devicemapper.go index 01d712d37a..c7157fe445 100644 --- a/guest/storage/devicemapper/devicemapper.go +++ b/guest/storage/devicemapper/devicemapper.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "path" + "syscall" + "time" "unsafe" "golang.org/x/sys/unix" @@ -19,6 +21,11 @@ const ( CreateReadOnly CreateFlags = 1 << iota ) +var ( + removeDeviceWrapper = removeDevice + openMapperWrapper = openMapper +) + const ( _IOC_WRITE = 1 _IOC_READ = 2 @@ -223,7 +230,7 @@ func makeTableIoctl(name string, targets []Target) *dmIoctl { // CreateDevice creates a device-mapper device with the given target spec. It returns // the path of the new device node. func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, err error) { - f, err := openMapper() + f, err := openMapperWrapper() if err != nil { return "", err } @@ -238,7 +245,7 @@ func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, e } defer func() { if err != nil { - removeDevice(f, name) + removeDeviceWrapper(f, name) } }() @@ -269,14 +276,30 @@ func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, e } // RemoveDevice removes a device-mapper device and its associated device node. -func RemoveDevice(name string) error { - f, err := openMapper() - if err != nil { - return err +func RemoveDevice(name string) (err error) { + rm := func() error { + f, err := openMapperWrapper() + if err != nil { + return err + } + defer f.Close() + os.Remove(path.Join("/dev/mapper", name)) + return removeDeviceWrapper(f, name) } - defer f.Close() - os.Remove(path.Join("/dev/mapper", name)) - return removeDevice(f, name) + + // This is workaround for "device or resource busy" error, which occasionally happens after the device mapper + // target has been unmounted. + for i := 0; i < 10; i++ { + if err = rm(); err != nil { + if e, ok := err.(*dmError); !ok || e.Err != syscall.EBUSY { + break + } + time.Sleep(10 * time.Millisecond) + continue + } + break + } + return } func removeDevice(f *os.File, name string) error { diff --git a/guest/storage/devicemapper/devicemapper_test.go b/guest/storage/devicemapper/devicemapper_test.go index 723fe1496a..278345b484 100644 --- a/guest/storage/devicemapper/devicemapper_test.go +++ b/guest/storage/devicemapper/devicemapper_test.go @@ -4,7 +4,9 @@ package devicemapper import ( "flag" + "io/ioutil" "os" + "syscall" "testing" "unsafe" @@ -15,6 +17,11 @@ var ( integration = flag.Bool("integration", false, "run integration tests") ) +func clearTestDependencies() { + removeDeviceWrapper = removeDevice + openMapperWrapper = openMapper +} + func TestMain(m *testing.M) { flag.Parse() m.Run() @@ -75,6 +82,8 @@ func createDevice(name string, flags CreateFlags, targets []Target) (*device, er } func TestCreateError(t *testing.T) { + clearTestDependencies() + if !*integration { t.Skip() } @@ -94,6 +103,8 @@ func TestCreateError(t *testing.T) { } func TestReadOnlyError(t *testing.T) { + clearTestDependencies() + if !*integration { t.Skip() } @@ -113,6 +124,8 @@ func TestReadOnlyError(t *testing.T) { } func TestLinearError(t *testing.T) { + clearTestDependencies() + if !*integration { t.Skip() } @@ -140,3 +153,73 @@ func TestLinearError(t *testing.T) { t.Fatal(err) } } + +func TestRemoveDeviceRetriesOnSyscallEBUSY(t *testing.T) { + clearTestDependencies() + + rmDeviceCalled := false + retryDone := false + // Overrides openMapper to return temp file handle + openMapperWrapper = func() (*os.File, error) { + return ioutil.TempFile("", "") + } + removeDeviceWrapper = func(_ *os.File, _ string) error { + if !rmDeviceCalled { + rmDeviceCalled = true + return &dmError{ + Op: 1, + Err: syscall.EBUSY, + } + } + if !retryDone { + retryDone = true + return nil + } + return nil + } + + if err := RemoveDevice("test"); err != nil { + t.Fatalf("expected no error, got: %s", err) + } + if !rmDeviceCalled { + t.Fatalf("expected removeDevice to be called at least once") + } + if !retryDone { + t.Fatalf("expected removeDevice to be retried after initial failure") + } +} + +func TestRemoveDeviceFailsOnNonSyscallEBUSY(t *testing.T) { + clearTestDependencies() + + expectedError := &dmError{ + Op: 0, + Err: syscall.EACCES, + } + rmDeviceCalled := false + retryDone := false + openMapperWrapper = func() (*os.File, error) { + return ioutil.TempFile("", "") + } + removeDeviceWrapper = func(_ *os.File, _ string) error { + if !rmDeviceCalled { + rmDeviceCalled = true + return expectedError + } + if !retryDone { + retryDone = true + return nil + } + return nil + } + + if err := RemoveDevice("test"); err != expectedError { + t.Fatalf("expected error %q, instead got %q", expectedError, err) + } + if !rmDeviceCalled { + t.Fatalf("expected removeDevice to be called once") + } + if retryDone { + t.Fatalf("no retries should've been attempted") + } +} diff --git a/guest/storage/pmem/pmem.go b/guest/storage/pmem/pmem.go index 681659f061..8826e3d00d 100644 --- a/guest/storage/pmem/pmem.go +++ b/guest/storage/pmem/pmem.go @@ -143,14 +143,16 @@ func Unmount(ctx context.Context, devNumber uint32, target string, mappingInfo * if verityInfo != nil { dmVerityName := fmt.Sprintf(verityDeviceFmt, devNumber, verityInfo.RootDigest) if err := dm.RemoveDevice(dmVerityName); err != nil { - return errors.Wrapf(err, "failed to remove dm verity target: %s", dmVerityName) + // The target is already unmounted at this point, ignore potential errors + log.G(ctx).WithError(err).Debugf("failed to remove dm verity target: %s", dmVerityName) } } if mappingInfo != nil { dmLinearName := fmt.Sprintf(linearDeviceFmt, devNumber, mappingInfo.DeviceOffsetInBytes, mappingInfo.DeviceSizeInBytes) if err := dm.RemoveDevice(dmLinearName); err != nil { - return errors.Wrapf(err, "failed to remove dm linear target: %s", dmLinearName) + // The target is already unmounted at this point, ignore potential errors + log.G(ctx).WithError(err).Debugf("failed to remove dm linear target: %s", dmLinearName) } }