- Download Diff

Summary:

9627 No longer need 32-bit boot_archive

Review Request #1120 — Created June 27, 2018 and submitted July 16, 2018, 10:36 a.m.

Information
Owner:	citrus
Repository:	illumos-gate
Branch:	master
Bugs:	9627, 9628
Depends On:
Commit:	ecaf22c...
Reviewers
Groups:	general
People:	citrus

Description

9627 No longer need 32-bit boot_archive / 9628 UFS boot archives are too large

Testing Done

Tested by creating HSFS and UFS archives with both old and new code, comparing the contents and test booting a server with the new archives.

Also fixed a bug with UFS archives where too much space was being reserved (issue 9628)

Before: boot archive is almost twice as big as necessary due to reserving far too much space for directories.

bloody# /boot/solaris/bin/create_ramdisk --nocompress
updating /platform/i86pc/amd64/boot_archive
updating /platform/i86pc/boot_archive
bloody# gzip -dc /platform/i86pc/amd64/boot_archive > /tmp/l
bloody# ls -l /tmp/l
-rw-r--r--   1 root     root        175M Jun 27 00:49 /tmp/l
bloody# mount `lofiadm -a /tmp/l` /mnt
bloody# df -h /mnt
Filesystem             Size   Used  Available Capacity  Mounted on
/dev/lofi/1            163M  76.8M      70.2M    53%    /mnt
bloody# du -hs /mnt
75.8M   /mnt

and after:

bloody# /boot/solaris/bin/create_ramdisk.new --nocompress
updating /platform/i86pc/amd64/boot_archive
bloody# gzip -dc /platform/i86pc/amd64/boot_archive > /tmp/l
bloody# ls -l /tmp/l
-rw-r--r--   1 root     root       91.7M Jun 27 00:55 /tmp/l
bloody# mount `lofiadm -a /tmp/l` /mnt
bloody# df -h /mnt
Filesystem             Size   Used  Available Capacity  Mounted on
/dev/lofi/1           85.8M  76.8M      8.98M    90%    /mnt
bloody# du -hs /mnt
75.8M   /mnt

Issues

0
0
3
0
All Issues: 3

Description	From	Last Updated
Do we need this enum and all arguments of this type at all now? e.g. dircache_updatefile(path, FILE64) below -- the ...	yuripv	June 27, 2018, 2:16 p.m.
Guess we should print loud warning now so that everyone is aware thir 32bit modules aren't going to do anything ...	yuripv	June 27, 2018, 2:16 p.m.
We are no longer?	yuripv	June 27, 2018, 2:16 p.m.

People:

+	citrus

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)

Show all issues

Do we need this enum and all arguments of this type at all now? e.g. dircache_updatefile(path, FILE64) below -- the second argument is useless IMO.

citrus June 27, 2018, 9:26 a.m.

I chose to leave the multi-archive framework in place rather than strip it out. A framework that (currently) handles only one archive type is still valid but I'm interested in opinions here and am happy to go with the concensus.

yuripv June 27, 2018, 9:29 a.m.

OK. My opinion is "rip it out, multi-arch is useless and unhelpful" :-)

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)

Show all issues

Guess we should print loud warning now so that everyone is aware thir 32bit modules aren't going to do anything anymore?

citrus June 27, 2018, 9:26 a.m.

I can change that to an unconditional WARNING. The debug print is not triggering on any of my test systems so we should not expect any 32-bit objects.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)

Show all issues

We are no longer?

citrus June 27, 2018, 9:26 a.m.

Still paranoid in that we include the object in the archive "just in case".
Again this is not triggering on any of my test systems.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)

This for looks a bit useless now, doing exactly one iteration.

citrus June 27, 2018, 9:26 a.m.

See my reply to your first comment - the framework has been left in place.

Ship it!

Ship It!

citrus June 27, 2018, 11:07 p.m.

Is there any chance you could test that this doesn't break illumos on SPARC?

igork June 28, 2018, 2:42 a.m.

nope :)

i have fixed it a long time ago on my DilOS tree - i have no 32bit Intel kernel and did update bootadm.c by myself in the same/similar ways. if changes will break SPARC - i'll let you know.

Ship it!

```
Ship It!
```

Change Summary:

Drop multi-arch from bootadm.c

Commit:

-	116814fd4cc25fb2c9fe64b82b939befa0aad86a
+	679025631f3b7a44694da848cda4b5320f2ec9fd

Diff:

Revision 2 (+183 -348)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Ship it!

```
Thank you.
```

Change Summary:

Small updates following internal review. The only functional change is to stop including any ELF objects found which are neither 32 nor 64-bit - i.e. stop being paranoid. None of my test systems have anything that matched that check anyway.
Please can you confirm that you are still happy with this change, thanks.

Commit:

-	679025631f3b7a44694da848cda4b5320f2ec9fd
+	97625150cd045e8836a41fff190c8f7aa94dc289

Diff:

Revision 3 (+197 -373)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
```
Not ELF probably?
```
1. citrus June 28, 2018, 11:40 a.m.
  Well, whatever it is, the first 4 bytes match the ELF magic number..
2. yuripv June 28, 2018, 11:42 a.m.
  Indeed.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)

This could be:

if (strstr(path, "/amd64") != NULL && has_cachedir())


to reduce indentation.

Ship it!

Looks good except for some nits and the red ugliness (doesn't updated cstyle whine about it?) -- don't HAVE to fix those, so in any case, ship it!

citrus June 28, 2018, 11:40 a.m.

That's just <space><tab> in a couple of places. I can fix those before RTI (just didn't want to make the diff larger than necessary)

tsoome June 30, 2018, 5:31 a.m.

you can post it as separate update (rbt -u). If we could have decent RTI process, cstyle fix could be just separate commit...

Change Summary:

Whitespace fixes s/<space><tab>/<tab>/

Commit:

-	97625150cd045e8836a41fff190c8f7aa94dc289
+	6fee39baa1f03c43b377c3ea38d7074fa3e01143

Diff:

Revision 4 (+252 -428)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Change Summary:

Fix bug found in further testing that prevented compressed UFS archives from being created.
(they aren't used, but this update should not break the functionality)

Commit:

-	6fee39baa1f03c43b377c3ea38d7074fa3e01143
+	ecaf22caebc984c89972aeea3d234c9e680cd3db

Diff:

Revision 5 (+251 -428)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

usr/src/cmd/boot/scripts/create_ramdisk.ksh (Diff revisions 4 - 5)

Should it be compress=no instead of removing the line?

citrus July 5, 2018, 1:48 p.m.

No. compress is set to yes at the top of the script and can be set to no during options parsing if the user provided --nocompress. This block should not change compress for x86.

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

9627 No longer need 32-bit boot_archive

People:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)