9627 No longer need 32-bit boot_archive 9628 UFS boot archives are too large 9721 cmd/boot: support cpio boot archive

Review Request #1189 — Created Aug. 29, 2018 and submitted

citrus
illumos-gate
master
9627, 9628, 9721
1156
5e9492b...
general
9627 No longer need 32-bit boot_archive 9628 UFS boot archives are too large 9721 cmd/boot: support cpio boot archive

This has been tested on OmniOS, creating HSFS and UFS archives with both
old and new code and comparing contents, and test booting. Also testing with CPIO.
For the size calculation problem, there is more testing information in the issue -
1024x too much space was being reserved for directories.

With this change I'm adding a new property on the system/boot-archive:default service to specify the desired archive format; this defaults to cpio. This property is honoured by both the C bootadm code, which is responsible for building the HSFS format archives, and by the script that handles UFS and CPIO.
If the property is not present then the default is HSFS if /usr/bin/mkisofs is present, and CPIO otherwise. If the property is set to an unknown value then CPIO will be used.

The man page for bootadm has been updated to include information on the new option and also to show the -f option for bootadm update-archive (force archive creation) which was previously undocumented.

Testing

On my test system a boot archive contains 1056 files, 92 directories and 216 hardlinks (mostly CPU firmware).

During system boot, 2953 file opens are performed of which 2274 result in ENOENT; the total data read from the archive is 46MiB.

Testing with each of the four archive formats below shows that processing time during boot is in the range 3-9 seconds for all formats, with different boots giving different results. On average archive processing takes around 5 seconds regardless of format.

bloody% svcprop -p config/format boot-archive
cpio

bloody% time pfexec bootadm update-archive -f
updating platform/i86pc/amd64/boot_archive (CPIO)
pfexec bootadm update-archive -f  5.56s user 0.15s system 98% cpu 5.826 total

bloody% file /platform/i86pc/amd64/boot_archive
/platform/i86pc/amd64/boot_archive:     gzip compressed data - deflate method , original file name
bloody% gzip -dc /platform/i86pc/amd64/boot_archive > /tmp/l
bloody% file /tmp/l
/tmp/l:         ASCII cpio archive - CHR (-c) header
bloody% ls -lh /platform/i86pc/amd64/boot_archive /tmp/l
-rw-r--r--   1 root     root       31.6M Jul  5 15:45 /platform/i86pc/amd64/boot_archive
-rw-r--r--   1 af       other      83.1M Jul  5 15:45 /tmp/l

bloody% svccfg -s boot-archive setprop config/format = ufs
bloody% time pfexec bootadm update-archive -f
updating platform/i86pc/amd64/boot_archive (UFS)
pfexec bootadm update-archive -f  3.86s user 0.92s system 6% cpu 1:15.96 total

bloody% file /platform/i86pc/amd64/boot_archive
/platform/i86pc/amd64/boot_archive:     English text
bloody% ls -l /platform/i86pc/amd64/boot_archive
-rw-r--r--   1 root     root       45.8M Jul  5 15:48 /platform/i86pc/amd64/boot_archive

bloody% svccfg -s boot-archive setprop config/format = hsfs
bloody% time pfexec bootadm update-archive -f
updating //platform/i86pc/amd64/boot_archive (HSFS)
pfexec bootadm update-archive -f  0.17s user 0.12s system 1% cpu 21.938 total

bloody% file /platform/i86pc/amd64/boot_archive
/platform/i86pc/amd64/boot_archive:     ISO 9660 filesystem image
bloody% ls -lh /platform/i86pc/amd64/boot_archive
-rw-r--r--   1 root     root       36.4M Jul  5 15:46 /platform/i86pc/amd64/boot_archive

bloody% svccfg -s boot-archive setprop config/format = ufs-nocompress
bloody% time pfexec bootadm update-archive -f
updating platform/i86pc/amd64/boot_archive (UFS-nocompress)
pfexec bootadm update-archive -f  5.32s user 1.54s system 5% cpu 1:56.01 total

bloody% file /platform/i86pc/amd64/boot_archive
/platform/i86pc/amd64/boot_archive:     gzip compressed data - deflate method , original file name
bloody% gzip -dc /platform/i86pc/amd64/boot_archive >| /tmp/l
bloody% file /tmp/l
/tmp/l:         English text
bloody% ls -lh /platform/i86pc/amd64/boot_archive /tmp/l
-rw-r--r--   1 root     root       27.5M Jul  5 15:43 /platform/i86pc/amd64/boot_archive
-rw-r--r--   1 af       other      91.7M Jul  5 15:44 /tmp/l

Also, testing with multiple mounted boot environments with differing properties, including unset (i.e. an old BE)

omni# beadm list | awk '$3 ~ /\// {print $3}' | while read mp; do
echo "$mp: \\c"
SVCCFG_REPOSITORY=$mp/etc/svc/repository.db svccfg -s boot-archive listprop config/format
done

