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) } }