AU: Use full rootfs partition for scratch.

delta_diff_generator.cc

Index: delta_diff_generator.cc
diff --git a/delta_diff_generator.cc b/delta_diff_generator.cc
index 679f095dbe695d55d302579fe7a2beeab46c5325..b38b495a51f5ecf0f77973b3577a5f0bad6128b2 100644
--- a/delta_diff_generator.cc
+++ b/delta_diff_generator.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -40,6 +40,7 @@ typedef DeltaDiffGenerator::Block Block;
 
 namespace {
 const size_t kBlockSize = 4096;
+const size_t kRootFSPartitionSize = 1 * 1024 * 1024 * 1024;  // 1 GiB

[Will Drewry, 2010/09/29 01:10:47]

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. :/

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

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?

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

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 :)

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

$ 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?

[Will Drewry, 2010/09/29 19:07:36]

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

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

crosbug/7181 filed.

 const uint64_t kVersionNumber = 1;
 
 // Stores all Extents for a file into 'out'. Returns true on success.
@@ -209,32 +210,36 @@ bool DeltaReadFiles(Graph* graph,
   return true;
 }
 
-// Attempts to find block_count blocks to use as scratch space.
-// Returns true on success.
-// Right now we return exactly as many blocks as are required.
-// TODO(adlr): consider returning all scratch blocks,
-// even if there are extras, to make it easier for a scratch allocator
-// to find contiguous regions for specific scratch writes.
+// Attempts to find |block_count| blocks to use as scratch space. Returns true
+// on success. Right now we return exactly as many blocks as are required.
+//
+// TODO(adlr): Consider returning all scratch blocks, even if there are extras,
+// to make it easier for a scratch allocator to find contiguous regions for
+// specific scratch writes.
 bool FindScratchSpace(const vector<Block>& blocks,
                       vector<Block>::size_type block_count,
                       vector<Extent>* out) {
-  // Scan blocks for blocks that are neither read nor written.
-  // If we don't find enough of those, return false.
-  // TODO(adlr): return blocks that are written by
-  // operations that don't have incoming edges (and thus, can be
-  // deferred until all old blocks are read by other operations).
+  // Scan |blocks| for blocks that are neither read, nor written. If we don't
+  // find enough of those, look past the end of |blocks| till the end of the
+  // partition. If we don't find |block_count| scratch blocks, return false.
+  //
+  // TODO(adlr): Return blocks that are written by operations that don't have
+  // incoming edges (and thus, can be deferred until all old blocks are read by
+  // other operations).
   vector<Extent> ret;
   vector<Block>::size_type blocks_found = 0;
+  const size_t kPartitionBlocks = kRootFSPartitionSize / kBlockSize;
   for (vector<Block>::size_type i = 0;
-       i < blocks.size() && blocks_found < block_count; i++) {
-    if (blocks[i].reader == Vertex::kInvalidIndex &&
-        blocks[i].writer == Vertex::kInvalidIndex) {
+       i < kPartitionBlocks && blocks_found < block_count; i++) {
+    if (i >= blocks.size() ||
+        (blocks[i].reader == Vertex::kInvalidIndex &&
+         blocks[i].writer == Vertex::kInvalidIndex)) {
       graph_utils::AppendBlockToExtents(&ret, i);
       blocks_found++;
     }
   }
+  LOG(INFO) << "found " << blocks_found << " scratch blocks";
   if (blocks_found == block_count) {
-    LOG(INFO) << "returning " << blocks_found << " scratch blocks";
     out->swap(ret);
     return true;
   }
@@ -874,7 +879,8 @@ bool DeltaDiffGenerator::GenerateDeltaUpdateFile(
         const Extent& extent = op->dst_extents(j);
         for (uint64_t block = extent.start_block();
              block < (extent.start_block() + extent.num_blocks()); block++) {
-          written_count[block]++;
+          if (block < blocks.size())

[Will Drewry, 2010/09/29 01:10:47]

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]

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]

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

+            written_count[block]++;
         }
       }
       if (op->has_data_offset()) {