/: config/format  astring  cpio
/hsfs: config/format  astring  hsfs
/ufs: config/format  astring  ufs
/ufs-nocompress: config/format  astring  ufs-nocompress
/cpio: config/format  astring  cpio
/unset: zsh: exit 1


omni# bootadm -a update_all -f
updating /platform/i86pc/amd64/boot_archive (CPIO)
Creating boot_archive for /hsfs
updating /hsfs//platform/i86pc/amd64/boot_archive (HSFS)
Creating boot_archive for /ufs
updating /ufs/platform/i86pc/amd64/boot_archive (UFS)
Creating boot_archive for /ufs-nocompress
updating /ufs-nocompress/platform/i86pc/amd64/boot_archive (UFS-nocompress)
Creating boot_archive for /cpio
updating /cpio/platform/i86pc/amd64/boot_archive (CPIO)
Creating boot_archive for /unset
updating /unset/platform/i86pc/amd64/boot_archive (UFS)
  • 0
  • 0
  • 11
  • 7
  • 18
Description From Last Updated
citrus
citrus
alp
  1. 
      
  2. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)
     
     

    Old code called dircache_updatefile() in default case. Should the new code do the same?

    1. I dropped the /* paranoid */ and elected not to copy the file across in this case.
      It's a file which has an ELF magic but cannot be identified as ELFCLASS32 or ELFCLASS64 (non-ELF files are always copied over).
      I checked and this clause is not matching any file in the current boot archive.

      It's something that was changed during the previous review cycle (https://illumos.org/rb/r/1120/)

      I think it's safe to remove this now.

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

    better switch the return type to bool - we only return 0 or 1 values there.

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

    I'd like to have verbose log there to track what is going on, please add:

    if (bam_verbose)                                bam_print("mkisofs not found\n");
    
  4. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)
     
     

    also for verbose tracking:

    if (bam_verbose) {
      bam_print("%s does not have %s/%s property, using "
           "mkisofs\n", BOOT_ARCHIVE_FMRI, SCF_PG_CONFIG,
           SCF_PROPERTY_FORMAT);
    }
    
  5. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 1)
     
     

    and here:

    if (bam_verbose) {
        bam_print("creating %s boot archive\n",
            format != NULL? format : "hsfs");
    }
    
    1. Done slightly differently, but fixed.

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

    this printout can now be removed as we just did announce in use_mkisofs(), also the message is wrong anyhow.

  7. "on x86 get rid of transient boot configuration"

    both grub and loader use transient config.

    1. I'm not changing anything here.. This is the start script for svc:/system/boot-archive-update:default so is post-boot clean-up

    2. yep, just noting that the comment is a bit misleading - this is not grub specific cleanup.

  8. 
      
tsoome
  1. 
      
  2. missing:

           -f)     shift
                   FORMAT="$1"
                   ;;
    
  3. add error printout here?

  4. 
      
tsoome
  1. 
      
  2. actually for sparc we should default to compress=yes, because it will compress individual files in the archive (as opposed to compress the entire boot archive). loader and grub can gunzip the boot_archive while loading it, but sparc can not, which is why we gzip individual files there.

    I am not entirely sure about whole archive versus files compression on x86 - whole archive compression will save us a bit more loading bandwith, individual compressed files will save us memory needed for boot archive.

    Also there is a question, should we still need uncompressed kernel on sparc, because x86 seems to be able to cope with compressed kernel just fine...

    1. The old code used compress=no for SPARC, see https://github.com/illumos/illumos-gate/blob/master/usr/src/cmd/boot/scripts/create_ramdisk.ksh#L112

      so this would be a separate change.

  3. BOOT_ARCHIVE is set as relative path (without starting '/'), should we set archive with "/$ALT_ROOT" here? The message below is confusing..

  4. There we do have one small issue - we always use mkfile below, therefore we always will create new archive there.

  5. here we need $GZIP_CMD

  6. 
      
