AU: Use full rootfs partition for scratch.

AU: Use full rootfs partition for scratch.

This patch allows the delta generator to use blocks beyond rootfs up to
the full rootfs partition as scratch. This is really a stop gap solution.
The updater needs to be fixed to work with as little as one block of
scratch if necessary.

BUG=6531
TEST=unit tests, generated a delta payload and updated from 70.8 to 72.3

Change-Id: I52b7d04d5a5345c34c9c647f9878c836be9f489c

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c0b7a53

[Will Drewry, 2010-09-29 01:10:43]

Some questions/comments for my edification, but right now the fact the test
passes is a good start ;)

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number? (Or
fstat if the ioctl fails)?

This is fine, but I hate to see it hardcoded in one more place.  If you dont see
an easy way out for now, please file a bug to clean this up. :/

http://codereview.chromium.org/3562001/diff/1/2#newcode882
delta_diff_generator.cc:882: if (block < blocks.size())
Does this just catch if scratch blocks are written? It seems weird that the
scratch blocks would turn up as written extents, but I don't really understand
all the details here.

[petkov, 2010-09-29 04:27:40]

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
On 2010/09/29 01:10:47, Will Drewry wrote:
> Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number? (Or
> fstat if the ioctl fails)?

I don't think I can do this -- this is running on the server on a .bin image
file. I.e., I don't think the actual partition size is available. Right?

> 
> This is fine, but I hate to see it hardcoded in one more place.  If you dont
see
> an easy way out for now, please file a bug to clean this up. :/

http://codereview.chromium.org/3562001/diff/1/2#newcode882
delta_diff_generator.cc:882: if (block < blocks.size())
On 2010/09/29 01:10:47, Will Drewry wrote:
> Does this just catch if scratch blocks are written? It seems weird that the
> scratch blocks would turn up as written extents, but I don't really understand
> all the details here.

My understanding is that this code makes sure that every block in the target
rootfs will get written at least once. This happens to include scratch blocks.
Since this patch may allocate scratch blocks beyond the actual rootfs, we need
to ignore them in the check.

In other words, I think this check needs to be improved to ignore writes to
scratch blocks. But this is not part of this CL.

[Will Drewry, 2010-09-29 04:33:41]

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
On 2010/09/29 04:27:40, petkov wrote:
> On 2010/09/29 01:10:47, Will Drewry wrote:
> > Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number? (Or
> > fstat if the ioctl fails)?
> 
> I don't think I can do this -- this is running on the server on a .bin image
> file. I.e., I don't think the actual partition size is available. Right?

Hrm the size is definitely available in the gpt table of the .bin and if a loop
device is provided, it may also be configured with the size. 

Is it safe to assume there's no cgpt linkage in there already?

Also, is this a server-side decision or a decision that happens during
install/apply, or both?  If it is the former, we could just add a cmdline arg
for the rootfs size :)

http://codereview.chromium.org/3562001/diff/1/2#newcode882
delta_diff_generator.cc:882: if (block < blocks.size())
On 2010/09/29 04:27:40, petkov wrote:
> On 2010/09/29 01:10:47, Will Drewry wrote:
> > Does this just catch if scratch blocks are written? It seems weird that the
> > scratch blocks would turn up as written extents, but I don't really
understand
> > all the details here.
> 
> My understanding is that this code makes sure that every block in the target
> rootfs will get written at least once. This happens to include scratch blocks.
> Since this patch may allocate scratch blocks beyond the actual rootfs, we need
> to ignore them in the check.
> 
> In other words, I think this check needs to be improved to ignore writes to
> scratch blocks. But this is not part of this CL.
> 

Fair enough.  We can go through that with adlr@ when he is back!

[petkov, 2010-09-29 04:37:59]

On 2010/09/29 04:33:41, Will Drewry wrote:
> http://codereview.chromium.org/3562001/diff/1/2
> File delta_diff_generator.cc (right):
> 
> http://codereview.chromium.org/3562001/diff/1/2#newcode43
> delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 *
1024
> * 1024;  // 1 GiB
> On 2010/09/29 04:27:40, petkov wrote:
> > On 2010/09/29 01:10:47, Will Drewry wrote:
> > > Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number?
(Or
> > > fstat if the ioctl fails)?
> > 
> > I don't think I can do this -- this is running on the server on a .bin image
> > file. I.e., I don't think the actual partition size is available. Right?
> 
> Hrm the size is definitely available in the gpt table of the .bin and if a
loop
> device is provided, it may also be configured with the size. 
> 
> Is it safe to assume there's no cgpt linkage in there already?

I'll check tomorrow if there's a way to get the partition size through cgpt. I
assumed I couldn't -- it seemed as if cgpt gave the actual rootfs size, not the
partition size. I'll double check.

> 
> Also, is this a server-side decision or a decision that happens during
> install/apply, or both?  If it is the former, we could just add a cmdline arg
> for the rootfs size :)

It's server-side only decision, no checks on the client side (I think). I also
thought about making this unlimited and issuing a warning message at the end
about the minimum partition size required.

I'll check tomorrow what's available in the .bin through cgpt.

> 
> http://codereview.chromium.org/3562001/diff/1/2#newcode882
> delta_diff_generator.cc:882: if (block < blocks.size())
> On 2010/09/29 04:27:40, petkov wrote:
> > On 2010/09/29 01:10:47, Will Drewry wrote:
> > > Does this just catch if scratch blocks are written? It seems weird that
the
> > > scratch blocks would turn up as written extents, but I don't really
> understand
> > > all the details here.
> > 
> > My understanding is that this code makes sure that every block in the target
> > rootfs will get written at least once. This happens to include scratch
blocks.
> > Since this patch may allocate scratch blocks beyond the actual rootfs, we
need
> > to ignore them in the check.
> > 
> > In other words, I think this check needs to be improved to ignore writes to
> > scratch blocks. But this is not part of this CL.
> > 
> 
> Fair enough.  We can go through that with adlr@ when he is back!

