Issue 6759: reduce size of stateful partition for non-developer recovery image

Issue 6759: reduce size of stateful partition for non-developer recovery image

Change-Id: I636f3156e549508230515174edffaff650c9d46e

BUG=chromium-os:6759
TEST=(1)manually built a non-developer recovery image and verified recovery install
completes on an agz device and the device is able to boot from HD upon reboot
(2) repeat for a developer recovery image and verified dev image from dev_payload is installed onto target and the target is able to boot from HD

[Tan Gao, 2010-09-24 22:12:28]

Hi Will,

This CL fixes issue 6759 by reducing size of stateful partition when creating a
non-developer recovery image based on a pristine image generated by build_image.
I also refactored some partition resize logic out of
mod_image_for_dev_recovery.sh into a separate script
resize_stateful_partition.sh so that the common logic may be shared between
mod_image_for_recovery.sh and mod_image_for_dev_recovery.sh (and potentially any
other script that needs to resize stateful partition).

PTAL thx!
Tan

[Will Drewry, 2010-10-05 15:28:02]

mostly looks good with some minor cleanup and questions

thanks!

http://codereview.chromium.org/3391018/diff/21001/22002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/21001/22002#newcode89
mod_image_for_recovery.sh:89: # convert floating point number to integer and
round up to 4Mb blocks
4Mb?

http://codereview.chromium.org/3391018/diff/21001/22002#newcode96
mod_image_for_recovery.sh:96: seek=$((512 * ${new_sectors} - 1)) #&>/dev/null
#&>?

http://codereview.chromium.org/3391018/diff/21001/22002#newcode106
mod_image_for_recovery.sh:106: sudo mkfs.ext3 "${new_loop_dev}"
Since you're shrinking the fs, it'd make sense to pass in -b 4096 to ensure that
it doesn't pick a smaller block size.

http://codereview.chromium.org/3391018/diff/21001/22002#newcode110
mod_image_for_recovery.sh:110: #sudo mount -o loop=${new_loop_dev}
"${new_state}" "${new_mnt}"
Is this comment no longer needed?

http://codereview.chromium.org/3391018/diff/21001/22002#newcode114
mod_image_for_recovery.sh:114: sudo tar cf - * | ( cd ${new_mnt}; sudo tar xfp
-)
Use . instead of * so it doesn't miss any existing dot files (like .recovery :)

As a nit, I tend to prefer to keep both sides matching even if the arguments are
really meant for extraction:

sudo tar clp . | sudo tar xlp -C $new_mnt 

Also, if you end up using my version, ditch the attribution - if not, keep it :)

http://codereview.chromium.org/3391018/diff/21001/22002#newcode119
mod_image_for_recovery.sh:119: local temp_img=$(update_partition_table $src_img
$new_state $new_sectors)
have you tried the output with a non-cros_fw machine?  I'm curious if you're
getting the right legacy mbr in place and the right syslinux values in the ESP.

Not a huge deal since recovery is pretty tied to the custom firmware.

http://codereview.chromium.org/3391018/diff/21001/22003
File resize_stateful_partition.sh (right):

http://codereview.chromium.org/3391018/diff/21001/22003#newcode7
resize_stateful_partition.sh:7: # Utility methods used to resize a stateful
partition and updates GPT table
nit: and update the

http://codereview.chromium.org/3391018/diff/21001/22003#newcode34
resize_stateful_partition.sh:34: # Due to this resize, we need to create a new
partition table and a new image.
I'm inclined to suggest that you rename this function "enlarge_partition_image"
or something similar because it enlarges the file in place and not a partition
in an actual disk image or drive.

http://codereview.chromium.org/3391018/diff/21001/22003#newcode49
resize_stateful_partition.sh:49: dd if=/dev/zero of="${source_part}" bs=1
count=1 \
nit: count can be = 0 and dd will expand it sparsely.

http://codereview.chromium.org/3391018/diff/21001/22003#newcode81
resize_stateful_partition.sh:81: "${temp_pmbr}" "${esp_count}" false $(roundup
${rootfs_count}) &>/dev/null
Why are you silencing the output?

http://codereview.chromium.org/3391018/diff/21001/22003#newcode83
resize_stateful_partition.sh:83: rm -rf "${temp_pmbr}"
Should this be done in a trap in case the caller has done a "set -e" and
install_gpt fails?

http://codereview.chromium.org/3391018/diff/21001/22003#newcode88
resize_stateful_partition.sh:88: &>/dev/null
It might make more sense to redirect stderr instead of slapping /dev/null on all
of these calls, but it's okay as is too.

[Tan Gao, 2010-10-06 18:20:55]

Thanks for the thorough review, Will!

