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 <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?
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
