procsfdisk.awk "original_variable" calulation?
-
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=83My 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)
-
@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. -
@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=83To 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.)
-
@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 } -
@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.
-
@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?
-
@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.
-
@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.
-
@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.)
-
@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
-
@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.
-
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
-
