Re: [PATCH v2 2/2] fadump: preserve file modification time to help with hardlinking

[Hari Bathini]

On 09/11/22 2:18 pm, Coiby Xu wrote:
>  On Sat, Nov 05, 2022 at 12:10:28AM +0530, Hari Bathini wrote:
> >
> >
> > On 04/11/22 4:21 pm, Philipp Rudo wrote:
> >> Hi Hari,
> >>
> >> just a small nit.
> >
> > Thanks for the review, Philipp.
> >
> >> On Mon, 31 Oct 2022 15:42:21 +0530
> >> Hari Bathini <hbathini(a)linux.ibm.com&gt; wrote:
> >>
> >>> With commit fa9201b2 ("fadump: isolate fadump initramfs image within
> >>> the default one"), initramfs image gets to hold two images, one for
> >>> production kernel boot purpose and the other for capture kernel boot.
> >>> Most files are common among the two images. Retain file modification
> >>> time to replace duplicate files with hardlinks and save space. Also,
> >>> avoid unnecessarily compressing fadump image that is decompressed
> >>> immediately anyway.
> >>>
> >>> Signed-off-by: Hari Bathini <hbathini(a)linux.ibm.com&gt;
> >>> ---
> >>>  mkfadumprd | 10 ++++++----
> >>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/mkfadumprd b/mkfadumprd
> >>> index 5587ccf..4a4b98f 100644
> >>> --- a/mkfadumprd
> >>> +++ b/mkfadumprd
> >>> @@ -40,14 +40,16 @@ touch "$MKFADUMPRD_TMPDIR/fadump.initramfs"
> >>>  ddebug "rebuild fadump initrd: $FADUMP_INITRD $DEFAULT_INITRD 
> >>> $KDUMP_KERNELVER"
> >>>  # Don't use squash for capture image or default image as it 
> >>> negatively impacts
> >>>  # compression ratio and increases the size of the initramfs image.
> >>> -if ! $MKDUMPRD "$FADUMP_INITRD" -i 
> >>> "$MKFADUMPRD_TMPDIR/fadump.initramfs" /etc/fadump.initramfs --omit
>
> >>> squash; then
> >>> +# Don't compress the capture image as uncompressed image is needed 
> >>> immediately.
> >>> +# Also, early microcode would not be needed here.
> >>> +if ! $MKDUMPRD "$FADUMP_INITRD" -i 
> >>> "$MKFADUMPRD_TMPDIR/fadump.initramfs" /etc/fadump.initramfs --omit
>
> >>> squash --no-compress --no-early-microcode; then
> >>>      perror_exit "mkfadumprd: failed to build image with dump 
> >>> capture support"
> >>>  fi
> >>> -### Unpack the initramfs having dump capture capability
> >>> +### Unpack the initramfs having dump capture capability retaining 
> >>> previous file modification time.
> >>> +# This helps in saving space by hardlinking identical files.
> >>>  mkdir -p "$MKFADUMPRD_TMPDIR/fadumproot"
> >>> -if ! (pushd "$MKFADUMPRD_TMPDIR/fadumproot" > /dev/null
> && lsinitrd 
> >>> --unpack "$FADUMP_INITRD" &&
> >>> -    popd > /dev/null); then
> >>> +if ! (cpio -id --preserve-modification-time --quiet -D 
> >>> "$MKFADUMPRD_TMPDIR/fadumproot" < "$FADUMP_INITRD");
> then
> >>
> >> without changing the directories I don't think you need the subshell
> >> here.
> >
> > True.
> >
> > Coiby, can you take care of this while merging (assuming you are ok with
> > the changes). Do let me know if you are expecting a respin from me.
>  
>  Patches merged with the aforementioned change, thanks! 
Thanks a lot!

>  
>  Btw, during the testing, I notice something strange,
>  1. Applying this patch set to 2.0.25-2.el9 and 2.0.25-4.el9 leads to an
>      initramfs of 44M and 49M respectively. The only difference between
>      these two files is the files provided by kexec-tools package,
>      - when measuring in the MB unit, they have the same size after
>        unpacking
>      - if I repack the unpacked files, they have the same size again 
I will look into this.

>  2. On ppc64le, with squash enabled, why aren't kdump or fadump
> initramfs
>      compressed? If compressed, at least the fadump initrams would not 
>  hit the
>      limit of grub,
>      $ file /boot/initramfs-5.14.0-185.el9.ppc64le.img      
>  /boot/initramfs-5.14.0-185.el9.ppc64le.img: ASCII cpio archive (SVR4 
>  with no CRC)
>  
>      $ file -i /boot/initramfs-5.14.0-185.el9.ppc64lekdump.img    
>  /boot/initramfs-5.14.0-185.el9.ppc64lekdump.img: 
>  application/octet-stream; charset=binary
>  
>      $ du -hs /boot/initramfs-5.14.0-185.el9.ppc64le.img      73M     
>  /boot/initramfs-5.14.0-185.el9.ppc64le.img
>      $ lsinitrd --unpack /boot/initramfs-5.14.0-185.el9.ppc64le.img
>      $ find . | cpio -o -c -R root:root | gzip -9 > ../repacked_gzipped.img
>        147817 blocks
>      $ du -hs ../repacked_gzipped.img
>        63M   ../repacked_gzipped.img 
I could not locate the exact commit that did this but if I recall past
discussions around this topic, the conclusion was squash already does
compression and doing a double compression with gzip on top is
not going to add much benefit in terms of space while slowing the
system down a bit to uncompress..

Thanks
Hari