tsoome
  1. boot after pkg update, 32-bit archive is gone:

    tsoome@uefi-oi:~$ ls -l /platform/i86pc/
    total 10
    drwxr-xr-x   4 root     sys            6 aug 30 12:10 amd64
    drwxr-xr-x  10 root     sys           10 sept 26  2017 kernel
    drwxr-xr-x   4 root     sys            8 jaan 19  2018 ucode
    drwxr-xr-x   2 root     root           2 aug 22 11:10 updates
    tsoome@uefi-oi:~$
    
    tsoome@uefi-oi:~$ sudo -s
    Password: 
    root@uefi-oi:/home/tsoome# svcprop -p config/format boot-archive
    cpio
    root@uefi-oi:/home/tsoome# svccfg -s boot-archive setprop config/format = ufs
    root@uefi-oi:/home/tsoome# svccfg -s boot-archive:default refresh
    root@uefi-oi:/home/tsoome# svcprop -p config/format boot-archive
    ufs
    root@uefi-oi:/home/tsoome# bootadm update-archive -fv
    forced update of archive requested
    cannot find: /etc/cluster/nodeid: No such file or directory
    cannot find: /etc/devices/mdi_ib_cache: No such file or directory
    cannot find: /etc/devices/retire_store: No such file or directory
    creating ufs boot archive
    updating platform/i86pc/amd64/boot_archive (UFS)
    root@uefi-oi:/home/tsoome# 
    root@uefi-oi:/home/tsoome# reboot
    

    system booted OK with cpio/ufs/ufs-nocompress/hsfs, however, there is one change missing:

    diff --git a/usr/src/cmd/halt/halt.c b/usr/src/cmd/halt/halt.c
    index 166e5a163b..657f207a85 100644
    --- a/usr/src/cmd/halt/halt.c
    +++ b/usr/src/cmd/halt/halt.c
    @@ -1584,12 +1584,12 @@ main(int argc, char *argv[])
            if (cmd != A_DUMP) {
                    int     start, end, delta;
    
    -               (void) kill(-1, SIGTERM);
    -               start = time(NULL);
    -
                    if (zoneid == GLOBAL_ZONEID && !nosync)
                            do_archives_update(fast_reboot);
    
    +               (void) kill(-1, SIGTERM);
    +               start = time(NULL);
    +
                    end = time(NULL);
                    delta = end - start;
                    if (delta < 5)
    

    it is needed to make sure we can get format from SMF, otherwise we will have format ping-pong...

  2. 
      
citrus
citrus
tsoome
  1. 
      
  2. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
     
     

    update comment too:)

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

    gcc7:

    bootadm.c:3332:5: error: suggest explicit braces to avoid ambiguous 'else' [-Werror=dangling-else]
    
  3. 
      
citrus
tsoome
  1. Ship It!
  2. 
      
ptribble
  1. 
      
  2. Should that not be "Unsupported archive format"?

  3. 
      
ptribble
  1. 
      
  2. Should ufs-nocompress be shown in the usage output?

    1. For interactive use (testing), it would be -f ufs --nocompress

  3. 
      
ptribble
  1. Is the logic correct in the following situation: An updated system running this code running bootadm -R against an OS instance that doesn't have cpio support in the kernel?

    Will the system still generate a cpio archive in this case, because that's the default? Then the system won't boot.

    This is one case of a general issue, of what happens when it can't contact SMF? If SMF isn't running, then I believe the error path is safe in the simplest case of that. But with an alternate root, using SMF seems entirely unsafe.

    Adding an SMF dependency seems undesirable in any case.

    How about setting a property on the zfs root filesystem for the BE instead?

    I think you have to be absolutely sure that the target instance you're generating the archive for understands the format you're generating. So you can only use cpio if you can confirm it's valid, else the old hsfs/ufs has to be the default.

    1. Entirely right, good spot.
      I've updated the diff to check the SMF property in the alternate root that is being updated.
      If the property is not found then a UFS archive will be generated.

  2. 
      
citrus
citrus
citrus
ptribble
  1. 
      
  2. First, shouldn't that be $FORMAT, not $1, in the error message?

    Second, what if the admin sets the archive property to mkisofs, but that doesn't work because mkisofs is missing (for whatever reason)? We then fall through to running this script. In the old implementation, that was fine because we then tried ufs. But in this case we error out.

    I believe we shouldn't error out on invalid input, but choose a sensible format here. Whether we do or we don't, we need to produce a more helpful error or warning: both for completely invalid formats, and separately for the case where mkisofs is asked for but not available.

    (And yes, there's the possibility that an a;ternate BE specifies mkisofs but the current system doesn't have it.)

    1. Yes, fixed, thanks.

      Fallback is handled earlier on in the script, this *) case should actually never be triggered - it's just belt and braces.

      omni# bootadm update-archive -R /hsfs -f
      Creating boot_archive for /hsfs
      updating /hsfs//platform/i86pc/amd64/boot_archive (HSFS)
      
      omni# mv /usr/bin/mkisofs{,~}
      
      omni# bootadm update-archive -R /hsfs -f
      Creating boot_archive for /hsfs
      bootadm: cannot create hsfs archive
      Failed to create HSFS archive, falling back.
      updating /hsfs/platform/i86pc/amd64/boot_archive (UFS)
      
    2. If it's never going to be triggered (and reading the script, it can't be) then the error case should be removed entirely. This block isn't really checking for valid formats at all. What it's really doing is:

      Checking that /usr/bin/cpio exists for cpio archives to be viable, and

      Splitting ufs-nocompress into its constituent parts.

    3. Fair enough, removed.

  3. 
      
citrus
ptribble
  1. Ship It!
  2. 
      
citrus
tsoome
  1. Ship It!
  2. 
      
citrus
Review request changed

Status: Closed (submitted)

Loading...