From 9e4207016903d274900c0585405cc0bfcfd2e4ef Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Fri, 13 Mar 2020 14:40:58 +0800 Subject: [PATCH 1/4] mount: handle loopback mount If a mount has specified `loop` option, we need to handle it on our own instead of passing it to the kernel. In such case, create a loopback device, attach the mount source to it, and mount the loopback device rather than the mount source. Signed-off-by: Peng Tao --- mount/losetup_linux.go | 230 ++++++++++++++++++++++++++++++++++++ mount/losetup_linux_test.go | 117 ++++++++++++++++++ mount/mount_linux.go | 30 +++-- 3 files changed, 370 insertions(+), 7 deletions(-) create mode 100644 mount/losetup_linux.go create mode 100644 mount/losetup_linux_test.go diff --git a/mount/losetup_linux.go b/mount/losetup_linux.go new file mode 100644 index 000000000..68fd799b0 --- /dev/null +++ b/mount/losetup_linux.go @@ -0,0 +1,230 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package mount + +import ( + "fmt" + "math/rand" + "os" + "strings" + "syscall" + "time" + "unsafe" + + "github.com/pkg/errors" +) + +const ( + loopControlPath = "/dev/loop-control" + loopDevFormat = "/dev/loop%d" + + // According to util-linux/include/loopdev.h + ioctlSetFd = 0x4C00 + ioctlClrFd = 0x4C01 + ioctlSetStatus64 = 0x4C04 + ioctlGetFree = 0x4C82 + + loFlagsReadonly = 1 + //loFlagsUseAops = 2 + loFlagsAutoclear = 4 + //loFlagsPartScan = 8 + loFlagsDirectIO = 16 + + ebusyString = "device or resource busy" +) + +// parameters to control loop device setup +type LoopParams struct { + // Loop device should forbid write + Readonly bool + // Loop device is automatically cleared by kernel when the + // last opener closes it + Autoclear bool + // Use direct IO to access the loop backing file + Direct bool +} + +// struct loop_info64 in util-linux/include/loopdev.h +type loopInfo struct { + /* + device uint64 + inode uint64 + rdevice uint64 + offset uint64 + sizelimit uint64 + number uint32 + encryptType uint32 + encryptKeySize uint32 + */ + _ [13]uint32 + flags uint32 + fileName [64]byte + /* + cryptName [64]byte + encryptKey [32]byte + init [2]uint64 + */ + _ [112]byte +} + +func ioctl(fd, req, args uintptr) (uintptr, uintptr, error) { + r1, r2, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, req, args) + if errno != 0 { + return 0, 0, errno + } + + return r1, r2, nil +} + +func getFreeLoopDev() (uint32, error) { + ctrl, err := os.OpenFile(loopControlPath, os.O_RDWR, 0) + if err != nil { + return 0, errors.Errorf("could not open %v: %v", loopControlPath, err) + } + defer ctrl.Close() + num, _, err := ioctl(ctrl.Fd(), ioctlGetFree, 0) + if err != nil { + return 0, errors.Wrap(err, "could not get free loop device") + } + return uint32(num), nil +} + +func setupLoopDev(backingFile, loopDev string, param LoopParams) (devFile *os.File, err error) { + // 1. Open backing file and loop device + oflags := os.O_RDWR + if param.Readonly { + oflags = os.O_RDONLY + } + back, err := os.OpenFile(backingFile, oflags, 0) + if err != nil { + return nil, errors.Errorf("could not open backing file: %v", err) + } + defer back.Close() + + loopFile, err := os.OpenFile(loopDev, oflags, 0) + if err != nil { + return nil, errors.Errorf("could not open loop device: %v", err) + } + defer func() { + if err != nil { + loopFile.Close() + } + }() + + // 2. Set FD + if _, _, err = ioctl(loopFile.Fd(), ioctlSetFd, back.Fd()); err != nil { + return nil, errors.Errorf("could not set loop fd: %v", err) + } + + // 3. Set Info + info := loopInfo{} + copy(info.fileName[:], []byte(backingFile)) + if param.Readonly { + info.flags |= loFlagsReadonly + } + if param.Autoclear { + info.flags |= loFlagsAutoclear + } + if param.Direct { + info.flags |= loFlagsAutoclear + } + if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil { + // Retry w/o direct IO flag in case kernel does not support it. The downside is that + // it will suffer from double cache problem. + info.flags &= ^(uint32(loFlagsDirectIO)) + if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil { + ioctl(loopFile.Fd(), ioctlClrFd, 0) + return nil, errors.Errorf("cannot set loop info:%v", err) + } + } + + return loopFile, nil +} + +// setupLoop looks for (and possibly creates) a free loop device, and +// then attaches backingFile to it. +// +// When autoclear is true, caller should take care to close it when +// done with the loop device. The loop device file handle keeps +// loFlagsAutoclear in effect and we rely on it to clean up the loop +// device. If caller closes the file handle after mounting the device, +// kernel will clear the loop device after it is umounted. Otherwise +// the loop device is cleared when the file handle is closed. +// +// When autoclear is false, caller should be responsible to remove +// the loop device when done with it. +// +// Upon success, the file handle to the loop device is returned. +func setupLoop(backingFile string, param LoopParams) (*os.File, error) { + var loopDev string + + for retry := 1; retry < 200; retry++ { + num, err := getFreeLoopDev() + if err != nil { + return nil, err + } + + loopDev = fmt.Sprintf(loopDevFormat, num) + loopFile, err := setupLoopDev(backingFile, loopDev, param) + if err != nil { + // Per util-linux/sys-utils/losetup.c:create_loop(), + // free loop device can race and we end up failing + // with EBUSY when trying to set it up. + if strings.Contains(err.Error(), ebusyString) { + // Fallback a bit to avoid live lock + time.Sleep(time.Millisecond * time.Duration(rand.Intn(retry*10))) + continue + } + return nil, err + } + return loopFile, nil + } + + return nil, errors.New("Timeout creating new loopback device") +} + +func removeLoop(loopdev string) error { + dev, err := os.Open(loopdev) + if err != nil { + return err + } + _, _, err = ioctl(dev.Fd(), ioctlClrFd, 0) + dev.Close() + return err +} + +// Attach a specified backing file to a loop device +func AttachLoopDevice(backingFile string) (string, error) { + dev, err := setupLoop(backingFile, LoopParams{}) + if err != nil { + return "", err + } + dev.Close() + + return dev.Name(), nil +} + +// Detach a loop device +func DetachLoopDevice(devices ...string) error { + for _, dev := range devices { + if err := removeLoop(dev); err != nil { + return err + } + } + + return nil +} diff --git a/mount/losetup_linux_test.go b/mount/losetup_linux_test.go new file mode 100644 index 000000000..6ed1e0eb8 --- /dev/null +++ b/mount/losetup_linux_test.go @@ -0,0 +1,117 @@ +// +build linux + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package mount + +import ( + "io/ioutil" + "os" + "testing" + + "github.com/containerd/continuity/testutil" +) + +func TestSetupLoop(t *testing.T) { + testutil.RequiresRoot(t) + const randomdata = "randomdata" + + /* Non-existing loop */ + backingFile := "setup-loop-test-no-such-file" + _, err := setupLoop(backingFile, LoopParams{}) + if err == nil { + t.Fatalf("setupLoop with non-existing file should fail") + } + + f, err := ioutil.TempFile("", "losetup") + if err != nil { + t.Fatal(err) + } + if err = f.Truncate(512); err != nil { + t.Fatal(err) + } + backingFile = f.Name() + f.Close() + defer func() { + if err := os.Remove(backingFile); err != nil { + t.Fatal(err) + } + }() + + /* RO loop */ + f, err = setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true}) + if err != nil { + t.Fatal(err) + } + ff, err := os.OpenFile(f.Name(), os.O_RDWR, 0) + if err != nil { + t.Fatal(err) + } + if _, err = ff.Write([]byte(randomdata)); err == nil { + t.Fatalf("writing to readonly loop device should fail") + } + if err = ff.Close(); err != nil { + t.Fatal(err) + } + if err = f.Close(); err != nil { + t.Fatal(err) + } + + /* RW loop */ + f, err = setupLoop(backingFile, LoopParams{Autoclear: true}) + if err != nil { + t.Fatal(err) + } + ff, err = os.OpenFile(f.Name(), os.O_RDWR, 0) + if err != nil { + t.Fatal(err) + } + if _, err = ff.Write([]byte(randomdata)); err != nil { + t.Fatal(err) + } + if err = ff.Close(); err != nil { + t.Fatal(err) + } + if err = f.Close(); err != nil { + t.Fatal(err) + } +} + +func TestAttachDetachLoopDevice(t *testing.T) { + testutil.RequiresRoot(t) + f, err := ioutil.TempFile("", "losetup") + if err != nil { + t.Fatal(err) + } + if err = f.Truncate(512); err != nil { + t.Fatal(err) + } + f.Close() + defer func() { + if err := os.Remove(f.Name()); err != nil { + t.Fatal(err) + } + }() + + dev, err := AttachLoopDevice(f.Name()) + if err != nil { + t.Fatal(err) + } + if err = DetachLoopDevice(dev); err != nil { + t.Fatal(err) + } +} diff --git a/mount/mount_linux.go b/mount/mount_linux.go index 6d28f0a97..b0e24f8f0 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -42,7 +42,7 @@ func init() { // // If m.Type starts with "fuse." or "fuse3.", "mount.fuse" or "mount.fuse3" // helper binary is called. -func (m *Mount) Mount(target string) error { +func (m *Mount) Mount(target string) (err error) { for _, helperBinary := range allowedHelperBinaries { // helperBinary = "mount.fuse", typePrefix = "fuse." typePrefix := strings.TrimPrefix(helperBinary, "mount.") + "." @@ -62,7 +62,7 @@ func (m *Mount) Mount(target string) error { chdir, options = compactLowerdirOption(options) } - flags, data := parseMountOptions(options) + flags, data, losetup := parseMountOptions(options) if len(data) > pagesize { return errors.Errorf("mount options is too long") } @@ -77,7 +77,19 @@ func (m *Mount) Mount(target string) error { if flags&unix.MS_REMOUNT == 0 || data != "" { // Initial call applying all non-propagation flags for mount // or remount with changed data - if err := mountAt(chdir, m.Source, target, m.Type, uintptr(oflags), data); err != nil { + source := m.Source + if losetup { + devFile, err := setupLoop(m.Source, LoopParams{ + Readonly: oflags&unix.MS_RDONLY == unix.MS_RDONLY, + Autoclear: true}) + if err != nil { + return err + } + defer devFile.Close() + // Mount the loop device instead + source = devFile.Name() + } + if err := mountAt(chdir, source, target, m.Type, uintptr(oflags), data); err != nil { return err } } @@ -186,11 +198,13 @@ func UnmountAll(mount string, flags int) error { // parseMountOptions takes fstab style mount options and parses them for // use with a standard mount() syscall -func parseMountOptions(options []string) (int, string) { +func parseMountOptions(options []string) (int, string, bool) { var ( - flag int - data []string + flag int + losetup bool + data []string ) + loopOpt := "loop" flags := map[string]struct { clear bool flag int @@ -231,11 +245,13 @@ func parseMountOptions(options []string) (int, string) { } else { flag |= f.flag } + } else if o == loopOpt { + losetup = true } else { data = append(data, o) } } - return flag, strings.Join(data, ",") + return flag, strings.Join(data, ","), losetup } // compactLowerdirOption updates overlay lowdir option and returns the common From b7026236f42915fbf8cb72761aca254747200379 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 14 Apr 2020 00:37:54 -0700 Subject: [PATCH 2/4] snapshot/devmapper: use losetup in mount package No need to use the private losetup command line wrapper package. The generic package provides the same functionality. Signed-off-by: Peng Tao --- snapshots/devmapper/dmsetup/dmsetup_test.go | 13 +- snapshots/devmapper/losetup/losetup.go | 92 --------------- snapshots/devmapper/losetup/losetup_test.go | 124 -------------------- snapshots/devmapper/pool_device_test.go | 5 +- snapshots/devmapper/snapshotter_test.go | 3 +- 5 files changed, 8 insertions(+), 229 deletions(-) delete mode 100644 snapshots/devmapper/losetup/losetup.go delete mode 100644 snapshots/devmapper/losetup/losetup_test.go diff --git a/snapshots/devmapper/dmsetup/dmsetup_test.go b/snapshots/devmapper/dmsetup/dmsetup_test.go index 3b4c688bb..e8b95330b 100644 --- a/snapshots/devmapper/dmsetup/dmsetup_test.go +++ b/snapshots/devmapper/dmsetup/dmsetup_test.go @@ -29,8 +29,8 @@ import ( "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" + "github.com/containerd/containerd/mount" "github.com/containerd/containerd/pkg/testutil" - "github.com/containerd/containerd/snapshots/devmapper/losetup" ) const ( @@ -55,16 +55,13 @@ func TestDMSetup(t *testing.T) { metaImage, loopMetaDevice := createLoopbackDevice(t, tempDir) defer func() { - err = losetup.RemoveLoopDevicesAssociatedWithImage(dataImage) - assert.NilError(t, err, "failed to detach loop devices for data image: %s", dataImage) - - err = losetup.RemoveLoopDevicesAssociatedWithImage(metaImage) - assert.NilError(t, err, "failed to detach loop devices for meta image: %s", metaImage) + err = mount.DetachLoopDevice(loopDataDevice, loopMetaDevice) + assert.NilError(t, err, "failed to detach loop devices for data image: %s and meta image: %s", dataImage, metaImage) }() t.Run("CreatePool", func(t *testing.T) { err := CreatePool(testPoolName, loopDataDevice, loopMetaDevice, 128) - assert.NilError(t, err, "failed to create thin-pool") + assert.NilError(t, err, "failed to create thin-pool with %s %s", loopDataDevice, loopMetaDevice) table, err := Table(testPoolName) t.Logf("table: %s", table) @@ -201,7 +198,7 @@ func createLoopbackDevice(t *testing.T, dir string) (string, string) { imagePath := file.Name() - loopDevice, err := losetup.AttachLoopDevice(imagePath) + loopDevice, err := mount.AttachLoopDevice(imagePath) assert.NilError(t, err) return imagePath, loopDevice diff --git a/snapshots/devmapper/losetup/losetup.go b/snapshots/devmapper/losetup/losetup.go deleted file mode 100644 index b9e9bf980..000000000 --- a/snapshots/devmapper/losetup/losetup.go +++ /dev/null @@ -1,92 +0,0 @@ -// +build linux - -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package losetup - -import ( - "os/exec" - "strings" - - "github.com/pkg/errors" - "golang.org/x/sys/unix" -) - -// FindAssociatedLoopDevices returns a list of loop devices attached to a given image -func FindAssociatedLoopDevices(imagePath string) ([]string, error) { - output, err := losetup("--list", "--output", "NAME", "--associated", imagePath) - if err != nil { - return nil, errors.Wrapf(err, "failed to get loop devices: '%s'", output) - } - - if output == "" { - return []string{}, nil - } - - items := strings.Split(output, "\n") - if len(items) <= 1 { - return []string{}, nil - } - - // Skip header with column names - return items[1:], nil -} - -// AttachLoopDevice finds first available loop device and associates it with an image. -func AttachLoopDevice(imagePath string) (string, error) { - return losetup("--find", "--show", imagePath) -} - -// DetachLoopDevice detaches loop devices -func DetachLoopDevice(loopDevice ...string) error { - args := append([]string{"--detach"}, loopDevice...) - _, err := losetup(args...) - return err -} - -// RemoveLoopDevicesAssociatedWithImage detaches all loop devices attached to a given sparse image -func RemoveLoopDevicesAssociatedWithImage(imagePath string) error { - loopDevices, err := FindAssociatedLoopDevices(imagePath) - if err != nil { - return err - } - - for _, loopDevice := range loopDevices { - if err = DetachLoopDevice(loopDevice); err != nil && err != unix.ENOENT { - return err - } - } - - return nil -} - -// losetup is a wrapper around losetup command line tool -func losetup(args ...string) (string, error) { - cmd := exec.Command("losetup", args...) - cmd.Env = append(cmd.Env, "LANG=C") - data, err := cmd.CombinedOutput() - output := string(data) - if err != nil { - if strings.Contains(output, "No such file or directory") || strings.Contains(output, "No such device") { - return "", unix.ENOENT - } - - return "", errors.Wrapf(err, "losetup %s\nerror: %s\n", strings.Join(args, " "), output) - } - - return strings.TrimSuffix(output, "\n"), err -} diff --git a/snapshots/devmapper/losetup/losetup_test.go b/snapshots/devmapper/losetup/losetup_test.go deleted file mode 100644 index b6d2d5ebc..000000000 --- a/snapshots/devmapper/losetup/losetup_test.go +++ /dev/null @@ -1,124 +0,0 @@ -// +build linux - -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package losetup - -import ( - "io/ioutil" - "os" - "testing" - - "github.com/docker/go-units" - "golang.org/x/sys/unix" - "gotest.tools/v3/assert" - is "gotest.tools/v3/assert/cmp" - - "github.com/containerd/containerd/pkg/testutil" -) - -func TestLosetup(t *testing.T) { - testutil.RequiresRoot(t) - - var ( - imagePath = createSparseImage(t) - loopDevice1 string - loopDevice2 string - ) - - defer func() { - err := os.Remove(imagePath) - assert.NilError(t, err) - }() - - t.Run("AttachLoopDevice", func(t *testing.T) { - dev1, err := AttachLoopDevice(imagePath) - assert.NilError(t, err) - assert.Assert(t, dev1 != "") - - dev2, err := AttachLoopDevice(imagePath) - assert.NilError(t, err) - assert.Assert(t, dev2 != dev1, "should attach different loop device") - - loopDevice1 = dev1 - loopDevice2 = dev2 - }) - - t.Run("AttachEmptyLoopDevice", func(t *testing.T) { - _, err := AttachLoopDevice("") - assert.Assert(t, err != nil, "shouldn't attach empty path") - }) - - t.Run("FindAssociatedLoopDevices", func(t *testing.T) { - devices, err := FindAssociatedLoopDevices(imagePath) - assert.NilError(t, err) - assert.Assert(t, is.Len(devices, 2), "unexpected number of attached devices") - assert.Assert(t, is.Contains(devices, loopDevice1)) - assert.Assert(t, is.Contains(devices, loopDevice2)) - }) - - t.Run("FindAssociatedLoopDevicesForInvalidImage", func(t *testing.T) { - devices, err := FindAssociatedLoopDevices("") - assert.NilError(t, err) - assert.Assert(t, is.Len(devices, 0)) - }) - - t.Run("DetachLoopDevice", func(t *testing.T) { - err := DetachLoopDevice(loopDevice2) - assert.NilError(t, err, "failed to detach %q", loopDevice2) - }) - - t.Run("DetachEmptyDevice", func(t *testing.T) { - err := DetachLoopDevice("") - assert.Assert(t, err != nil, "shouldn't detach empty path") - }) - - t.Run("RemoveLoopDevicesAssociatedWithImage", func(t *testing.T) { - err := RemoveLoopDevicesAssociatedWithImage(imagePath) - assert.NilError(t, err) - - devices, err := FindAssociatedLoopDevices(imagePath) - assert.NilError(t, err) - assert.Assert(t, is.Len(devices, 0)) - }) - - t.Run("RemoveLoopDevicesAssociatedWithInvalidImage", func(t *testing.T) { - err := RemoveLoopDevicesAssociatedWithImage("") - assert.NilError(t, err) - }) - - t.Run("DetachInvalidDevice", func(t *testing.T) { - err := DetachLoopDevice("/dev/loop_invalid_idx") - assert.Equal(t, unix.ENOENT, err) - }) -} - -func createSparseImage(t *testing.T) string { - file, err := ioutil.TempFile("", "losetup-tests-") - assert.NilError(t, err) - - size, err := units.RAMInBytes("16Mb") - assert.NilError(t, err) - - err = file.Truncate(size) - assert.NilError(t, err) - - err = file.Close() - assert.NilError(t, err) - - return file.Name() -} diff --git a/snapshots/devmapper/pool_device_test.go b/snapshots/devmapper/pool_device_test.go index cc729a3a0..8525c0028 100644 --- a/snapshots/devmapper/pool_device_test.go +++ b/snapshots/devmapper/pool_device_test.go @@ -31,7 +31,6 @@ import ( "github.com/containerd/containerd/mount" "github.com/containerd/containerd/pkg/testutil" "github.com/containerd/containerd/snapshots/devmapper/dmsetup" - "github.com/containerd/containerd/snapshots/devmapper/losetup" "github.com/docker/go-units" "github.com/sirupsen/logrus" "gotest.tools/v3/assert" @@ -74,7 +73,7 @@ func TestPoolDevice(t *testing.T) { defer func() { // Detach loop devices and remove images - err := losetup.DetachLoopDevice(loopDataDevice, loopMetaDevice) + err := mount.DetachLoopDevice(loopDataDevice, loopMetaDevice) assert.NilError(t, err) err = os.RemoveAll(tempDir) @@ -306,7 +305,7 @@ func createLoopbackDevice(t *testing.T, dir string) (string, string) { imagePath := file.Name() - loopDevice, err := losetup.AttachLoopDevice(imagePath) + loopDevice, err := mount.AttachLoopDevice(imagePath) assert.NilError(t, err) return imagePath, loopDevice diff --git a/snapshots/devmapper/snapshotter_test.go b/snapshots/devmapper/snapshotter_test.go index 5157be869..419ed7206 100644 --- a/snapshots/devmapper/snapshotter_test.go +++ b/snapshots/devmapper/snapshotter_test.go @@ -37,7 +37,6 @@ import ( "github.com/containerd/containerd/pkg/testutil" "github.com/containerd/containerd/snapshots" "github.com/containerd/containerd/snapshots/devmapper/dmsetup" - "github.com/containerd/containerd/snapshots/devmapper/losetup" "github.com/containerd/containerd/snapshots/testsuite" ) @@ -70,7 +69,7 @@ func TestSnapshotterSuite(t *testing.T) { removePool := func() error { result := multierror.Append( snap.pool.RemovePool(ctx), - losetup.DetachLoopDevice(loopDataDevice, loopMetaDevice)) + mount.DetachLoopDevice(loopDataDevice, loopMetaDevice)) return result.ErrorOrNil() } From c5fa0298c110d52841def1333632afb6bc09b59f Mon Sep 17 00:00:00 2001 From: Maksym Pavlenko Date: Mon, 4 Jan 2021 10:44:29 -0800 Subject: [PATCH 3/4] Address loop dev PR comments #4178 Signed-off-by: Maksym Pavlenko --- mount/losetup_linux.go | 132 +++++++++++++----------------------- mount/losetup_linux_test.go | 14 ++-- mount/mount_linux.go | 3 +- 3 files changed, 54 insertions(+), 95 deletions(-) diff --git a/mount/losetup_linux.go b/mount/losetup_linux.go index 68fd799b0..f88a9d41c 100644 --- a/mount/losetup_linux.go +++ b/mount/losetup_linux.go @@ -26,28 +26,17 @@ import ( "unsafe" "github.com/pkg/errors" + "golang.org/x/sys/unix" ) const ( loopControlPath = "/dev/loop-control" loopDevFormat = "/dev/loop%d" - // According to util-linux/include/loopdev.h - ioctlSetFd = 0x4C00 - ioctlClrFd = 0x4C01 - ioctlSetStatus64 = 0x4C04 - ioctlGetFree = 0x4C82 - - loFlagsReadonly = 1 - //loFlagsUseAops = 2 - loFlagsAutoclear = 4 - //loFlagsPartScan = 8 - loFlagsDirectIO = 16 - ebusyString = "device or resource busy" ) -// parameters to control loop device setup +// LoopParams parameters to control loop device setup type LoopParams struct { // Loop device should forbid write Readonly bool @@ -58,29 +47,6 @@ type LoopParams struct { Direct bool } -// struct loop_info64 in util-linux/include/loopdev.h -type loopInfo struct { - /* - device uint64 - inode uint64 - rdevice uint64 - offset uint64 - sizelimit uint64 - number uint32 - encryptType uint32 - encryptKeySize uint32 - */ - _ [13]uint32 - flags uint32 - fileName [64]byte - /* - cryptName [64]byte - encryptKey [32]byte - init [2]uint64 - */ - _ [112]byte -} - func ioctl(fd, req, args uintptr) (uintptr, uintptr, error) { r1, r2, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, req, args) if errno != 0 { @@ -96,63 +62,70 @@ func getFreeLoopDev() (uint32, error) { return 0, errors.Errorf("could not open %v: %v", loopControlPath, err) } defer ctrl.Close() - num, _, err := ioctl(ctrl.Fd(), ioctlGetFree, 0) + num, _, err := ioctl(ctrl.Fd(), unix.LOOP_CTL_GET_FREE, 0) if err != nil { return 0, errors.Wrap(err, "could not get free loop device") } return uint32(num), nil } -func setupLoopDev(backingFile, loopDev string, param LoopParams) (devFile *os.File, err error) { +func setupLoopDev(backingFile, loopDev string, param LoopParams) error { // 1. Open backing file and loop device - oflags := os.O_RDWR + flags := os.O_RDWR if param.Readonly { - oflags = os.O_RDONLY + flags = os.O_RDONLY } - back, err := os.OpenFile(backingFile, oflags, 0) + + back, err := os.OpenFile(backingFile, flags, 0) if err != nil { - return nil, errors.Errorf("could not open backing file: %v", err) + return errors.Wrapf(err, "could not open backing file: %s", backingFile) } defer back.Close() - loopFile, err := os.OpenFile(loopDev, oflags, 0) + loop, err := os.OpenFile(loopDev, flags, 0) if err != nil { - return nil, errors.Errorf("could not open loop device: %v", err) + return errors.Wrapf(err, "could not open loop device: %s", loopDev) } - defer func() { - if err != nil { - loopFile.Close() - } - }() + defer loop.Close() // 2. Set FD - if _, _, err = ioctl(loopFile.Fd(), ioctlSetFd, back.Fd()); err != nil { - return nil, errors.Errorf("could not set loop fd: %v", err) + if _, _, err = ioctl(loop.Fd(), unix.LOOP_SET_FD, back.Fd()); err != nil { + return errors.Wrapf(err, "could not set loop fd for device: %s", loopDev) } // 3. Set Info - info := loopInfo{} - copy(info.fileName[:], []byte(backingFile)) + info := unix.LoopInfo64{} + copy(info.File_name[:], backingFile) if param.Readonly { - info.flags |= loFlagsReadonly + info.Flags |= unix.LO_FLAGS_READ_ONLY } + if param.Autoclear { - info.flags |= loFlagsAutoclear + info.Flags |= unix.LO_FLAGS_AUTOCLEAR } + if param.Direct { - info.flags |= loFlagsAutoclear + info.Flags |= unix.LO_FLAGS_DIRECT_IO } - if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil { + + _, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info))) + if err == nil { + return nil + } + + if param.Direct { // Retry w/o direct IO flag in case kernel does not support it. The downside is that // it will suffer from double cache problem. - info.flags &= ^(uint32(loFlagsDirectIO)) - if _, _, err := ioctl(loopFile.Fd(), ioctlSetStatus64, uintptr(unsafe.Pointer(&info))); err != nil { - ioctl(loopFile.Fd(), ioctlClrFd, 0) - return nil, errors.Errorf("cannot set loop info:%v", err) + info.Flags &= ^(uint32(unix.LO_FLAGS_DIRECT_IO)) + _, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info))) + if err == nil { + return nil } } - return loopFile, nil + // Cleanup loop fd and return error + _, _, _ = ioctl(loop.Fd(), unix.LOOP_CLR_FD, 0) + return errors.Errorf("failed to set loop device info: %v", err) } // setupLoop looks for (and possibly creates) a free loop device, and @@ -169,18 +142,15 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) (devFile *os.Fi // the loop device when done with it. // // Upon success, the file handle to the loop device is returned. -func setupLoop(backingFile string, param LoopParams) (*os.File, error) { - var loopDev string - +func setupLoop(backingFile string, param LoopParams) (string, error) { for retry := 1; retry < 200; retry++ { num, err := getFreeLoopDev() if err != nil { - return nil, err + return "", err } - loopDev = fmt.Sprintf(loopDevFormat, num) - loopFile, err := setupLoopDev(backingFile, loopDev, param) - if err != nil { + loopDev := fmt.Sprintf(loopDevFormat, num) + if err := setupLoopDev(backingFile, loopDev, param); err != nil { // Per util-linux/sys-utils/losetup.c:create_loop(), // free loop device can race and we end up failing // with EBUSY when trying to set it up. @@ -189,40 +159,36 @@ func setupLoop(backingFile string, param LoopParams) (*os.File, error) { time.Sleep(time.Millisecond * time.Duration(rand.Intn(retry*10))) continue } - return nil, err + return "", err } - return loopFile, nil + + return loopDev, nil } - return nil, errors.New("Timeout creating new loopback device") + return "", errors.New("timeout creating new loopback device") } func removeLoop(loopdev string) error { - dev, err := os.Open(loopdev) + file, err := os.Open(loopdev) if err != nil { return err } - _, _, err = ioctl(dev.Fd(), ioctlClrFd, 0) - dev.Close() + defer file.Close() + + _, _, err = ioctl(file.Fd(), unix.LOOP_CLR_FD, 0) return err } // Attach a specified backing file to a loop device func AttachLoopDevice(backingFile string) (string, error) { - dev, err := setupLoop(backingFile, LoopParams{}) - if err != nil { - return "", err - } - dev.Close() - - return dev.Name(), nil + return setupLoop(backingFile, LoopParams{}) } // Detach a loop device func DetachLoopDevice(devices ...string) error { for _, dev := range devices { if err := removeLoop(dev); err != nil { - return err + return errors.Wrapf(err, "failed to remove loop device: %s", dev) } } diff --git a/mount/losetup_linux_test.go b/mount/losetup_linux_test.go index 6ed1e0eb8..2b5383250 100644 --- a/mount/losetup_linux_test.go +++ b/mount/losetup_linux_test.go @@ -53,11 +53,11 @@ func TestSetupLoop(t *testing.T) { }() /* RO loop */ - f, err = setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true}) + path, err := setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true}) if err != nil { t.Fatal(err) } - ff, err := os.OpenFile(f.Name(), os.O_RDWR, 0) + ff, err := os.OpenFile(path, os.O_RDWR, 0) if err != nil { t.Fatal(err) } @@ -67,16 +67,13 @@ func TestSetupLoop(t *testing.T) { if err = ff.Close(); err != nil { t.Fatal(err) } - if err = f.Close(); err != nil { - t.Fatal(err) - } /* RW loop */ - f, err = setupLoop(backingFile, LoopParams{Autoclear: true}) + path, err = setupLoop(backingFile, LoopParams{Autoclear: true}) if err != nil { t.Fatal(err) } - ff, err = os.OpenFile(f.Name(), os.O_RDWR, 0) + ff, err = os.OpenFile(path, os.O_RDWR, 0) if err != nil { t.Fatal(err) } @@ -86,9 +83,6 @@ func TestSetupLoop(t *testing.T) { if err = ff.Close(); err != nil { t.Fatal(err) } - if err = f.Close(); err != nil { - t.Fatal(err) - } } func TestAttachDetachLoopDevice(t *testing.T) { diff --git a/mount/mount_linux.go b/mount/mount_linux.go index b0e24f8f0..7191d40b1 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -85,9 +85,8 @@ func (m *Mount) Mount(target string) (err error) { if err != nil { return err } - defer devFile.Close() // Mount the loop device instead - source = devFile.Name() + source = devFile } if err := mountAt(chdir, source, target, m.Type, uintptr(oflags), data); err != nil { return err From eb1649225ddbf2dddc6a28709cdf6009bdec5961 Mon Sep 17 00:00:00 2001 From: Maksym Pavlenko Date: Tue, 5 Jan 2021 10:06:02 -0800 Subject: [PATCH 4/4] Refactor loseup test Signed-off-by: Maksym Pavlenko --- mount/losetup_linux.go | 2 +- mount/losetup_linux_test.go | 86 +++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/mount/losetup_linux.go b/mount/losetup_linux.go index f88a9d41c..a0040dc18 100644 --- a/mount/losetup_linux.go +++ b/mount/losetup_linux.go @@ -143,7 +143,7 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) error { // // Upon success, the file handle to the loop device is returned. func setupLoop(backingFile string, param LoopParams) (string, error) { - for retry := 1; retry < 200; retry++ { + for retry := 1; retry < 100; retry++ { num, err := getFreeLoopDev() if err != nil { return "", err diff --git a/mount/losetup_linux_test.go b/mount/losetup_linux_test.go index 2b5383250..445bb05d0 100644 --- a/mount/losetup_linux_test.go +++ b/mount/losetup_linux_test.go @@ -26,85 +26,89 @@ import ( "github.com/containerd/continuity/testutil" ) -func TestSetupLoop(t *testing.T) { - testutil.RequiresRoot(t) - const randomdata = "randomdata" +var randomData = []byte("randomdata") - /* Non-existing loop */ - backingFile := "setup-loop-test-no-such-file" - _, err := setupLoop(backingFile, LoopParams{}) - if err == nil { - t.Fatalf("setupLoop with non-existing file should fail") - } +func createTempFile(t *testing.T) string { + t.Helper() f, err := ioutil.TempFile("", "losetup") if err != nil { t.Fatal(err) } + defer f.Close() + if err = f.Truncate(512); err != nil { t.Fatal(err) } - backingFile = f.Name() - f.Close() + + return f.Name() +} + +func TestNonExistingLoop(t *testing.T) { + testutil.RequiresRoot(t) + + backingFile := "setup-loop-test-no-such-file" + _, err := setupLoop(backingFile, LoopParams{}) + if err == nil { + t.Fatalf("setupLoop with non-existing file should fail") + } +} + +func TestRoLoop(t *testing.T) { + testutil.RequiresRoot(t) + + backingFile := createTempFile(t) defer func() { if err := os.Remove(backingFile); err != nil { t.Fatal(err) } }() - /* RO loop */ path, err := setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true}) if err != nil { t.Fatal(err) } - ff, err := os.OpenFile(path, os.O_RDWR, 0) - if err != nil { - t.Fatal(err) - } - if _, err = ff.Write([]byte(randomdata)); err == nil { + + if err := ioutil.WriteFile(path, randomData, os.ModePerm); err == nil { t.Fatalf("writing to readonly loop device should fail") } - if err = ff.Close(); err != nil { +} + +func TestRwLoop(t *testing.T) { + testutil.RequiresRoot(t) + + backingFile := createTempFile(t) + defer func() { + if err := os.Remove(backingFile); err != nil { + t.Fatal(err) + } + }() + + path, err := setupLoop(backingFile, LoopParams{Autoclear: true}) + if err != nil { t.Fatal(err) } - /* RW loop */ - path, err = setupLoop(backingFile, LoopParams{Autoclear: true}) - if err != nil { - t.Fatal(err) - } - ff, err = os.OpenFile(path, os.O_RDWR, 0) - if err != nil { - t.Fatal(err) - } - if _, err = ff.Write([]byte(randomdata)); err != nil { - t.Fatal(err) - } - if err = ff.Close(); err != nil { + if err := ioutil.WriteFile(path, randomData, os.ModePerm); err != nil { t.Fatal(err) } } func TestAttachDetachLoopDevice(t *testing.T) { testutil.RequiresRoot(t) - f, err := ioutil.TempFile("", "losetup") - if err != nil { - t.Fatal(err) - } - if err = f.Truncate(512); err != nil { - t.Fatal(err) - } - f.Close() + + path := createTempFile(t) defer func() { - if err := os.Remove(f.Name()); err != nil { + if err := os.Remove(path); err != nil { t.Fatal(err) } }() - dev, err := AttachLoopDevice(f.Name()) + dev, err := AttachLoopDevice(path) if err != nil { t.Fatal(err) } + if err = DetachLoopDevice(dev); err != nil { t.Fatal(err) }