Debian / raspi3-image-spec

contains the files to build the https://wiki.debian.org/RaspberryPi3 image
127 stars 32 forks source link

New resizerootfs #28

Closed chschlue closed 6 years ago

chschlue commented 6 years ago

More clarity, less Perl masturbation.

Fixes #15 and #20

gwolf commented 6 years ago

I completely go with following this direction over the very correctly named "perl masturbation" mounstrosity. However, I have a couple of questions:

chschlue commented 6 years ago

Yes, the script is sequential, but systemd is not.

The sleeps are there because partprobe failed occasionally during testing, even though one would think the boot process overall should be pretty much deterministic. Prolonging the first boot by 10 seconds doesn't hurt much IMO, so it's a case of better safe than sorry. One second would probably be more than enough but I chose not to boot 100 times each with different settings to optimize a script which only runs once. My spare time doesn't grow on trees ;-)

stapelberg commented 6 years ago

I completely go with following this direction over the very correctly named "perl masturbation" mounstrosity.

As the new maintainer of this project, it’s up to you to set the tone. I think calling my work a monstrosity isn’t very nice, and arrogant terms such as “perl masturbation” should be objected to, not encouraged.

No hard feelings on my end, but can we please all strive to be constructive, respectful and welcoming? A pleasant tone goes a long way in attracting contributors. Thank you.

gwolf commented 6 years ago

Sorry, Michael, I have to agree you are completely right. I spent a long time reading and understanding the resizerootfs you wrote - I had done several similar things in the past, and the effort you got into getting all the bits, the spelling out of the block layout, all that... I think it is excessive, and it seems to me Christian's take is trustable enough to do a good job on this. I do understand the need for the udevadm settle (as otherwise it's possible to attempt a FS resize before the kernel knows the FS new dimensions). I do believe the sleep 5 are not necessarily needed - but, yes, the penalty is very little... And, yes, thanks, I will remove the parted addition.

gwolf commented 6 years ago

And, again – I am sorry for the disrespectful tone. It is not the least warranted. You did a hell of a work. And I can say I am among the masters of overengineering :-P

chschlue commented 6 years ago

And, yes, thanks, I will remove the parted addition.

Don't! It's needed for partprobe.

I also don't think the sleeps should be necessary, but as I said, it occasionally broke without them while testing other changes.

@stapelberg Sry, agreed, "perl masturbation" was a bit harsh. Still, I stand by my point that it's unnecessarily brittle and hard to maintain.

stapelberg commented 6 years ago

Thanks for the reasonable replies, everyone.

gwolf commented 6 years ago

Right - Dependency not removed then. Thanks for saving me the time and frustration of finding out!

chschlue commented 6 years ago

@stapelberg Out of curiosity, would you tell me why you went through the effort of writing the original script in the first place?

stapelberg commented 6 years ago

Sure. Note that in its original form, it was 125 lines and a bit easier to understand: https://github.com/Debian/raspi3-image-spec/blob/331f68931a0726f2c790aa69bf1d6f23054c6321/rpi3-resizerootfs

I had also looked at how other distributions were resizing the root partition and the solutions I found were pretty clumsy (some resized the root partition and rebooted), so I wanted to implement a way which would be fast and not require any reboots.

I knew about the required syscalls (BLKGETSIZE64 to find the device size in bytes, BLKPG to make the kernel re-read the partition table) from another project (https://gokrazy.org), and the only remaining task was increasing a single uint32 in the partition table.

Calling these syscalls from a Perl script seemed like a simple way without any extra dependencies.

Comparing the two implementations, I didn’t know that lsblk could be used to quickly identify the root device, and I wasn’t familiar with sfdisk’s syntax/capabilities. I’m a bit torn on the BLKPG call vs. the sleep, udevadm settle, sleep, partprobe sequence — the BLKPG call definitely is faster, but I don’t fully understand whether the issues you mentioned would also happen with it, or whether they are inherent to the new sequence of commands.

To summarize: my experience was from a very different angle :)

chschlue commented 6 years ago

Thanks for the insight. Now I get it. Seemed masochistic to me before, no offense.

A summary of my motivation, in case you want to know:

stapelberg commented 6 years ago

Yeah, I understand the advantages of the current solution.

I’m not in the critical path here, so take my word for what it’s worth, but I think speed is important: a user will be more delighted by an image which boots within 10 seconds than one that boots within 20 seconds, even if it’s the first boot (the user doesn’t necessarily know that the first boot is different from other boots) :)

chschlue commented 6 years ago

As this is basically in an experimental state anyway: How about removing the sleeps for now and waiting for user feedback?