Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disk: align LVM2 volumes to the extent size #3137

Merged
merged 2 commits into from Nov 18, 2022

Conversation

gicmo
Copy link
Contributor

@gicmo gicmo commented Nov 15, 2022

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 (see metadata.c):

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 <uuid> 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.

@thozza thozza self-requested a review November 15, 2022 18:30
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few minor comments, otherwise this looks great!

Thanks for a quick fix.

internal/disk/lvm.go Outdated Show resolved Hide resolved
internal/disk/btrfs.go Outdated Show resolved Hide resolved
internal/disk/lvm.go Show resolved Hide resolved
internal/disk/lvm.go Outdated Show resolved Hide resolved
gicmo and others added 2 commits November 17, 2022 18:21
Extract a `CreateLogicalVolume` method from `CreateMountpoint`
and implement the latter via the former. This makes it possible
to create a Logical Volume for an existing payload.
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 <uuid> 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

Co-authored-by: Achilleas Koutsou <achilleas@koutsou.net>
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@achilleas-k achilleas-k merged commit 9ea58d1 into osbuild:main Nov 18, 2022
@gicmo gicmo deleted the lvm_extent_alignment branch November 18, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants