Re: [PATCH 1/2] Check if last partition ends at GPT backup header

[Henning Schild <henning.schild@siemens.com>]

Am Wed, 9 Nov 2022 10:40:20 +0100
schrieb "Schaffner, Tobias (T CED SES-DE)"
<tobias.schaffner@siemens.com>:

> On 09.11.22 09:26, Schild, Henning (T CED SES-DE) wrote:
> > Am Tue, 8 Nov 2022 12:28:36 +0100
> > schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> >   
> >> From: Tobias Schaffner <tobias.schaffner@siemens.com>
> >>
> >> The GPT backup header has a fixed size of 33 512 byte blocks. Check
> >> if the last partition ends at the block before the start of the
> >> GUID partition table backup header. If so the partition is fully
> >> expanded.
> >>
> >> Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
> >> ---
> >>   .../files/expand-last-partition.sh            | 21
> >> +++++++++---------- 1 file changed, 10 insertions(+), 11
> >> deletions(-)
> >>
> >> diff --git
> >> a/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
> >> b/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
> >> index 57055cc..0d662cc 100755 ---
> >> a/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
> >> +++
> >> b/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
> >> @@ -22,24 +22,23 @@ if [ "${ROOT_DEV}" = "${BOOT_DEV}" ]; then
> >> exit 1 fi -# this value is in blocks. Normally a block has 512
> >> bytes. -BUFFER_SIZE=32768
> >>   BOOT_DEV_NAME=${BOOT_DEV##*/}
> >> +LAST_PART_NAME="$(lsblk -l -o NAME "${BOOT_DEV}" | tail -1)"
> >> +LAST_PART="/dev/${LAST_PART_NAME}"
> >> +
> >>   DISK_SIZE="$(cat /sys/class/block/"${BOOT_DEV_NAME}"/size)"
> >> -ALL_PARTS_SIZE=0
> >> -for PARTITION in
> >> /sys/class/block/"${BOOT_DEV_NAME}"/"${BOOT_DEV_NAME}"*; do
> >> -	PART_SIZE=$(cat "${PARTITION}"/size)
> >> -	ALL_PARTS_SIZE=$((ALL_PARTS_SIZE + PART_SIZE))
> >> -done
> >> +LAST_PART_SIZE="$(cat /sys/class/block/"${LAST_PART_NAME}"/size)"
> >> +LAST_PART_START="$(cat
> >> /sys/class/block/"${LAST_PART_NAME}"/start)" +
> >> +# The GUID partition table stores its backup in the last 33 blocks
> >> of the table. +# Therefore the last partition ends 33 before the
> >> end of the disk if expanded. +GPT_BACKUP_SIZE=33
> >>   
> >> -MINIMAL_SIZE=$((ALL_PARTS_SIZE + BUFFER_SIZE))
> >> -if [ "$DISK_SIZE" -lt "$MINIMAL_SIZE" ]; then
> >> +if [ $((LAST_PART_START + LAST_PART_SIZE + GPT_BACKUP_SIZE)) -lt
> >> "${DISK_SIZE}" ]; then echo "Disk is practically already full,
> >> doing nothing." >&2 exit 0  
> > 
> > There was a good reason for that. I think when for alignment
> > reasons or so it looks like there is a little bit of space left.
> > But the resize would then not work or we would in fact try a shrink
> > and fail. Here you seem the switch the logic from "practically
> > full" to "really full" and remove that margin where a resize would
> > be tried but would fail.
> > 
> > The trick with last_part start + size looks nicer than summing up
> > the whole size.
> > 
> > Henning  
> 
> Any ideas how I could reproduce such a case?
> 
> I have not seen any partition alignment in my tests so far. The only 
> thing that has to be respected is the GUID backup header in the end
> of the partition.
> 
> What I have seen is a alignment of the filesystem inside of the 
> partition. Ext4 is aligned to a minimum of 4096 bytes / 8 sysfs
> blocks. But this is not relevant in the check if the partition was
> expanded.

I think the repro was with an example image booted in qemu without
growing that image with qemu-img convert. In which case the partition
table resize was a noop but the fs resize wanted to shrink for alignment
reasons. And that failed because the fs was just very full. Might be
hard to repro but also not too smart to basically revert the patch that
introduced that "close to full ... better not touch".

Likely not all versions/distros affected in the same way. And it was
with resize2fs, not sure how systemd would react but i assume it will
in effect also call resize2fs at the end of the day.

https://github.com/ilbers/isar/commit/bd9bd34c978829ffc13d61c36af90363030c2349

The commit does not explain in detail which combination was "broken".
May be spread somewhere in the discussions that have been going on
around that.

I took Tobias Schmidl on Cc, maybe he will answer. But we might be on
our own.

I have sent a patch to CI test expand-on-first-boot. While that still
needs a rewrite, it can already be used to test this series. Maybe you
find your repro that way.

Henning

> 
> All the best
> 
> >>   fi
> >>   
> >> -LAST_PART="$(sfdisk -d "${BOOT_DEV}" 2>/dev/null | tail -1 | cut
> >> -d ' ' -f 1)" -
> >>   # Transform the partition table as follows:
> >>   #
> >>   # - Remove any 'last-lba' header so sfdisk uses the entire
> >> available space.  
> >