http://codereview.chromium.org/3391018/diff/21001/22002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/21001/22002#newcode89
mod_image_for_recovery.sh:89: # convert floating point number to integer and
round up to 4Mb blocks
in chromeos-common.sh, the comment for roundup() method states that we align our
partitions to 2Mb block.

s/4Mb/2Mb/

http://codereview.chromium.org/3391018/diff/21001/22002#newcode96
mod_image_for_recovery.sh:96: seek=$((512 * ${new_sectors} - 1)) #&>/dev/null
removed '#'

http://codereview.chromium.org/3391018/diff/21001/22002#newcode106
mod_image_for_recovery.sh:106: sudo mkfs.ext3 "${new_loop_dev}"
added. along with comment

http://codereview.chromium.org/3391018/diff/21001/22002#newcode110
mod_image_for_recovery.sh:110: #sudo mount -o loop=${new_loop_dev}
"${new_state}" "${new_mnt}"
good catch! removed

http://codereview.chromium.org/3391018/diff/21001/22002#newcode114
mod_image_for_recovery.sh:114: sudo tar cf - * | ( cd ${new_mnt}; sudo tar xfp
-)
s/*/./

keeping original version to save some testing :-)

http://codereview.chromium.org/3391018/diff/21001/22002#newcode119
mod_image_for_recovery.sh:119: local temp_img=$(update_partition_table $src_img
$new_state $new_sectors)
will try this and update bug if recovery doesn't work

http://codereview.chromium.org/3391018/diff/21001/22003
File resize_stateful_partition.sh (right):

http://codereview.chromium.org/3391018/diff/21001/22003#newcode7
resize_stateful_partition.sh:7: # Utility methods used to resize a stateful
partition and updates GPT table
On 2010/10/05 15:28:02, Will Drewry wrote:
> nit: and update the

Done.

http://codereview.chromium.org/3391018/diff/21001/22003#newcode34
resize_stateful_partition.sh:34: # Due to this resize, we need to create a new
partition table and a new image.
On 2010/10/05 15:28:02, Will Drewry wrote:
> I'm inclined to suggest that you rename this function
"enlarge_partition_image"
> or something similar because it enlarges the file in place and not a partition
> in an actual disk image or drive.

Done.

http://codereview.chromium.org/3391018/diff/21001/22003#newcode49
resize_stateful_partition.sh:49: dd if=/dev/zero of="${source_part}" bs=1
count=1 \
good to know!

http://codereview.chromium.org/3391018/diff/21001/22003#newcode81
resize_stateful_partition.sh:81: "${temp_pmbr}" "${esp_count}" false $(roundup
${rootfs_count}) &>/dev/null
silencing stdout/stderr to make sure caller of this function gets the proper
*return* value from the "echo" statement at the end of this function :-)
otherwise the value returned would be cluttered w/ stdout/stderr info

http://codereview.chromium.org/3391018/diff/21001/22003#newcode83
resize_stateful_partition.sh:83: rm -rf "${temp_pmbr}"
moved this line to a trap before install_gpt

http://codereview.chromium.org/3391018/diff/21001/22003#newcode88
resize_stateful_partition.sh:88: &>/dev/null
keeping it as-is for echo reason above

[Tan Gao, 2010-10-06 21:37:39]

Hi Will,

While testing Set 9 of this CL, I ran into a few issues which prompted me to
revert some changes in response to your Set 8 feedback, e.g. adding "-b 4096"
flag to mkfs.ext3 causes write error for directory copy due to lack of space.
Set 10 is tested with both non-developer and developer recovery and they both
work :-)

PTAL thx!
Tan

[Will Drewry, 2010-10-07 22:51:09]

I'd like the comment updated with the actual problem, but otherwise this looks
better :)

thanks

http://codereview.chromium.org/3391018/diff/44001/45002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/44001/45002#newcode103
mod_image_for_recovery.sh:103: # Do NOT use 4Kb block size as it breaks
directory copy below
Was this the actual problem or was it some other change?  I'd like to understand
why a 4k block size breaks things (or is the partition too small to use 4k
chunks for the amount of data you're putting on it?)

If there is a specific reason, I'd prefer it in the comment rather than that it
breaks a copy :)

[Tan Gao, 2010-10-07 23:47:35]

http://codereview.chromium.org/3391018/diff/44001/45002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/44001/45002#newcode103
mod_image_for_recovery.sh:103: # Do NOT use 4Kb block size as it breaks
directory copy below
hmm .. I don't know what the root cause of the breakage is. What I've seen is
that with "-b 4096" option added, directory copy (specifically "tar xfp") fails
with error messages indicating no space left on target device.

Should I include the above observation in the comment? Or how would one proceed
with investigating this issue?

[Will Drewry, 2010-10-08 02:01:13]

I can help with this tomorrow if that's useful - it shouldn't take long.  I'd
just like the proper note in there so that it's clear why the decision was made!

http://codereview.chromium.org/3391018/diff/44001/45002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/44001/45002#newcode103
mod_image_for_recovery.sh:103: # Do NOT use 4Kb block size as it breaks
directory copy below
On 2010/10/07 23:47:35, Tan Gao wrote:
> hmm .. I don't know what the root cause of the breakage is. What I've seen is
> that with "-b 4096" option added, directory copy (specifically "tar xfp")
fails
> with error messages indicating no space left on target device.
> 
> Should I include the above observation in the comment? Or how would one
proceed
> with investigating this issue?

Hrm sounds like too many files smaller than 4k with the smaller size :)  How are
you getting the new size? (I need to go back through the scripts again to see
myself :)

To cross check, just create the sized fs with dd and mkfs then mount it -o loop
and see if you can do the tar manaully or just have a script touch as many files
as you expect to be copying.

[kliegs1, 2010-10-08 05:33:58]

Random guess of something to try. Run 'df -i' on the volume in question.
Will show you unidentified counts.

-jon

On Oct 7, 2010 10:01 PM, <wad@chromium.org> wrote:
> I can help with this tomorrow if that's useful - it shouldn't take long.
> I'd
> just like the proper note in there so that it's clear why the decision was

> made!
>
>
> http://codereview.chromium.org/3391018/diff/44001/45002
> File mod_image_for_recovery.sh (right):
>
> http://codereview.chromium.org/3391018/diff/44001/45002#newcode103
> mod_image_for_recovery.sh:103: # Do NOT use 4Kb block size as it breaks
> directory copy below
> On 2010/10/07 23:47:35, Tan Gao wrote:
>> hmm .. I don't know what the root cause of the breakage is. What I've
> seen is
>> that with "-b 4096" option added, directory copy (specifically "tar
> xfp") fails
>> with error messages indicating no space left on target device.
>
>> Should I include the above observation in the comment? Or how would
> one proceed
>> with investigating this issue?
>
> Hrm sounds like too many files smaller than 4k with the smaller size :)
> How are you getting the new size? (I need to go back through the scripts
> again to see myself :)
>
> To cross check, just create the sized fs with dd and mkfs then mount it
> -o loop and see if you can do the tar manaully or just have a script
> touch as many files as you expect to be copying.
>
> http://codereview.chromium.org/3391018/show

[Will Drewry, 2010-10-08 16:34:53]

Based on our chat, it sounds like you're running out of inodes because the size
is determined using the sectors used and not the filesystem block allocation.

It's fine to add that to the comment, but it's possible we'll hit issues later. 
Using resize2fs would be a bit cleaner because it would do it based on
filesystem allocations and not pure data use.

http://codereview.chromium.org/3391018/diff/44001/45002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/44001/45002#newcode74
mod_image_for_recovery.sh:74: sudo mount -o loop=${src_loop_dev} "${src_state}"
"${src_mnt}"
I didn't notice this before but after we discussed it, I realized why things are
a bit weird.  Later, if we need to handle properly padding in 20% without making
it weird by changing inode/block sizes, I would've suggested:

# Setup loop dev
losetup --offset $((stateful_offset * 512)) --sizelimit $((stateful_count * 512)
$src_loop_dev
block_size=$((dumpe2fs $src_loop_dev | grep "Block size:" | tr -d ' ' | cut -f2
-d:)
min_size=$(resize2fs -P $src_loop_dev)
# Add 20%
min_size=$(((min_size * 120) / 100))
resize2fs $src_loop_dev $min_size  # size is already in fs block size!
# Delete the loop
...
# Truncate the image at the new size
blocksz=$(dumpe2fs $src_state | grep 'Block count' | tr -d ' ' | cut -f2 -d:)
dd if=/dev/zero of=$src_state bs=$blocksz seek=$min_size count=0 
# Mount and touch .recovery  # Soon not to be needed :/
...
# Update partition table
...

[Tan Gao, 2010-10-08 22:01:13]

hew .. hope it's ready to go this time :-)

http://codereview.chromium.org/3391018/diff/44001/45002
File mod_image_for_recovery.sh (right):

http://codereview.chromium.org/3391018/diff/44001/45002#newcode74
mod_image_for_recovery.sh:74: sudo mount -o loop=${src_loop_dev} "${src_state}"
"${src_mnt}"
done. thx for working w/ me over chat .. this patch is ready for review :-)

[Will Drewry, 2010-10-08 22:19:21]

cool LGTM