New issue
Advanced search Search tips

Issue 706484 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

e2fsprogs: mkfs.ext4 -U should support same options as tune2fs

Project Member Reported by vapier@chromium.org, Mar 29 2017

Issue description

specifically, it would let us simplify mk_fs in src/scripts/build_library/disk_layout_util.sh because we could pass the $fs_uuid to the mkfs.ext4 step instead of tune2fs.  the tune2fs step requires us to rebuild checksums in the image.
 

Comment 1 by sjg@chromium.org, Apr 10 2017

Owner: ddavenp...@chromium.org

Comment 2 by sjg@chromium.org, Apr 10 2017

Status: Assigned (was: Available)

Comment 3 by sjg@chromium.org, Apr 12 2017

Labels: Team-BLD
Patch sent to e2fsprogs maintainers (http://www.spinics.net/lists/linux-ext4/msg56020.html).

Per conversation with vapier, once this is applied we can wait for a new version of e2fsprogs containing this change to be released and update to this new version in portage.

Comment 5 by vapier@chromium.org, Apr 13 2017

nice!  you'll probably want to update the docs too though -- see the -U option in misc/mke2fs.8.in.  you can probably copy & paste the content from misc/tune2fs.8.in.
Thanks for the feedback - the reviewer also noted the same thing. I updated the documentation and uploaded a new path, so I guess now it's just a matter of waiting until it gets merged.

In the meantime, I noticed that if one does not supply a -U argument to mke2fs it generates a random one. The semantics are slightly different, as highlighted here:

https://linux.die.net/man/3/uuid_generate

Supplying -U random for tune2fs (and for mke2fs after my patch) will use uuid_generate_random, where omitting -U will use uuid_generate.

Is the difference between these two significant? If not, it seems to me that we could remove the need to call tune2fs by checking if $fs_uuid equals the string "random" and omit the -U option in that case. This could be done without needing to wait for this patch to be merged.
https://chromium.googlesource.com/chromiumos/platform/crosutils/+/master/build_library/disk_layout_util.sh#365

My understanding is that $fs_uuid will only ever be a valid UUID or the string "random"
https://chromium.googlesource.com/chromiumos/platform/crosutils/+/HEAD/build_library/README.disk_layout

Oh, a correction to what I just stated - supplying -U random will use uuid_generate, *not* uuid_generate_random, so there's no difference between passing in -U random and not passing in anything at all.

Comment 8 by vapier@chromium.org, Apr 20 2017

changing the default to uuid_generate sounds fine to me.  just make sure you note it upstream so they're aware too.

wrt disk_layout_util.sh, if we can adapt it to get the same behavior right now, that sounds fine too.  and we can note in the code of dropping the conditional logic once we update.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosutils/+/fdbefe2efa4908e23f0f7d16bb802812483f7064

commit fdbefe2efa4908e23f0f7d16bb802812483f7064
Author: Drew Davenport <ddavenport@google.com>
Date: Wed Apr 26 23:19:52 2017

Avoid changing the uuid when creating file system

The conditional logic around $fs_uuid can be removed once
mke2fs supports the same values for the -U option as tune2fs.

BUG= chromium:706484 
TEST=Built and booted an image on device and VM

Change-Id: I46f817d85807792e7ae51c2c6e326422383d2e43
Reviewed-on: https://chromium-review.googlesource.com/486148
Commit-Ready: Drew Davenport <ddavenport@chromium.org>
Tested-by: Drew Davenport <ddavenport@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/fdbefe2efa4908e23f0f7d16bb802812483f7064/build_library/disk_layout_util.sh

This patch has been applied:
http://www.spinics.net/lists/linux-ext4/msg56667.html


Status: Fixed (was: Assigned)
I'm going to go ahead and mark this as done. The original intent of this bug (avoid recomputing checksums when resetting uuid) has been addressed in a different way than originally proposed, with the cost of a couple lines of script.

The patch to change the -U parameter has been submitted and applied. We'll just have to wait for a new version of e2fsprogs to be released, then for that version to be marked stable in portage, and then upgrade the package. Or we can cherry-pick the change if we really want to get it before then, but it doesn't seem worth the trouble.

Comment 12 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment