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 <mre...@redhat.com> >>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@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.shinkev...@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)? >> >> 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? -- Best regards, Vladimir
