Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5682006:
Add support for preserving on-disk file layout (Closed)
DescriptionAdd support for preserving on-disk file layout
BUG=chromium-os:10188
TEST=Build image with and without new flags, verify by booting image,
auto-update and boot image
Change-Id: I5ddb1becc4f1dc7a8620b2f317e08850df65f9b3
Patch Set 1 #
Total comments: 19
Messages
Total messages: 13 (0 generated)
This CL preserves the on-disk file layout between two builds. This minimizes the delta payload and speed up the application of the delta because we do not need to shuffle files around the disk.
On 2010/12/10 00:52:55, thieule wrote: > This CL preserves the on-disk file layout between two builds. This minimizes > the delta payload and speed up the application of the delta because we do not > need to shuffle files around the disk. The one change I recommend is to pull the new logic out of build_image into it's own script (align_image_fs or something) that build_image calls. build_image is already too large, and being separate will make testing, etc easier.
+robotboy who is working on a "imager" that does all of the dd/fs operations. On Thu, Dec 9, 2010 at 5:13 PM, <dgarrett@chromium.org> wrote: > On 2010/12/10 00:52:55, thieule wrote: >> >> This CL preserves the on-disk file layout between two builds. This >> minimizes >> the delta payload and speed up the application of the delta because we do >> not >> need to shuffle files around the disk. > > The one change I recommend is to pull the new logic out of build_image into > it's > own script (align_image_fs or something) that build_image calls. > > build_image is already too large, and being separate will make testing, etc > easier. > > http://codereview.chromium.org/5682006/ >
Is it OK to split this into two separate patches -- one that zeroes out the free space and one for alignment? They seem independent and it doesn't seem that there's a big downside to always zeroing out the freespace -- in some sense, we don't even need yet another option for that. Also, in terms of alignment -- it would have been good if we could come up with something like a canonical order for the image contents so that we can avoid having a base image. It might be tricky to use an implementation that requires a base image in the build flow, and requiring a base image might make it less useful in the context of releasing several deltas from different source versions. http://codereview.chromium.org/5682006/diff/1/build_image File build_image (right): http://codereview.chromium.org/5682006/diff/1/build_image#newcode89 build_image:89: DEFINE_boolean zero_freespace ${FLAGS_FALSE} \ any downside in defaulting this to TRUE? http://codereview.chromium.org/5682006/diff/1/build_image#newcode137 build_image:137: if [ ! -z "${FLAGS_base_image}" ] && [ ! -f "${FLAGS_base_image}" ] ; then if [ -n "${FLAGS_base_image}" ] ...
In regard to zeroing out the free space -- it will benefit both full and delta updates because the free space will compress better. So, we should do it regardless of the presence of a base image.
I'll split this CL into a zero free space and an alignment. As for using some canonical order instead of a base image, there are two problems with this approach. If we insert a new file in the middle of the order, it shifts the rest of the files down. If a file grows, it also shifts the rest of the files down. This results in unnecessary reshuffling of the disk blocks on the device. Another alternative is to use a template image instead of a base image. Take an existing shipping image and zero out the content of all the files so we can compress this image. We can then check in this smaller image and use it as our base image. The drawback with this approach is that we will need to occasionally update this template as we add more files to the image. On Fri, Dec 10, 2010 at 10:36 AM, <petkov@chromium.org> wrote: > In regard to zeroing out the free space -- it will benefit both full and > delta > updates because the free space will compress better. So, we should do it > regardless of the presence of a base image. > > > > http://codereview.chromium.org/5682006/ >
http://codereview.chromium.org/5682006/diff/1/build_image File build_image (right): http://codereview.chromium.org/5682006/diff/1/build_image#newcode137 build_image:137: if [ ! -z "${FLAGS_base_image}" ] && [ ! -f "${FLAGS_base_image}" ] ; then -n instead of ! -z http://codereview.chromium.org/5682006/diff/1/build_image#newcode239 build_image:239: NEW_ROOT_FS_DIR=/tmp/mnew Create a temp dir ... why use the same one everytime? http://codereview.chromium.org/5682006/diff/1/build_image#newcode521 build_image:521: # Disable die on error so dd can write until EOF Make this more function-y .. i.e. use $1 $2 etc http://codereview.chromium.org/5682006/diff/1/build_image#newcode522 build_image:522: set +e Don't set +e / -e ... use || instead on the dd line http://codereview.chromium.org/5682006/diff/1/build_image#newcode548 build_image:548: echo "Aligning rootfs on-disk file layout to base image" info http://codereview.chromium.org/5682006/diff/1/build_image#newcode555 build_image:555: enable_rw_mount "${NEW_ROOT_FS}" Why is this necessary? Can't rw just be set in the mount options? http://codereview.chromium.org/5682006/diff/1/build_image#newcode558 build_image:558: NEW_ROOT_FS_LOOP_DEV=$(sudo losetup -f) Not sure you even have to do this rather than mount use mount -o loop. There are some other cases where it is necessary but not here http://codereview.chromium.org/5682006/diff/1/build_image#newcode571 build_image:571: immutable_files=() local? http://codereview.chromium.org/5682006/diff/1/build_image#newcode572 build_image:572: NEW_ROOT_FS_DIR_ESCAPED=$(echo "${NEW_ROOT_FS_DIR}" | sed 's/\//\\\//g') Why global? http://codereview.chromium.org/5682006/diff/1/build_image#newcode572 build_image:572: NEW_ROOT_FS_DIR_ESCAPED=$(echo "${NEW_ROOT_FS_DIR}" | sed 's/\//\\\//g') Use a diff seperater in your sed than / to make this easier to read http://codereview.chromium.org/5682006/diff/1/build_image#newcode573 build_image:573: local enum_files_cmd='sudo find "${NEW_ROOT_FS_DIR}" -xdev -type f' Why exclude directories and such? http://codereview.chromium.org/5682006/diff/1/build_image#newcode574 build_image:574: eval $enum_files_cmd | sed "s/${NEW_ROOT_FS_DIR_ESCAPED}//" | \ {} http://codereview.chromium.org/5682006/diff/1/build_image#newcode576 build_image:576: immutable=$(sudo lsattr "${NEW_ROOT_FS_DIR}$FILE" | cut -d' ' -f1 | \ shouldn't there be spaces in the cut http://codereview.chromium.org/5682006/diff/1/build_image#newcode578 build_image:578: if [ $immutable -eq 0 ] ; then {} http://codereview.chromium.org/5682006/diff/1/build_image#newcode578 build_image:578: if [ $immutable -eq 0 ] ; then No spaces before ; same elsewhere http://codereview.chromium.org/5682006/diff/1/build_image#newcode580 build_image:580: sudo chattr -i "${NEW_ROOT_FS_DIR}$FILE" Little confused ... you set this immmutable twice ... here and below ... why? http://codereview.chromium.org/5682006/diff/1/build_image#newcode585 build_image:585: # as possible. Layout? maybe metadata is more accurate
Speaking at a higher level, I'm not quite sure how we would use the base image in the release process. We have some requirements including that after delta applications two images are exactly the same which would be non-trivial to get with the base image. Thieu, can you wrote up a short design doc discussing this before we get the code in? -Sosa On Fri, Dec 10, 2010 at 11:28 AM, <sosa@chromium.org> wrote: > > http://codereview.chromium.org/5682006/diff/1/build_image > File build_image (right): > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode137 > build_image:137: if [ ! -z "${FLAGS_base_image}" ] && [ ! -f > "${FLAGS_base_image}" ] ; then > -n instead of ! -z > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode239 > build_image:239: NEW_ROOT_FS_DIR=/tmp/mnew > Create a temp dir ... why use the same one everytime? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode521 > build_image:521: # Disable die on error so dd can write until EOF > Make this more function-y .. i.e. use $1 $2 etc > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode522 > build_image:522: set +e > Don't set +e / -e ... use || instead on the dd line > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode548 > build_image:548: echo "Aligning rootfs on-disk file layout to base > image" > info > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode555 > build_image:555: enable_rw_mount "${NEW_ROOT_FS}" > Why is this necessary? Can't rw just be set in the mount options? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode558 > build_image:558: NEW_ROOT_FS_LOOP_DEV=$(sudo losetup -f) > Not sure you even have to do this rather than mount use mount -o loop. > There are some other cases where it is necessary but not here > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode571 > build_image:571: immutable_files=() > local? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode572 > build_image:572: NEW_ROOT_FS_DIR_ESCAPED=$(echo "${NEW_ROOT_FS_DIR}" | > sed 's/\//\\\//g') > Why global? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode572 > build_image:572: NEW_ROOT_FS_DIR_ESCAPED=$(echo "${NEW_ROOT_FS_DIR}" | > sed 's/\//\\\//g') > Use a diff seperater in your sed than / to make this easier to read > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode573 > build_image:573: local enum_files_cmd='sudo find "${NEW_ROOT_FS_DIR}" > -xdev -type f' > Why exclude directories and such? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode574 > build_image:574: eval $enum_files_cmd | sed > "s/${NEW_ROOT_FS_DIR_ESCAPED}//" | \ > {} > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode576 > build_image:576: immutable=$(sudo lsattr "${NEW_ROOT_FS_DIR}$FILE" | cut > -d' ' -f1 | \ > shouldn't there be spaces in the cut > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode578 > build_image:578: if [ $immutable -eq 0 ] ; then > {} > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode578 > build_image:578: if [ $immutable -eq 0 ] ; then > No spaces before ; same elsewhere > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode580 > build_image:580: sudo chattr -i "${NEW_ROOT_FS_DIR}$FILE" > Little confused ... you set this immmutable twice ... here and below ... > why? > > http://codereview.chromium.org/5682006/diff/1/build_image#newcode585 > build_image:585: # as possible. > Layout? maybe metadata is more accurate > > http://codereview.chromium.org/5682006/ >
On 2010/12/10 19:08:19, thieule wrote: > I'll split this CL into a zero free space and an alignment. > > As for using some canonical order instead of a base image, there are two > problems with this approach. If we insert a new file in the middle of the > order, it shifts the rest of the files down. If a file grows, it also > shifts the rest of the files down. This results in unnecessary reshuffling > of the disk blocks on the device. > > Another alternative is to use a template image instead of a base image. > Take an existing shipping image and zero out the content of all the files > so we can compress this image. We can then check in this smaller image and > use it as our base image. The drawback with this approach is that we will > need to occasionally update this template as we add more files to the image. Agreed that it's tricky. Another heuristic-based alternative is to split the image contents into a part that changes often (e.g, files coming from all chromiumos-overlay packages) and a part that doesn't change that often (e.g., files coming from the rest of the packages). Then allocate the two into two separate parts of the image. I'm sure one can come up with more clever ways to do this... > > On Fri, Dec 10, 2010 at 10:36 AM, <mailto:petkov@chromium.org> wrote: > > > In regard to zeroing out the free space -- it will benefit both full and > > delta > > updates because the free space will compress better. So, we should do it > > regardless of the presence of a base image. > > > > > > > > http://codereview.chromium.org/5682006/ > >
I'll go ponder this some more and put together a doc that describes this further. On Fri, Dec 10, 2010 at 12:44 PM, <petkov@chromium.org> wrote: > On 2010/12/10 19:08:19, thieule wrote: > >> I'll split this CL into a zero free space and an alignment. >> > > As for using some canonical order instead of a base image, there are two >> problems with this approach. If we insert a new file in the middle of the >> order, it shifts the rest of the files down. If a file grows, it also >> shifts the rest of the files down. This results in unnecessary >> reshuffling >> of the disk blocks on the device. >> > > Another alternative is to use a template image instead of a base image. >> Take an existing shipping image and zero out the content of all the files >> so we can compress this image. We can then check in this smaller image >> and >> use it as our base image. The drawback with this approach is that we will >> need to occasionally update this template as we add more files to the >> image. >> > > Agreed that it's tricky. Another heuristic-based alternative is to split > the > image contents into a part that changes often (e.g, files coming from all > chromiumos-overlay packages) and a part that doesn't change that often > (e.g., > files coming from the rest of the packages). Then allocate the two into two > separate parts of the image. I'm sure one can come up with more clever ways > to > do this... > > > > On Fri, Dec 10, 2010 at 10:36 AM, <mailto:petkov@chromium.org> wrote: >> > > > In regard to zeroing out the free space -- it will benefit both full and >> > delta >> > updates because the free space will compress better. So, we should do it >> > regardless of the presence of a base image. >> > >> > >> > >> > http://codereview.chromium.org/5682006/ >> > >> > > > > http://codereview.chromium.org/5682006/ >
On 2010/12/10 21:08:57, thieule wrote: > I'll go ponder this some more and put together a doc that describes this > further. Sounds great! It still makes sense to split out the zero-free-space change and review/push it separately. > > On Fri, Dec 10, 2010 at 12:44 PM, <mailto:petkov@chromium.org> wrote: > > > On 2010/12/10 19:08:19, thieule wrote: > > > >> I'll split this CL into a zero free space and an alignment. > >> > > > > As for using some canonical order instead of a base image, there are two > >> problems with this approach. If we insert a new file in the middle of the > >> order, it shifts the rest of the files down. If a file grows, it also > >> shifts the rest of the files down. This results in unnecessary > >> reshuffling > >> of the disk blocks on the device. > >> > > > > Another alternative is to use a template image instead of a base image. > >> Take an existing shipping image and zero out the content of all the files > >> so we can compress this image. We can then check in this smaller image > >> and > >> use it as our base image. The drawback with this approach is that we will > >> need to occasionally update this template as we add more files to the > >> image. > >> > > > > Agreed that it's tricky. Another heuristic-based alternative is to split > > the > > image contents into a part that changes often (e.g, files coming from all > > chromiumos-overlay packages) and a part that doesn't change that often > > (e.g., > > files coming from the rest of the packages). Then allocate the two into two > > separate parts of the image. I'm sure one can come up with more clever ways > > to > > do this... > > > > > > > > On Fri, Dec 10, 2010 at 10:36 AM, <mailto:petkov@chromium.org> wrote: > >> > > > > > In regard to zeroing out the free space -- it will benefit both full and > >> > delta > >> > updates because the free space will compress better. So, we should do it > >> > regardless of the presence of a base image. > >> > > >> > > >> > > >> > http://codereview.chromium.org/5682006/ > >> > > >> > > > > > > > > http://codereview.chromium.org/5682006/ > >
I have captured my current thinking along with ideas from this thread in this document: https://docs.google.com/a/google.com/document/d/1Tbb1I-_yT1RMx4s8r_-4YZVLfd7q... Feel free to add more options to the list. On Fri, Dec 10, 2010 at 1:10 PM, <petkov@chromium.org> wrote: > On 2010/12/10 21:08:57, thieule wrote: > >> I'll go ponder this some more and put together a doc that describes this >> further. >> > > Sounds great! It still makes sense to split out the zero-free-space change > and > review/push it separately. > > > > On Fri, Dec 10, 2010 at 12:44 PM, <mailto:petkov@chromium.org> wrote: >> > > > On 2010/12/10 19:08:19, thieule wrote: >> > >> >> I'll split this CL into a zero free space and an alignment. >> >> >> > >> > As for using some canonical order instead of a base image, there are >> two >> >> problems with this approach. If we insert a new file in the middle of >> the >> >> order, it shifts the rest of the files down. If a file grows, it also >> >> shifts the rest of the files down. This results in unnecessary >> >> reshuffling >> >> of the disk blocks on the device. >> >> >> > >> > Another alternative is to use a template image instead of a base image. >> >> Take an existing shipping image and zero out the content of all the >> files >> >> so we can compress this image. We can then check in this smaller image >> >> and >> >> use it as our base image. The drawback with this approach is that we >> will >> >> need to occasionally update this template as we add more files to the >> >> image. >> >> >> > >> > Agreed that it's tricky. Another heuristic-based alternative is to split >> > the >> > image contents into a part that changes often (e.g, files coming from >> all >> > chromiumos-overlay packages) and a part that doesn't change that often >> > (e.g., >> > files coming from the rest of the packages). Then allocate the two into >> two >> > separate parts of the image. I'm sure one can come up with more clever >> ways >> > to >> > do this... >> > >> > >> > >> > On Fri, Dec 10, 2010 at 10:36 AM, <mailto:petkov@chromium.org> wrote: >> >> >> > >> > > In regard to zeroing out the free space -- it will benefit both full >> and >> >> > delta >> >> > updates because the free space will compress better. So, we should do >> it >> >> > regardless of the presence of a base image. >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/5682006/ >> >> > >> >> >> > >> > >> > >> > http://codereview.chromium.org/5682006/ >> > >> > > > > http://codereview.chromium.org/5682006/ >
On 2010/12/14 02:05:53, thieule wrote: > I have captured my current thinking along with ideas from this thread in > this document: > > https://docs.google.com/a/google.com/document/d/1Tbb1I-_yT1RMx4s8r_-4YZVLfd7q... > > Feel free to add more options to the list. I think the doc should capture plans for updating the official release process -- maybe discuss with djmm, scottz, raja -- especially in the context of needing a base image for generating a new image. > > > On Fri, Dec 10, 2010 at 1:10 PM, <mailto:petkov@chromium.org> wrote: > > > On 2010/12/10 21:08:57, thieule wrote: > > > >> I'll go ponder this some more and put together a doc that describes this > >> further. > >> > > > > Sounds great! It still makes sense to split out the zero-free-space change > > and > > review/push it separately. > > > > > > > > On Fri, Dec 10, 2010 at 12:44 PM, <mailto:petkov@chromium.org> wrote: > >> > > > > > On 2010/12/10 19:08:19, thieule wrote: > >> > > >> >> I'll split this CL into a zero free space and an alignment. > >> >> > >> > > >> > As for using some canonical order instead of a base image, there are > >> two > >> >> problems with this approach. If we insert a new file in the middle of > >> the > >> >> order, it shifts the rest of the files down. If a file grows, it also > >> >> shifts the rest of the files down. This results in unnecessary > >> >> reshuffling > >> >> of the disk blocks on the device. > >> >> > >> > > >> > Another alternative is to use a template image instead of a base image. > >> >> Take an existing shipping image and zero out the content of all the > >> files > >> >> so we can compress this image. We can then check in this smaller image > >> >> and > >> >> use it as our base image. The drawback with this approach is that we > >> will > >> >> need to occasionally update this template as we add more files to the > >> >> image. > >> >> > >> > > >> > Agreed that it's tricky. Another heuristic-based alternative is to split > >> > the > >> > image contents into a part that changes often (e.g, files coming from > >> all > >> > chromiumos-overlay packages) and a part that doesn't change that often > >> > (e.g., > >> > files coming from the rest of the packages). Then allocate the two into > >> two > >> > separate parts of the image. I'm sure one can come up with more clever > >> ways > >> > to > >> > do this... > >> > > >> > > >> > > >> > On Fri, Dec 10, 2010 at 10:36 AM, <mailto:petkov@chromium.org> wrote: > >> >> > >> > > >> > > In regard to zeroing out the free space -- it will benefit both full > >> and > >> >> > delta > >> >> > updates because the free space will compress better. So, we should do > >> it > >> >> > regardless of the presence of a base image. > >> >> > > >> >> > > >> >> > > >> >> > http://codereview.chromium.org/5682006/ > >> >> > > >> >> > >> > > >> > > >> > > >> > http://codereview.chromium.org/5682006/ > >> > > >> > > > > > > > > http://codereview.chromium.org/5682006/ > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
