Re: [PATCH RFC] meson: add option to use zstd for qcow2 compression by default

[Paolo Bonzini <pbonzini@redhat.com>]

On 17/06/21 21:51, Vladimir Sementsov-Ogievskiy wrote:
> So, it's an RFC. I also can split the patch so that refactoring of
> qcow2_co_create() go in a separate preparation patch.
> 
> Another RFC question, shouldn't we move to zstd by default in upstream
> too?

I think backwards-incompatible changes in the past were not handled with 
build options, but that can be changed.

However I would prefer to have an option like 
--with-qcow2-compression={zstd,zlib}.  Meson supports multiple-choice 
options, they don't have to use enabled/disabled or (if boolean) true/false.

Regarding changing the default, that would make images unreadable to 
QEMU 5.0 and earlier versions.  Does that apply to images that have no 
compressed clusters?

Paolo

>   configure         | 10 +++++++++-
>   meson.build       |  4 ++++
>   block/qcow2.c     | 32 +++++++++++++++++++++++++-------
>   meson_options.txt |  2 ++
>   4 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/configure b/configure
> index debd50c085..b19af43525 100755
> --- a/configure
> +++ b/configure
> @@ -385,6 +385,7 @@ snappy="auto"
>   bzip2="auto"
>   lzfse="auto"
>   zstd="auto"
> +qcow2_zstd_default="no"
>   guest_agent="$default_feature"
>   guest_agent_with_vss="no"
>   guest_agent_ntddscsi="no"
> @@ -1318,6 +1319,10 @@ for opt do
>     ;;
>     --enable-zstd) zstd="enabled"
>     ;;
> +  --disable-qcow2-zstd-default) qcow2_zstd_default="disabled"
> +  ;;
> +  --enable-qcow2-zstd-default) qcow2_zstd_default="enabled"
> +  ;;
>     --enable-guest-agent) guest_agent="yes"
>     ;;
>     --disable-guest-agent) guest_agent="no"
> @@ -1903,6 +1908,8 @@ disabled with --disable-FEATURE, default is enabled if available
>                     (for reading lzfse-compressed dmg images)
>     zstd            support for zstd compression library
>                     (for migration compression and qcow2 cluster compression)
> +  qcow2-zstd-default  Use zstd by default for qcow2 image creation
> +                  (requires zstd enabled)
>     seccomp         seccomp support
>     coroutine-pool  coroutine freelist (better performance)
>     glusterfs       GlusterFS backend
> @@ -6424,7 +6431,8 @@ if test "$skip_meson" = no; then
>           -Dcurl=$curl -Dglusterfs=$glusterfs -Dbzip2=$bzip2 -Dlibiscsi=$libiscsi \
>           -Dlibnfs=$libnfs -Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
>           -Drbd=$rbd -Dlzo=$lzo -Dsnappy=$snappy -Dlzfse=$lzfse \
> -        -Dzstd=$zstd -Dseccomp=$seccomp -Dvirtfs=$virtfs -Dcap_ng=$cap_ng \
> +        -Dzstd=$zstd -Dqcow2_zstd_default=$qcow2_zstd_default \
> +        -Dseccomp=$seccomp -Dvirtfs=$virtfs -Dcap_ng=$cap_ng \
>           -Dattr=$attr -Ddefault_devices=$default_devices \
>           -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
>           -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \
> diff --git a/meson.build b/meson.build
> index d8a92666fb..3d65b6c46b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -484,6 +484,9 @@ if not get_option('zstd').auto() or have_block
>                       required: get_option('zstd'),
>                       method: 'pkg-config', kwargs: static_kwargs)
>   endif
> +if not zstd.found() and get_option('qcow2_zstd_default').enabled()
> +  error('--enable-qcow2-zstd-default: Cannot use zstd by default without enabling zstd')
> +endif
>   gbm = not_found
>   if 'CONFIG_GBM' in config_host
>     gbm = declare_dependency(compile_args: config_host['GBM_CFLAGS'].split(),
> @@ -1168,6 +1171,7 @@ config_host_data.set('CONFIG_GETTID', has_gettid)
>   config_host_data.set('CONFIG_MALLOC_TRIM', has_malloc_trim)
>   config_host_data.set('CONFIG_STATX', has_statx)
>   config_host_data.set('CONFIG_ZSTD', zstd.found())
> +config_host_data.set('CONFIG_QCOW2_ZSTD_DEFAULT', get_option('qcow2_zstd_default').enabled())
>   config_host_data.set('CONFIG_FUSE', fuse.found())
>   config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
>   config_host_data.set('CONFIG_X11', x11.found())
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ee4530cdbd..06bfbbf7b8 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3540,17 +3540,36 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>           }
>       }
>   
> -    if (qcow2_opts->has_compression_type &&
> -        qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
> -
> -        ret = -EINVAL;
> -
> -        if (version < 3) {
> +    if (version < 3) {
> +        if (qcow2_opts->has_compression_type &&
> +            qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB)
> +        {
> +            ret = -EINVAL;
>               error_setg(errp, "Non-zlib compression type is only supported with "
>                          "compatibility level 1.1 and above (use version=v3 or "
>                          "greater)");
>               goto out;
>           }
> +    } else {
> +        if (qcow2_opts->has_compression_type) {
> +            compression_type = qcow2_opts->compression_type;
> +#ifdef CONFIG_QCOW2_ZSTD_DEFAULT
> +        } else {
> +            compression_type = QCOW2_COMPRESSION_TYPE_ZSTD;
> +#endif
> +        }
> +
> +#ifndef CONFIG_ZSTD
> +        assert(compression_type == QCOW2_COMPRESSION_TYPE_ZLIB);
> +#endif
> +    }
> +
> +    if (qcow2_opts->has_compression_type &&
> +        qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
> +
> +        ret = -EINVAL;
> +
> +        compression_type = qcow2_opts->compression_type;
>   
>           switch (qcow2_opts->compression_type) {
>   #ifdef CONFIG_ZSTD
> @@ -3562,7 +3581,6 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>               goto out;
>           }
>   
> -        compression_type = qcow2_opts->compression_type;
>       }
>   
>       /* Create BlockBackend to write to the image */
> diff --git a/meson_options.txt b/meson_options.txt
> index 3d304cac96..8af9bb97f5 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -108,6 +108,8 @@ option('xkbcommon', type : 'feature', value : 'auto',
>          description: 'xkbcommon support')
>   option('zstd', type : 'feature', value : 'auto',
>          description: 'zstd compression support')
> +option('qcow2_zstd_default', type : 'feature', value : 'disabled',
> +       description: 'Use zstd compression type for qcow2 image creation by default')
>   option('fuse', type: 'feature', value: 'auto',
>          description: 'FUSE block device export')
>   option('fuse_lseek', type : 'feature', value : 'auto',
>