From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>,
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
"armbru@redhat.com" <armbru@redhat.com>,
Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH v7 1/3] block: introduce compress filter driver
Date: Fri, 15 Nov 2019 10:52:13 +0000 [thread overview]
Message-ID: <36394cc3-9d68-5f56-7d44-ca56ff3cf7e3@virtuozzo.com> (raw)
In-Reply-To: <dcc46a1b-cd8a-78e3-6353-bf9fb2210d9b@redhat.com>
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 <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)?
>
> 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!
--
Best regards,
Vladimir
next prev parent reply other threads:[~2019-11-15 10:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-13 18:43 [PATCH v7 0/3] qcow2: advanced compression options Andrey Shinkevich
2019-11-13 18:43 ` [PATCH v7 1/3] block: introduce compress filter driver Andrey Shinkevich
2019-11-14 11:27 ` Max Reitz
2019-11-14 11:59 ` Vladimir Sementsov-Ogievskiy
2019-11-15 9:32 ` Max Reitz
2019-11-15 10:12 ` Andrey Shinkevich
2019-11-15 12:03 ` Max Reitz
2019-11-15 10:52 ` Vladimir Sementsov-Ogievskiy [this message]
2019-11-15 10:55 ` Vladimir Sementsov-Ogievskiy
2019-11-15 12:05 ` Max Reitz
2019-11-13 18:43 ` [PATCH v7 2/3] qcow2: Allow writing compressed data of multiple clusters Andrey Shinkevich
2019-11-13 18:43 ` [PATCH v7 3/3] tests/qemu-iotests: add case to write " Andrey Shinkevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=36394cc3-9d68-5f56-7d44-ca56ff3cf7e3@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=andrey.shinkevich@virtuozzo.com \
--cc=armbru@redhat.com \
--cc=den@virtuozzo.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).