From 2a1ad27713f1f3ae73fc0165d171e249f15a93e0 Mon Sep 17 00:00:00 2001 From: Christian Kellner Date: Tue, 15 Nov 2022 18:58:28 +0100 Subject: [PATCH] disk: align LVM2 volumes to the extent size When the size of a logical volume is not aligned to the extent size of the volume group, LVM2 will automatically align it by rounding up[1]: Rounding up size to full physical extent 29.80 GiB Rounding up size to full physical extent <3.82 GiB Since we don't take that into account when we create a new volume or set the size of an existing one, the size for the whole volume group will be short by that amount and thus the creation of the last volume will fail: Volume group has insufficient free space (975 extents): 977 required. To fix this a new `AlignUp` method is added to the `MountpointCreator` creator interface. It will align a given size to the requirements of the implementing container, like e.g. `LVMVolumeGroup`. It is then used by a new `alignEntityBranch` which takes a size and walks the entity path, calling `AlignUp` for all entities that implement said `MountpointCreator` interface; thus the resulting size should fullfil the alignment requirement for all elements in the path. NB: `PartitionTable` already had an `AlignUp` method. Add a corresponding test. [1]: https://github.com/lvmteam/lvm2/blob/868665766491d9d42b8acedaf07cafc7118d165c/lib/metadata/metadata.c#L1072 --- internal/disk/btrfs.go | 4 ++ internal/disk/disk.go | 4 ++ internal/disk/disk_test.go | 86 ++++++++++++++++++++++++++++++++ internal/disk/lvm.go | 17 ++++++- internal/disk/partition_table.go | 34 +++++++++---- 5 files changed, 135 insertions(+), 10 deletions(-) diff --git a/internal/disk/btrfs.go b/internal/disk/btrfs.go index a4f0e658399..5f48c7cc22c 100644 --- a/internal/disk/btrfs.go +++ b/internal/disk/btrfs.go @@ -67,6 +67,10 @@ func (b *Btrfs) CreateMountpoint(mountpoint string, size uint64) (Entity, error) return &b.Subvolumes[len(b.Subvolumes)-1], nil } +func (b *Btrfs) AlignUp(size uint64) uint64 { + return size // No extra alignment necessary for subvolumes +} + func (b *Btrfs) GenUUID(rng *rand.Rand) { if b.UUID == "" { b.UUID = uuid.Must(newRandomUUIDFromReader(rng)).String() diff --git a/internal/disk/disk.go b/internal/disk/disk.go index 85680feb3cf..14fa14d4439 100644 --- a/internal/disk/disk.go +++ b/internal/disk/disk.go @@ -120,6 +120,10 @@ type Mountable interface { // returns the entity that represents the new mountpoint. type MountpointCreator interface { CreateMountpoint(mountpoint string, size uint64) (Entity, error) + + // AlignUp will align the given bytes according to the + // requirements of the container type. + AlignUp(size uint64) uint64 } // A UniqueEntity is an entity that can be uniquely identified via a UUID. diff --git a/internal/disk/disk_test.go b/internal/disk/disk_test.go index 7e98ced6268..e77c5c47e7d 100644 --- a/internal/disk/disk_test.go +++ b/internal/disk/disk_test.go @@ -617,6 +617,92 @@ func TestMinimumSizes(t *testing.T) { } } +func TestLVMExtentAlignment(t *testing.T) { + assert := assert.New(t) + + // math/rand is good enough in this case + /* #nosec G404 */ + rng := rand.New(rand.NewSource(13)) + pt := testPartitionTables["plain"] + + type testCase struct { + Blueprint []blueprint.FilesystemCustomization + ExpectedSizes map[string]uint64 + } + + testCases := []testCase{ + { + Blueprint: []blueprint.FilesystemCustomization{ + { + Mountpoint: "/var", + MinSize: 1*GiB + 1, + }, + }, + ExpectedSizes: map[string]uint64{ + "/var": 1*GiB + LVMDefaultExtentSize, + }, + }, + { + // lots of mount points in /var + // https://bugzilla.redhat.com/show_bug.cgi?id=2141738 + Blueprint: []blueprint.FilesystemCustomization{ + { + Mountpoint: "/", + MinSize: 32000000000, + }, + { + Mountpoint: "/var", + MinSize: 4096000000, + }, + { + Mountpoint: "/var/log", + MinSize: 4096000000, + }, + }, + ExpectedSizes: map[string]uint64{ + "/": 32002539520, + "/var": 3908 * MiB, + "/var/log": 3908 * MiB, + }, + }, + { + Blueprint: []blueprint.FilesystemCustomization{ + { + Mountpoint: "/", + MinSize: 32 * GiB, + }, + { + Mountpoint: "/var", + MinSize: 4 * GiB, + }, + { + Mountpoint: "/var/log", + MinSize: 4 * GiB, + }, + }, + ExpectedSizes: map[string]uint64{ + "/": 32 * GiB, + "/var": 4 * GiB, + "/var/log": 4 * GiB, + }, + }, + } + + for idx, tc := range testCases { + mpt, err := NewPartitionTable(&pt, tc.Blueprint, uint64(3*GiB), true, rng) + assert.NoError(err) + for mnt, expSize := range tc.ExpectedSizes { + path := entityPath(mpt, mnt) + assert.NotNil(path, "[%d] mountpoint %q not found", idx, mnt) + parent := path[1] + part, ok := parent.(*LVMLogicalVolume) + assert.True(ok, "[%d] %q parent (%v) is not an LVM logical volume", idx, mnt, parent) + assert.Equal(part.GetSize(), expSize, + "[%d] %q size %d should be equal to %d", idx, mnt, part.GetSize(), expSize) + } + } +} + func TestNewBootWithSizeLVMify(t *testing.T) { pt := testPartitionTables["plain-noboot"] assert := assert.New(t) diff --git a/internal/disk/lvm.go b/internal/disk/lvm.go index 539e4665830..809f5e3f952 100644 --- a/internal/disk/lvm.go +++ b/internal/disk/lvm.go @@ -3,8 +3,14 @@ package disk import ( "fmt" "strings" + + "github.com/osbuild/osbuild-composer/internal/common" ) +// Default physical extent size in bytes: logical volumes +// created inside the VG will be aligned to this. +const LVMDefaultExtentSize = 4 * common.MebiByte + type LVMVolumeGroup struct { Name string Description string @@ -105,7 +111,7 @@ func (vg *LVMVolumeGroup) CreateLogicalVolume(lvName string, size uint64, payloa lv := LVMLogicalVolume{ Name: name, - Size: size, + Size: vg.AlignUp(size), Payload: payload, } @@ -114,6 +120,15 @@ func (vg *LVMVolumeGroup) CreateLogicalVolume(lvName string, size uint64, payloa return &vg.LogicalVolumes[len(vg.LogicalVolumes)-1], nil } +func (vg *LVMVolumeGroup) AlignUp(size uint64) uint64 { + + if size%LVMDefaultExtentSize != 0 { + size += LVMDefaultExtentSize - size%LVMDefaultExtentSize + } + + return size +} + func (vg *LVMVolumeGroup) MetadataSize() uint64 { if vg == nil { return 0 diff --git a/internal/disk/partition_table.go b/internal/disk/partition_table.go index 91719deb0e3..27be1173e46 100644 --- a/internal/disk/partition_table.go +++ b/internal/disk/partition_table.go @@ -326,8 +326,9 @@ func (pt *PartitionTable) applyCustomization(mountpoints []blueprint.FilesystemC newMountpoints := []blueprint.FilesystemCustomization{} for _, mnt := range mountpoints { - size := pt.AlignUp(clampFSSize(mnt.Mountpoint, mnt.MinSize)) + size := clampFSSize(mnt.Mountpoint, mnt.MinSize) if path := entityPath(pt, mnt.Mountpoint); len(path) != 0 { + size = alignEntityBranch(path, size) resizeEntityBranch(path, size) } else { if !create { @@ -427,6 +428,7 @@ func (pt *PartitionTable) createFilesystem(mountpoint string, size uint64) error return fmt.Errorf("failed creating volume: " + err.Error()) } vcPath := append([]Entity{newVol}, rootPath[idx:]...) + size = alignEntityBranch(vcPath, size) resizeEntityBranch(vcPath, size) return nil } @@ -509,6 +511,20 @@ func clampFSSize(mountpoint string, size uint64) uint64 { return size } +func alignEntityBranch(path []Entity, size uint64) uint64 { + if len(path) == 0 { + return size + } + + element := path[0] + + if c, ok := element.(MountpointCreator); ok { + size = c.AlignUp(size) + } + + return alignEntityBranch(path[1:], size) +} + // resizeEntityBranch resizes the first entity in the specified path to be at // least the specified size and then grows every entity up the path to the // PartitionTable accordingly. @@ -579,18 +595,18 @@ func (pt *PartitionTable) ensureLVM() error { } else if part, ok := parent.(*Partition); ok { filesystem := part.Payload - part.Payload = &LVMVolumeGroup{ + vg := &LVMVolumeGroup{ Name: "rootvg", Description: "created via lvm2 and osbuild", - LogicalVolumes: []LVMLogicalVolume{ - { - Size: part.Size, - Name: "rootlv", - Payload: filesystem, - }, - }, } + _, err := vg.CreateLogicalVolume("root", part.Size, filesystem) + if err != nil { + panic(fmt.Sprintf("Could not create LV: %v", err)) + } + + part.Payload = vg + // reset it so it will be grown later part.Size = 0