Re: [PATCH 3/4] Split "auto" compression mode into its own path

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 6 Apr 2024 14:06:39 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Apr 05, 2024 at 08:10:02PM +0200, Stefano Brivio wrote:
> > On Fri, 22 Mar 2024 13:27:38 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > mbuto supports "auto" compression mode where we detect the fastest
> > > compressor and use it.  This is structured a bit oddly - cpio_compress()
> > > first handles the case of an explicitly selected compressor, then handles
> > > the auto-detected case, redundantly actually implementing the compression
> > > once it has picked one.  
> > 
> > Yes, we don't want to risk keeping around several formats if the
> > archives are big... but you're not changing this, right?  
> 
> I don't believe so, no.
> 
> > > Make this a bit clearer: first handle the "auto" case by calling out to
> > > the testing code, and using that to set the parameter for the specific
> > > compression path.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  mbuto | 53 ++++++++++++++++++++++++-----------------------------
> > >  1 file changed, 24 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/mbuto b/mbuto
> > > index 49d032c..550f76e 100755
> > > --- a/mbuto
> > > +++ b/mbuto
> > > @@ -566,31 +566,11 @@ cpio_init() {
> > >  	fi
> > >  }
> > >  
> > > -# cpio_compress() - Compress archive, test available methods if none is selected
> > > +# compress_select() - Try compressors and pick the fastest  
> > 
> > Now we can say we pick the fastest, but it's not clear in what: we care
> > about decompression, so perhaps:
> > 
> > # compress_select() - Try compressors and pick the fastest to decompress image
> > 
> > ?  
> 
> I don't think that's accurate.  AFAICT the code is selecting the
> fastest to compress, not decompress,

No, why? Look:

                __start=$("${CAT}" /proc/uptime)
                __start="${__start% *}"

                for _ in $("${SEQ}" 1 5); do
                        "${CP}" "${compress_test1}" "${compress_test2}"
                        ${__cmd} --force -d -c "${compress_test2}" > /dev/null
                done

                __end=$("${CAT}" /proc/uptime)
                __end="${__end% *}"

see the '-d' there ('-c' is usually '--stdout').

> and I didn't change that.  I agree that optimizing decompression
> speed would make more sense.
> 
> Honestly, I'm not really convinced that the auto mode is useful
> anyway: even if we changed to decompression speed, the speed on the
> location running mbuto isn't necessarily the same as on the target
> doing the decompression (though given mbuto's usage model, it's
> lkely).

Well, it's exactly the intended usage -- any other usage would be
rather tricky (at least, anything with a different architecture for the
guest). That's how the current passt test suite uses it, too. And it
really makes a difference if you're working with kselftests.

-- 
Stefano