procsfdisk.awk "original_variable" calulation?

george1421

@h3krn While I can’t answer the resize bit, I do have to ask are any of these LVM volumes? ATM FOG doesn’t handle expanding/shrinking LVM volumes well.

H3Krn

@george1421 Nope. Only a EFI partition (sda1) and ext4 root (sda5)

george1421

@h3krn OK good enough as long as lvm is not in the picture and the partitions are just regular old partitions then we can discount that as being an issue.

I can tell you that the developers will probably want to see the content of the following files for this image.

d1.fixed_size_partitions
d1.original.fstypes
d1.minimum.partitions
d1.partitions

They will all be in the /images/<image_name> directory on the FOG server. The content of these files describe the captured image.

H3Krn

These are the files as are currently in my image. I’ve manually modified “d1.fixed_size_partitions” which originally contained “1:2” and “d1.partitions” setting the size of the extended and logical partition to 99999999999 as I would like these partitions to fill the entire available space, whatever hardware or VM I deploy to. (Be it a VM with only 64GB disk or a PC with either 256 or 500GB disk)

d1.fixed_size_partitions

1:

d1.original.fstypes

/dev/sda5 extfs

d1.minimum.partitions

label: dos
label-id: 0x964ef18c
device: /dev/sda
unit: sectors
sector-size: 512

/dev/sda1 : start=        2048, size=     1048576, type=b, bootable
/dev/sda2 : start=     1052670, size=   103802882, type=5
/dev/sda5 : start=     1052672, size=    54399852, type=83

d1.partitions

label: dos
label-id: 0x964ef18c
device: /dev/sda
unit: sectors
sector-size: 512

/dev/sda1 : start=        2048, size=     1048576, type=b, bootable
/dev/sda2 : start=     1052670, size= 99999999999, type=5
/dev/sda5 : start=     1052672, size= 99999999999, type=83

H3Krn

While debugging my resizing issue on FOS (or rather lack of resizing), I found FOS is executing this command:

$ ./procsfdisk.awk -v SECTOR_SIZE=512 -v CHUNK_SIZE=512 -v MIN_START=2048 -v action=filldisk -v target=/dev/sda -v sizePos=134217728 -v diskSize=134217728 -v fixedList=1: d1.minimum.partitions d1.partitions
# Partition table is consistent.
label: dos
label-id: 0x964ef18c
device: /dev/sda
unit: sectors
sector-size: 512

/dev/sda1 : start=        2048, size=     1048576, type=b, bootable
/dev/sda2 : start=     1052670, size=   103802882, type=5
/dev/sda5 : start=     1052672, size=    66580992, type=83

My expectations would have been sda2 & sda5 to be around 133167104 in size. This led me to poking around in procsfdisk.awk and by adding some extra print statements I noticed “original_variable” was assigned 199999999998 which is the sum of my extended and logical partition. (While “new_variable” is the disk size minus space used by fixed size partitions)

Sebastian Roth

@H3Krn I think I remember a post about an issue with extended partitions some time ago but can’t seem to find it in the forums. Pretty sure you are on the right track here. Great you have dug into this fairly deep already. Thanks!

I can take a look at this as well but won’t be before next week I am afraid. So if you keep poking I can see you find and fix the bug before I get to it.

Just in case you did not already know: For quick debugging you can edit the script while in FOS debug mode using vi.

H3Krn

@sebastian-roth I won’t claim to completely understand the procsfdisk code, but I’m currently suspecting the “if” clause on line 436.

436                if (match(fixedList, regex)) {
437                    continue;
438                }

When encountering a extended partion, I think it’s size should never be taken into account when trying to resize variable primary or logical partitions. So the for loop should break there in whatever case.

I’ve removed that “if” clause, leaving the “continue;” in place. This gives me:

./procsfdisk.awk -v SECTOR_SIZE=512 -v CHUNK_SIZE=512 -v MIN_START=2048 -v action=filldisk -v target=/dev/sda -v sizePos=134217728 -v diskSize=134217728 -v fixedList=1: d1.minimum.partitions d1.partitions 
# Partition table is consistent.
label: dos
label-id: 0x964ef18c
device: /dev/sda
unit: sectors
sector-size: 512

/dev/sda1 : start=        2048, size=     1048576, type=b, bootable
/dev/sda2 : start=     1052670, size=   133162498, type=5
/dev/sda5 : start=     1052672, size=   133162496, type=83

To me this seems a much more believable layout. So I’ve patched my init.xz, copying in the new procsfdisk.awk script using the postinitscript method. This seems to solve the issue for me. (… after deleting the d1p2.* files from my image as they overwrite the new layout of the extended partition.)

