Re: [PATCH v10 1/3] block: introduce compress filter driver

[Andrey Shinkevich]

On 28/11/2019 17:37, Vladimir Sementsov-Ogievskiy wrote:
> 28.11.2019 12:36, Andrey Shinkevich wrote:
>> Allow writing all the data compressed through the filter driver.
>> The written data will be aligned by the cluster size.
>> Based on the QEMU current implementation, that data can be written to
>> unallocated clusters only. May be used for a backup job.
>>
>> Suggested-by: Max Reitz <address@hidden>
>> Signed-off-by: Andrey Shinkevich <address@hidden>
>> ---
>>    block/Makefile.objs     |   1 +
>>    block/filter-compress.c | 166 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>    qapi/block-core.json    |  10 +--
>>    3 files changed, 173 insertions(+), 4 deletions(-)
>>    create mode 100644 block/filter-compress.c
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index e394fe0..330529b 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -43,6 +43,7 @@ block-obj-y += crypto.o
>>    
>>    block-obj-y += aio_task.o
>>    block-obj-y += backup-top.o
>> +block-obj-y += filter-compress.o
>>    
>>    common-obj-y += stream.o
>>    
>> diff --git a/block/filter-compress.c b/block/filter-compress.c
>> new file mode 100644
>> index 0000000..4aa189b
>> --- /dev/null
>> +++ b/block/filter-compress.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * Compress filter block driver
>> + *
>> + * Copyright (c) 2019 Virtuozzo International GmbH
>> + *
>> + * Author:
>> + *   Andrey Shinkevich <address@hidden>
>> + *   (based on block/copy-on-read.c by Max Reitz)
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 or
>> + * (at your option) any later version of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "block/block_int.h"
>> +#include "qemu/module.h"
>> +#include "qapi/error.h"
>> +
>> +
>> +static int compress_open(BlockDriverState *bs, QDict *options, int flags,
>> +                         Error **errp)
>> +{
>> +    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 
>> false,
>> +                               errp);
>> +    if (!bs->file) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!bs->file->bs->drv || 
>> !block_driver_can_compress(bs->file->bs->drv)) {
>> +        error_setg(errp, "Compression is not supported with this drive %s",
>> +                   bdrv_get_device_name(bs->file->bs));
> 
> I think we should rollback bdrv_open_child in this case.
> 
It is processed in the 'open_failed' section of the bdrv_open_driver()

Andrey

> Also I doubt that bdrv_get_device_name will work properly here. I think better
> something like
> "Compression is not supported for underlying format: %s", 
> bdrv_get_format_name(bs)
> 
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>> +        (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
>> +
>> +    bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
>> +        ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>> +            bs->file->bs->supported_zero_flags);
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +static int64_t compress_getlength(BlockDriverState *bs)
>> +{
>> +    return bdrv_getlength(bs->file->bs);
>> +}
>> +
>> +
>> +static int coroutine_fn compress_co_preadv_part(BlockDriverState *bs,
>> +                                                uint64_t offset, uint64_t 
>> bytes,
>> +                                                QEMUIOVector *qiov,
>> +                                                size_t qiov_offset,
>> +                                                int flags)
>> +{
>> +    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
>> +                               flags);
>> +}
>> +
>> +
>> +static int coroutine_fn compress_co_pwritev_part(BlockDriverState *bs,
>> +                                                 uint64_t offset,
>> +                                                 uint64_t bytes,
>> +                                                 QEMUIOVector *qiov,
>> +                                                 size_t qiov_offset, int 
>> flags)
>> +{
>> +    return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
>> +                                flags | BDRV_REQ_WRITE_COMPRESSED);
>> +}
>> +
>> +
>> +static int coroutine_fn compress_co_pwrite_zeroes(BlockDriverState *bs,
>> +                                                  int64_t offset, int bytes,
>> +                                                  BdrvRequestFlags flags)
>> +{
>> +    return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
>> +}
>> +
>> +
>> +static int coroutine_fn compress_co_pdiscard(BlockDriverState *bs,
>> +                                             int64_t offset, int bytes)
>> +{
>> +    return bdrv_co_pdiscard(bs->file, offset, bytes);
>> +}
>> +
>> +
>> +static void compress_refresh_limits(BlockDriverState *bs, Error **errp)
>> +{
>> +    BlockDriverInfo bdi;
>> +    int ret;
>> +
>> +    if (!bs->file) {
>> +        return;
>> +    }
>> +
>> +    ret = bdrv_get_info(bs->file->bs, &bdi);
>> +    if (ret < 0 || bdi.cluster_size == 0) {
>> +        return;
>> +    }
>> +
>> +    bs->bl.request_alignment = bdi.cluster_size;
>> +}
>> +
>> +
>> +static void compress_eject(BlockDriverState *bs, bool eject_flag)
>> +{
>> +    bdrv_eject(bs->file->bs, eject_flag);
>> +}
>> +
>> +
>> +static void compress_lock_medium(BlockDriverState *bs, bool locked)
>> +{
>> +    bdrv_lock_medium(bs->file->bs, locked);
>> +}
>> +
>> +
>> +static bool compress_recurse_is_first_non_filter(BlockDriverState *bs,
>> +                                                 BlockDriverState 
>> *candidate)
>> +{
>> +    return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
>> +}
>> +
>> +
>> +static BlockDriver bdrv_compress = {
>> +    .format_name                        = "compress",
>> +
>> +    .bdrv_open                          = compress_open,
>> +    .bdrv_child_perm                    = bdrv_filter_default_perms,
>> +
>> +    .bdrv_getlength                     = compress_getlength,
>> +
>> +    .bdrv_co_preadv_part                = compress_co_preadv_part,
>> +    .bdrv_co_pwritev_part               = compress_co_pwritev_part,
>> +    .bdrv_co_pwrite_zeroes              = compress_co_pwrite_zeroes,
>> +    .bdrv_co_pdiscard                   = compress_co_pdiscard,
>> +    .bdrv_refresh_limits                = compress_refresh_limits,
>> +
>> +    .bdrv_eject                         = compress_eject,
>> +    .bdrv_lock_medium                   = compress_lock_medium,
>> +
>> +    .bdrv_co_block_status               = bdrv_co_block_status_from_backing,
>> +
>> +    .bdrv_recurse_is_first_non_filter   = 
>> compress_recurse_is_first_non_filter,
>> +
>> +    .has_variable_length                = true,
>> +    .is_filter                          = true,
>> +};
>> +
>> +static void bdrv_compress_init(void)
>> +{
>> +    bdrv_register(&bdrv_compress);
>> +}
>> +
>> +block_init(bdrv_compress_init);
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0cf68fe..93ee04e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2884,15 +2884,16 @@
>>    # @copy-on-read: Since 3.0
>>    # @blklogwrites: Since 3.0
>>    # @blkreplay: Since 4.2
>> +# @compress: Since 5.0
>>    #
>>    # Since: 2.9
>>    ##
>>    { 'enum': 'BlockdevDriver',
>>      'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
>> -            'cloop', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 
>> 'gluster',
>> -            'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
>> -            'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 
>> 'qcow',
>> -            'qcow2', 'qed', 'quorum', 'raw', 'rbd',
>> +            'cloop', 'compress', 'copy-on-read', 'dmg', 'file', 'ftp', 
>> 'ftps',
>> +            'gluster', 'host_cdrom', 'host_device', 'http', 'https', 
>> 'iscsi',
>> +            'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 
>> 'parallels',
>> +            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
>>                { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' 
>> },
>>                'sheepdog',
>>                'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 
>> 'vxhs' ] }
>> @@ -4044,6 +4045,7 @@
>>          'blkreplay':  'BlockdevOptionsBlkreplay',
>>          'bochs':      'BlockdevOptionsGenericFormat',
>>          'cloop':      'BlockdevOptionsGenericFormat',
>> +      'compress':   'BlockdevOptionsGenericFormat',
>>          'copy-on-read':'BlockdevOptionsGenericFormat',
>>          'dmg':        'BlockdevOptionsGenericFormat',
>>          'file':       'BlockdevOptionsFile',
>>
> 
> 

-- 
With the best regards,
Andrey Shinkevich