|
|
Created:
10 years, 2 months ago by petkov Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, adlr Visibility:
Public. |
DescriptionAU: 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
Patch Set 1 #
Total comments: 9
Messages
Total messages: 9 (0 generated)
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.
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.
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!
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!
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 :)
Will, so does this LGTY?
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.
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. |