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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

14.11.2019 14:27, Max Reitz wrote:
> On 13.11.19 19:43, 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 <mreitz@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/Makefile.objs     |   1 +
>>   block/filter-compress.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi/block-core.json    |  10 ++-
>>   3 files changed, 208 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..64b1ee5
>> --- /dev/null
>> +++ b/block/filter-compress.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Compress filter block driver
>> + *
>> + * Copyright (c) 2019 Virtuozzo International GmbH
>> + *
>> + * Author:
>> + *   Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> + *   (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"
>> +
>> +
>> +static int compress_open(BlockDriverState *bs, QDict *options, int flags,
>> +                         Error **errp)
>> +{
>> +    bs->backing = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
>> +                                  errp);
> 
> Please don’t attach something that the QAPI schema calls “file” as
> bs->backing.


Agree, it's a mistake. If we want backing and user set backing in options, it's opened automatically, I think..

> 
> Yes, attaching it as bs->file would break backing chains.  That’s a bug
> in the block layer.  I’ve been working on a fix for a long time.
> 
> Please don’t introduce more weirdness just because we have a bug in the
> block layer.
> 
> (Note that I’d strongly oppose calling the child “backing” in the QAPI
> schema, as this would go against what all other user-creatable filters do.)
> 

So, are you opposite to correct backing-based user-creatable filter (with backing both
in QAPI and code)?

Do you think, that if we make backup-top to be user-creatable, we should move it to be
file-child-based, or support both backing and file child?


-- 
Best regards,
Vladimir