[petkov, 2010-09-29 16:17:36]

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
On 2010/09/29 04:33:41, Will Drewry wrote:
> On 2010/09/29 04:27:40, petkov wrote:
> > On 2010/09/29 01:10:47, Will Drewry wrote:
> > > Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number?
(Or
> > > fstat if the ioctl fails)?
> > 
> > I don't think I can do this -- this is running on the server on a .bin image
> > file. I.e., I don't think the actual partition size is available. Right?
> 
> Hrm the size is definitely available in the gpt table of the .bin and if a
loop
> device is provided, it may also be configured with the size. 

$ cgpt show chromiumos_base_image.bin | fgrep ROOT-A
     36864   1490944       3  Label: "ROOT-A"

1490944 blocks is 763363328 bytes, which is the actual rootfs size, rather than
the rootfs partition size on the device.

On the actual device:

# cgpt show /dev/sda | fgrep ROOT-A 
  13549568   2097152       3  Label: "ROOT-A"

Is there any other place I can get the partition size from on the server?

> 
> Is it safe to assume there's no cgpt linkage in there already?
> 
> Also, is this a server-side decision or a decision that happens during
> install/apply, or both?  If it is the former, we could just add a cmdline arg
> for the rootfs size :)

[petkov, 2010-09-29 18:22:58]

Will, so does this LGTY?

[Will Drewry, 2010-09-29 19:07:35]

LGTM

Please drop in a bug about the hard-coded root partition size.  We need to
codify that somewhere where C++ and shell scripts can use it cleanly. (Like
cros_constants or something...)

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
On 2010/09/29 16:17:36, petkov wrote:
> On 2010/09/29 04:33:41, Will Drewry wrote:
> > On 2010/09/29 04:27:40, petkov wrote:
> > > On 2010/09/29 01:10:47, Will Drewry wrote:
> > > > Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that number?
> (Or
> > > > fstat if the ioctl fails)?
> > > 
> > > I don't think I can do this -- this is running on the server on a .bin
image
> > > file. I.e., I don't think the actual partition size is available. Right?
> > 
> > Hrm the size is definitely available in the gpt table of the .bin and if a
> loop
> > device is provided, it may also be configured with the size. 
> 
> $ cgpt show chromiumos_base_image.bin | fgrep ROOT-A
>      36864   1490944       3  Label: "ROOT-A"
> 
> 1490944 blocks is 763363328 bytes, which is the actual rootfs size, rather
than
> the rootfs partition size on the device.
> 
> On the actual device:
> 
> # cgpt show /dev/sda | fgrep ROOT-A 
>   13549568   2097152       3  Label: "ROOT-A"
> 
> Is there any other place I can get the partition size from on the server?
> 
> > 
> > Is it safe to assume there's no cgpt linkage in there already?
> > 
> > Also, is this a server-side decision or a decision that happens during
> > install/apply, or both?  If it is the former, we could just add a cmdline
arg
> > for the rootfs size :)
> 
> 

It being hard-coded in the installer means we get forced into hard coding it
here. Ugh.

[petkov, 2010-09-29 19:44:35]

http://codereview.chromium.org/3562001/diff/1/2
File delta_diff_generator.cc (right):

http://codereview.chromium.org/3562001/diff/1/2#newcode43
delta_diff_generator.cc:43: const size_t kRootFSPartitionSize = 1 * 1024 * 1024
* 1024;  // 1 GiB
On 2010/09/29 19:07:36, Will Drewry wrote:
> On 2010/09/29 16:17:36, petkov wrote:
> > On 2010/09/29 04:33:41, Will Drewry wrote:
> > > On 2010/09/29 04:27:40, petkov wrote:
> > > > On 2010/09/29 01:10:47, Will Drewry wrote:
> > > > > Any chance you could open(tgt), ioctl(BLKGETSIZE64) and use that
number?
> > (Or
> > > > > fstat if the ioctl fails)?
> > > > 
> > > > I don't think I can do this -- this is running on the server on a .bin
> image
> > > > file. I.e., I don't think the actual partition size is available. Right?
> > > 
> > > Hrm the size is definitely available in the gpt table of the .bin and if a
> > loop
> > > device is provided, it may also be configured with the size. 
> > 
> > $ cgpt show chromiumos_base_image.bin | fgrep ROOT-A
> >      36864   1490944       3  Label: "ROOT-A"
> > 
> > 1490944 blocks is 763363328 bytes, which is the actual rootfs size, rather
> than
> > the rootfs partition size on the device.
> > 
> > On the actual device:
> > 
> > # cgpt show /dev/sda | fgrep ROOT-A 
> >   13549568   2097152       3  Label: "ROOT-A"
> > 
> > Is there any other place I can get the partition size from on the server?
> > 
> > > 
> > > Is it safe to assume there's no cgpt linkage in there already?
> > > 
> > > Also, is this a server-side decision or a decision that happens during
> > > install/apply, or both?  If it is the former, we could just add a cmdline
> arg
> > > for the rootfs size :)
> > 
> > 
> 
> It being hard-coded in the installer means we get forced into hard coding it
> here. Ugh.

crosbug/7181 filed.