rodluz

@h3krn & @Sebastian-Roth I may be completely wrong about this one (I haven’t really taken a deep dive into this file) but looking at the code, is the issue in the if statement in line 457? I only say this because the comment contradicts the statement. Should the ! be removed from the if statement?

456        # If fixed, add the partition size.
457        if (!match(fixedList, regex)) {
458            # Increment the variable value.
459            original_variable += p_size;
460            continue;
461        }

Tom Elliott

@rodluz It’s been a very long time since I’ve had to work on this file.

I suspect the comment is correct though, even if the if statement is odd (but I’d have to review the whole of procsfdisk.awk for full understanding. A few lines out of the whole isn’t generally enough of an overview, but that doesn’t mean we didn’t make a mistake either.

H3Krn

@rodluz allthough that comment did confuse me. I don’t think it is related to my issue with calculation of the extended partition. Currently what I know of the filldisk functions is:

It sums the sizes of all fixed partitions from the image in “original_fixed”
It sums the sizes of all variable partitions from the image in “original_variable”
It calculates “new_available” (new storage available for variable size partitions) by subtracting “original_fixed” from the new disk size.
It calculates a factor to resize all variable partitions by dividing “new_variable” by “original_variable”.

My specific problem is that the size of the extended partition is added to “original_variable” whereas it this should be completely ignored in the resize factoring. (only variable logical partitions should be added).

All in all, I think when a variable partition is encountered, that iteration of the for loop should never arrive at lines 456.

On a separate note, it now occurs to me that a fixed extended partition might also not be properly handled. If the extended partition is marked as fixed, I think that “fixed” flag should be inherited to the logical partitions. As you simply can’t resize logical partitions to outside of the extended one.

Hope this all makes sense?

Tom Elliott

@rodluz To your point I re-evaluated the file and again, you took these lines out of context:
The full scope of it is:

        # If fixed, add the partition size.
        if (!match(fixedList, regex)) {
            # Increment the variable value.
            original_variable += p_size;
            continue;
        }
        original_fixed += p_size;
        partitions[pName, "orig_size"] = p_size;

Essentially my approach to scripting is to do reverse style coding. What do I mean by this:

Do things that I expect to work a certain way after checking the reverse.

This allows less nesting statements (which makes managing the code a little easier in my eyes.)

So while you’re right that the comment doesn’t appear to match, we have it setting original_variable, and if it is in the fixed list, it gets put into original_fixed.

Specifically the continue says okay well that’s not true, so put it in this point and skip the rest of the loop it’s in. If that’s false, it will add it to the fixed list.

Tom Elliott

@h3krn I believe 436 thru 438 is safe to remove, but I’m trying to grapple my mind in why that was added.

I suspect the mentality was if it’s fixed skip trying to do anything resizing but would skip everything below it. I’ll try to think about what I was thinking here, but for now I believe it’s safe to just comment these 3 lines out.

These may have also been for debugging and I just forgot to remove them lol.

H3Krn

@tom-elliott I did remove the “if” clause on lines 436 and 438. But I left the continue; statement at line 437. That way the size of the extended partition is left out of scope for the resize variables starting line 457

See here: https://github.com/FOGProject/fos/pull/52

(Feel free to reject or disregard that PR if you feel it could have been done better.)

Sebastian Roth

@H3Krn Still have not found enough time to look into this thoroughly. Will do so soon. Thanks for your PR. We’ll discuss further details there.

Edit: Just found the other topic I was talking about before: https://forums.fogproject.org/topic/16106/resizing-extfs-volume-done-but-system-partition-not-expanded-to-disk-size

H3Krn

@sebastian-roth, no worries. Thnx for the update. I had a quick glance at the other topic but I’ll need to read I properly to make any accurate observations

Current causes I found for my own case:

The bug in the procsfdisk.awk resize script (fixed for myself with the PR)
After resizing partitions during a restore task the contents of the extended partition table (d1p2 in my case) is written to the disk. This contains the layout of the original small logical partitions. (solved by deleting d1p2.ebr &d1p2.img from the image)

For now I have my setup working. Just hoping I can help you guys with it.

H3Krn

Hi all,

been A while. I guess we’ve all been busy. Just wanted to check back in if there’s anything I can still help with on this topic?

Grtz, Harm

Sebastian Roth

@H3Krn Thanks for checking up in this. I’ve got it on my list but don’t seem to find the time.

@rodluz Do you want to take a look at this?

procsfdisk.awk "original_variable" calulation?

271

11.6k

17.1k

154.6k