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

[Max Reitz]

On 15.11.19 11:55, Vladimir Sementsov-Ogievskiy wrote:
> 15.11.2019 13:52, Vladimir Sementsov-Ogievskiy wrote:
>> 15.11.2019 12:32, Max Reitz wrote:
>>> On 14.11.19 12:59, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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 <address@hidden>
>>>>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>>>>> ---
>>>>>>    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 <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"
>>>>>> +
>>>>>> +
>>>>>> +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)?
>>>
>>> I’m not opposed to fixing it, but I don’t think the fix is to make all
>>> filters use bs->backing.
>>>
>>>> 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?
>>>
>>> I definitely don’t think it would be wrong.
>>>
>>> It depends on how difficult it is.  I’m currently working on (more
>>> groundwork for the filter series v7) a series to rework BdrvChildRole so
>>> we can see from it what a child is used for (data, metadata, filter,
>>> COW).  I can already see that it won‘t work out perfectly because
>>> whenever we attach "backing", the question is whether that’s a COW child
>>> now or whether it’s a filtered child.  I suppose I’m going to guess COW
>>> when there’s no way to get the information, and maybe sometimes be wrong.
>>>
>>> In my honest opinion, reusing bs->backing for filters was wrong.  I’m
>>> not saying that bs->file was any better.  But I have a bit of a gripe
>>> with filters using bs->backing, because it’s acknowledging a bug but not
>>> fixing it at the same time.  Had we fixed the bug when we first noticed
>>> it with the introduction of the mirror filter, maybe we wouldn’t be in
>>> this position now.  Or maybe we should have just added a bs->filtered link.
>>>
>>> But maybes aside, it still means that using bs->backing instead of
>>> bs->file is not really better.  Right now it’s both wrong, and we need
>>> to fix the block layer so it isn’t.
>>>
>>> So what to do for new filters?  Sure, bs->backing works around a bug
>>> now.  But it’ll be weird once the bug is fixed.  Then we’ll have filters
>>> that use @file and others will use @backing.  I don’t think we want
>>> that, I think we want a uniform interface for all filters.
>>>
>>> And yes, that implies we probably should change backup-top to use file
>>> instead of backing once it gets an external interface.
>>>
>>> (Compare
>>> https://lists.nongnu.org/archive/html/qemu-block/2017-09/msg00380.html
>>> )
>>>
>>> Max
>>>
>>
>> OK, got your point. Let's use file child in compress filter. Hope for your 
>> series!
>>
> 
> Interesting, how much of your series needed to make it possible to use 
> compress filter
> in stream? To make it work in 5.0?

I’ve never really considered the series as easily splittable, or I would
have done that so I wouldn’t have to rebase that monster every time.
But maybe I just didn’t take enough time to consider...

Max

signature.asc
Description: OpenPGP digital signature