QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v6 04/42] block: Add child access functions
Date: Tue, 10 Sep 2019 11:14:44 +0200
Message-ID: <89984d52-881f-a1e4-c14a-2bf05fc789d0@redhat.com> (raw)
In-Reply-To: <20190909161314.GJ17606@localhost.localdomain>

[-- Attachment #1.1: Type: text/plain, Size: 9972 bytes --]

On 09.09.19 18:13, Kevin Wolf wrote:
> Am 09.09.2019 um 16:04 hat Max Reitz geschrieben:
>> On 09.09.19 11:36, Kevin Wolf wrote:
>>> Am 09.09.2019 um 09:56 hat Max Reitz geschrieben:
>>>> On 04.09.19 18:16, Kevin Wolf wrote:
>>>>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>>>>>> There are BDS children that the general block layer code can access,
>>>>>> namely bs->file and bs->backing.  Since the introduction of filters and
>>>>>> external data files, their meaning is not quite clear.  bs->backing can
>>>>>> be a COW source, or it can be an R/W-filtered child; bs->file can be an
>>>>>> R/W-filtered child, it can be data and metadata storage, or it can be
>>>>>> just metadata storage.
>>>>>>
>>>>>> This overloading really is not helpful.  This patch adds function that
>>>>>> retrieve the correct child for each exact purpose.  Later patches in
>>>>>> this series will make use of them.  Doing so will allow us to handle
>>>>>> filter nodes and external data files in a meaningful way.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> Each time I look at this patch, I'm confused by the function names.
>>>>> Maybe I should just ask what the idea there was, or more specifically:
>>>>> What does the "filtered" in "filtered child" really mean?
>>>>>
>>>>> Apparently any child of a filter node is "filtered" (which makes sense),
>>>>
>>>> It isn’t, filters can have non-filter children.  For example, backup-top
>>>> could have the source as a filtered child and the target as a non-filter
>>>> child.
>>>
>>> Hm, okay, makes sense. I had a definition in mind that says that filter
>>> nodes only have a single child node. Is it that a filter may have only a
>>> single _filtered_ child node?
>>
>> Well, there’s Quorum...
> 
> Ah, nice, quorum sets is_filter = true even though it neither fulfulls
> the conditions for it before this series, nor the changed conditions
> after this series.
> 
> So either quorum lies and isn't actually a filter driver, or our
> definition in the documentation of is_filter is wrong.

You could say it lies because in FIFO mode it clearly isn’t a filter for
all of its children.

There is a reason for lying, though, which is
bdrv_recurse_is_first_non_filter(), which is necessary to use the whole
to_replace mirror stuff.

(You mirror from a quorum with a failed child and then replace the
failed child.  mirror needs to ensure that there are only R/W filters
between the child and the mirror source so that replacing it will not
suddenly change any visible data.  Which is actually a lie for quorum,
because the child is clearly broken and thus precisely doesn’t show the
same data...)

Maybe we should stop declaring Quorum a filter and then rename the
bdrv_recurse_is_first_non_filter() to, I don’t know,
bdrv_recurse_can_be_replaced_by_mirror()?

>>>>> but also bs->backing of a qcow2 image, while bs->file of qcow2 isn't.
>>>>> raw doesn't have any "filtered" child. What's the system behind this?
>>>>
>>>> “filtered” means: If the parent node returns data from this child, it
>>>> won’t modify it, neither its content nor its position.  COW and R/W
>>>> filters differ in how they handle writes; R/W filters pass them through
>>>> to the filtered child, COW filters copy them off to some other child
>>>> node (and then the filtered child’s data will no longer be visible at
>>>> that location).
>>>
>>> But there is no reason why a node couldn't fulfill this condition for
>>> more than one child node. bdrv_filtered_child() isn't well-defined then.
>>> Technically, the description "Return any filtered child" is correct
>>> because "any" can be interpreted as "an arbitrary", but obviously that
>>> makes the function useless.
>>
>> Which is why it currently returns NULL for Quorum.
> 
> Which is about the only possible choice that breaks the contract...
> 
>  * Return any filtered child, independently of how it reacts to write

I don’t know if you’re serious about this proposition, because I don’t
know whether that could be useful in any way. :-?

>  * accesses and whether data is copied onto this BDS through COR.

I meant the contract as “Return the single filtered child there is, or NULL”

> Maybe the documentation of bdrv_filtered_child() needs to be rephrased?
> 
> Going back to qcow2, it's really not much different as it has multiple
> (two) filtered children, too.

Well, it doesn’t.  It isn’t an R/W filter.

Maybe what we actually need to rephrase is the definition of .is_filter.
 (Namely something along the lines of “Fulfills these guarantees (same
data, etc. pp.), *and* should be skipped for allocation information
queries etc.”.

> So if quorum returns NULL to mean "no
> unambiguous result", why does it return bs->backing instead of NULL for
> a qcow2 node?
> 
> (Yes, I know, because it's useful. But I'm trying to get some basic
> consistency into these interfaces.)

Not precisely because it’s useful, but because qcow2 does not have
.is_filter set.  :-)
(And it doesn’t have it set because that wouldn’t be useful.)

>>> Specficially, according to your definition, qcow2 filters both the
>>> backing file (COW filter) and the external data file (R/W filter).
>>
>> Not wrong.  But the same question as for raw arises: Is there any use to
>> declaring qcow2 an R/W filter driver just because it fits the definition?
> 
> Wait, where is there even a place where this could be declared?
> 
> The once thing I see that a driver even can declare is drv->is_filter,
> which is about the whole driver and not about nodes. It is false for
> qcow2.

That’s correct.  But that’s not a fundamental problem, of course, we
could make it a per-BDS attribute if that made sense.

> Then you made some criteria above that tell us whether a specific child
> of a node is a filtered child or not. As it happens, qcow2 (which is not
> a filter driver) can have two children that match the criteria for being
> filtered children.

But just arguing that I’m incapable of giving a good definition won’t
bring us along.

> I already think this is a bit inconsistent, because why should a driver
> that declares itself a non-filter be considered to filter children?

.is_filter is for R/W filters.  COW filters have .supports_backing for that.

> Okay, you say a broader definition of a filtered child is useful because
> you can then include all BdrvChild links in a backing/filter chain. Fair
> enough, it's not intuitive, but use a broader definition then.
> 
> But the point where you say that even though two of the children
> are filtered children under your broader definition, for the purpose of
> the API only one of them should be considered because the other one
> isn't that useful, that's really one inconsistency too much for me. You
> can't use a broad definition and then arbitrarily restrict the
> definition again so that it matches the special case you're currently
> interested in.
> 
> Either use a narrow definition, or use a broad one. But use only one and
> use it consistently.

I think the problem appears because you restrict the process to a single
step where there’s actually two.

Drivers can be either
(1) R/W filters (e.g. throttle)
(2) COW filters (e.g. qcow2)
(3) None of the above (e.g. vhdx, curl)

This choice is made on the driver level, not on the node level (for good
reason, see below*).

And then we derive the node’s filtered children from what the driver is.
 If it’s an R/W filter, bdrv_filtered_child() will return the R/W
filtered child.  If it’s a COW filter, bdrv_filtered_child() will return
the potentially existing COW backing child (or NULL, if there is no
backing child).


*
What is clear to me is that it isn’t useful to treat nodes of a specific
driver sometimes as R/W filter nodes and sometimes not.  R/W filter
nodes are handled differently from other nodes, and it would be
confusing if a certain driver sometimes behaves this and sometimes that
way.  (For example, if you put a raw node on top of a qcow2 node,
sometimes it would stop the backing chain, sometimes it wouldn’t.  That
makes no sense to me.)

OTOH, for COW filters, we do exactly that.  Sometimes they have a
backing file, sometimes they don’t.  That’s completely fine because
their overall behavior doesn’t change.


That makes me agree that there is indeed too much of a difference
between R/W filters and COW filters to lump them together under the
“filter” label.

[...]

> My point is not about changing the logic in the code, but about using
> names that actually describe accurately what the logic does.

Again, naming things is hard.

[...]

>> You disagreeing with me on these terms to me shows that there is a need
>> to formalize.  This is precisely what I want to do in this series.
>>
>> The fact that we don’t use the term “filter chain” so far is the reason
>> why I introduce it.  Because it comes as a clean slate.  “backing chain”
>> already means something to me, and it means something different.
> 
> Well, if "backing chain" is too narrow, "filter chain" is both too
> unspecific and inconsistent with the various definitions of "filter" and
> "filtered" we're using, and we can't think of anything more concise, we
> might have to use names that just mention both.
> 
> bdrv_cow_child() // don't call COW a filter, because .is_filter = false
> bdrv_filter_child() // your R/W filter, only for .is_filter = true nodes
> bdrv_filter_or_cow_child()
> 
> Or something like that. This would bring some more consistency into the
> way we use the words filter/filtered at least.

I’ll see how that looks overall, but why not.  Sounds good to me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 132+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 16:13 [Qemu-devel] [PATCH v6 00/42] block: Deal with filters Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 01/42] block: Mark commit and mirror as filter drivers Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 02/42] copy-on-read: Support compressed writes Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 03/42] throttle: " Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 04/42] block: Add child access functions Max Reitz
2019-08-09 16:56   ` Eric Blake
2019-09-04 16:16   ` Kevin Wolf
2019-09-09  7:56     ` Max Reitz
2019-09-09  9:36       ` Kevin Wolf
2019-09-09 14:04         ` Max Reitz
2019-09-09 16:13           ` Kevin Wolf
2019-09-10  9:14             ` Max Reitz [this message]
2019-09-10 10:47               ` Kevin Wolf
2019-09-10 11:36                 ` Max Reitz
2019-09-10 12:48                   ` Kevin Wolf
2019-09-10 12:59                     ` Max Reitz
2019-09-10 13:10                       ` Kevin Wolf
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 05/42] block: Add chain helper functions Max Reitz
2019-08-09 17:01   ` Eric Blake
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 06/42] qcow2: Implement .bdrv_storage_child() Max Reitz
2019-08-09 17:07   ` Eric Blake
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 07/42] block: *filtered_cow_child() for *has_zero_init() Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 08/42] block: bdrv_set_backing_hd() is about bs->backing Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 09/42] block: Include filters when freezing backing chain Max Reitz
2019-08-10 13:32   ` Vladimir Sementsov-Ogievskiy
2019-08-12 12:56     ` Max Reitz
2019-09-05 13:05   ` Kevin Wolf
2019-09-09  8:02     ` Max Reitz
2019-09-09  9:40       ` Kevin Wolf
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 10/42] block: Drop bdrv_is_encrypted() Max Reitz
2019-08-10 13:42   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 11/42] block: Add bdrv_supports_compressed_writes() Max Reitz
2019-09-05 13:11   ` Kevin Wolf
2019-09-09  8:09     ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 12/42] block: Use bdrv_filtered_rw* where obvious Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 13/42] block: Use CAFs in block status functions Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 14/42] block: Use CAFs when working with backing chains Max Reitz
2019-08-10 15:19   ` Vladimir Sementsov-Ogievskiy
2019-09-05 14:05   ` Kevin Wolf
2019-09-09  8:25     ` Max Reitz
2019-09-09  9:55       ` Kevin Wolf
2019-09-09 14:08         ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 15/42] block: Re-evaluate backing file handling in reopen Max Reitz
2019-08-10 16:05   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 16/42] block: Flush all children in generic code Max Reitz
2019-08-10 15:36   ` Vladimir Sementsov-Ogievskiy
2019-08-12 12:58     ` Max Reitz
2019-09-05 16:24       ` Kevin Wolf
2019-09-09  8:31         ` Max Reitz
2019-09-09 10:01           ` Kevin Wolf
2019-09-09 14:15             ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 17/42] block: Use CAFs in bdrv_refresh_limits() Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 18/42] block: Use CAFs in bdrv_refresh_filename() Max Reitz
2019-08-10 16:22   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 19/42] block: Use CAF in bdrv_co_rw_vmstate() Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 20/42] block/snapshot: Fix fallback Max Reitz
2019-08-10 16:34   ` Vladimir Sementsov-Ogievskiy
2019-08-12 13:06     ` Max Reitz
2019-09-10 11:56   ` Kevin Wolf
2019-09-10 12:04     ` Max Reitz
2019-09-10 12:49       ` Kevin Wolf
2019-09-10 13:06         ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 21/42] block: Use CAFs for debug breakpoints Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 22/42] block: Fix bdrv_get_allocated_file_size's fallback Max Reitz
2019-08-10 16:41   ` Vladimir Sementsov-Ogievskiy
2019-08-12 13:09     ` Max Reitz
2019-08-12 17:14       ` Vladimir Sementsov-Ogievskiy
2019-08-12 19:15         ` Max Reitz
2019-09-10 14:52   ` Kevin Wolf
2019-09-11  6:20     ` Max Reitz
2019-09-11  6:55       ` Kevin Wolf
2019-09-11  7:37         ` Max Reitz
2019-09-11  8:27           ` Kevin Wolf
2019-09-11 10:00             ` Max Reitz
2019-09-11 10:31               ` Kevin Wolf
2019-09-11 11:00                 ` Max Reitz
2019-09-12 10:34                   ` Kevin Wolf
2019-11-14 13:11                   ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 23/42] blockdev: Use CAF in external_snapshot_prepare() Max Reitz
2019-09-10 15:02   ` Kevin Wolf
2019-09-11  6:21     ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 24/42] block: Use child access functions for QAPI queries Max Reitz
2019-08-10 16:57   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 25/42] mirror: Deal with filters Max Reitz
2019-08-12 11:09   ` Vladimir Sementsov-Ogievskiy
2019-08-12 13:26     ` Max Reitz
2019-08-14 15:17       ` Vladimir Sementsov-Ogievskiy
2019-08-31  9:57   ` Vladimir Sementsov-Ogievskiy
2019-09-02 14:35     ` Max Reitz
2019-09-03  8:32       ` Vladimir Sementsov-Ogievskiy
2019-09-09  7:41         ` Max Reitz
2019-09-13 12:55   ` Kevin Wolf
2019-09-16 10:26     ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 26/42] backup: " Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 27/42] commit: " Max Reitz
2019-08-31 10:44   ` Vladimir Sementsov-Ogievskiy
2019-09-02 14:55     ` Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 28/42] stream: " Max Reitz
2019-08-12 11:55   ` Vladimir Sementsov-Ogievskiy
2019-09-13 14:16   ` Kevin Wolf
2019-09-16  9:52     ` Max Reitz
2019-09-16 14:47       ` Kevin Wolf
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 29/42] nbd: Use CAF when looking for dirty bitmap Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 30/42] qemu-img: Use child access functions Max Reitz
2019-08-12 12:14   ` Vladimir Sementsov-Ogievskiy
2019-08-12 13:28     ` Max Reitz
2019-08-14 16:04   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 31/42] block: Drop backing_bs() Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 32/42] block: Make bdrv_get_cumulative_perm() public Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 33/42] blockdev: Fix active commit choice Max Reitz
2019-08-09 16:13 ` [Qemu-devel] [PATCH v6 34/42] block: Inline bdrv_co_block_status_from_*() Max Reitz
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 35/42] block: Fix check_to_replace_node() Max Reitz
2019-08-15 15:21   ` Vladimir Sementsov-Ogievskiy
2019-08-15 17:01     ` Max Reitz
2019-08-16 11:01       ` Vladimir Sementsov-Ogievskiy
2019-08-16 13:30         ` Max Reitz
2019-08-16 14:24           ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 36/42] iotests: Add tests for mirror @replaces loops Max Reitz
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 37/42] block: Leave BDS.backing_file constant Max Reitz
2019-08-16 16:16   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 38/42] iotests: Let complete_and_wait() work with commit Max Reitz
2019-08-23  5:59   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 39/42] iotests: Add filter commit test cases Max Reitz
2019-08-31 11:41   ` Vladimir Sementsov-Ogievskiy
2019-09-02 15:06     ` Max Reitz
2019-08-31 12:35   ` Vladimir Sementsov-Ogievskiy
2019-09-02 15:09     ` Max Reitz
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 40/42] iotests: Add filter mirror " Max Reitz
2019-08-31 12:35   ` Vladimir Sementsov-Ogievskiy
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 41/42] iotests: Add test for commit in sub directory Max Reitz
2019-08-09 16:14 ` [Qemu-devel] [PATCH v6 42/42] iotests: Test committing to overridden backing Max Reitz
2019-09-03  9:18   ` Vladimir Sementsov-Ogievskiy

Reply instructions:

You may reply publically 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=89984d52-881f-a1e4-c14a-2bf05fc789d0@redhat.com \
    --to=mreitz@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /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

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git