QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Max Reitz <mreitz@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: Mon, 9 Sep 2019 18:13:14 +0200
Message-ID: <20190909161314.GJ17606@localhost.localdomain> (raw)
In-Reply-To: <38c0ff7e-dfd3-189e-6026-3642d78e5029@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 10830 bytes --]

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.

> >>> 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
 * accesses and whether data is copied onto this BDS through COR.

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. 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.)

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

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.

I already think this is a bit inconsistent, because why should a driver
that declares itself a non-filter be considered to filter children?
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.

> >> The main reason behind the common “filtered” name is for the generic
> >> functions that work on both COW and true filter (R/W filters) chains.
> >> We need such functionality sometimes.  I personally felt like the
> >> concept of true (R/W) filters and COW children was similar enough to
> >> share a common name base.
> > 
> > We generally call this concept a "backing chain".
> 
> I suppose that’s an exclusive “we”?  Because I use ”backing chain” to
> refer to COW chains exclusively.
> 
> Such a chain may or may not include filters, but they are not really
> load-bearing nodes of the chain.  As such, I generally want to skip them
> when looking at a backing chain (hence e.g. bdrv_backing_chain_next()).
> 
> From what I can tell nobody has ever formalized any terms regarding COW
> backing chains or R/W filter chains.  This series is an attempt.

Well, as you can see, this attempt feels confusing to me.

I agree with your naming of bdrv_backing_chain_next(), it's clear enough
what path it will follow down the graph. I just disagree that "filter
chain" is a good term for something that prefers backing file links when
it has a choice.

> >> qcow2 has a COW child.  As such, it acts as a COW filter in the sense of
> >> the function names.
> >>
> >> raw has neither a COW child nor acts as an R/W filter.  As such, it has
> >> no filtered child.  My opinion on this hasn’t changed.
> >>
> >> (To reiterate, in practice I see no way anyone would ever use raw as an
> >> R/W filter.
> >> Either you use it without offset/size, in which case you simply use it
> >> in lieu of a format node, so you precisely don’t want it to act as a
> >> filter when it comes to allocation information and so on (even though it
> >> can be classified a filter here).
> >> Or you use it as kind of a filter with offset/size, but then it no
> >> longer is a filter.
> > 
> > Agreed with offset, but with only size, it matches your definition of a
> > filter.
> 
> So?
> 
> Should we treat it as a filter when @offset is 0 but otherwise not?
> That totally wouldn’t be confusing to users.

No, I'm just applying your definitions to see if the contradictions
between them and your explanations are of any importance. *shrug*

> >> Filters are defined by “Every filter must fulfill these conditions: ...”
> >> – not by “Everything that fulfills these conditions is a filter”.
> >> Marking a driver as a filter has consequences, and I don’t see why we
> >> would want those consequences for raw.)
> >>
> >>> It looks like bdrv_filtered_child() is the right function to iterate
> >>> along a backing file chain, but I just still fail to connect that and
> >>> the name of the function in a meaningful way.
> >>
> >> It‘s the right function to iterate along a filter chain.  This includes
> >> COW backing children and R/W filtered children.
> > 
> > qcow2 doesn't fulfill the conditions for begin a filter driver. Two of
> > its possible children fulfill the conditions for being a filtered child.
> > You can pick either approach, talking about a "filter chain" just
> > doesn't make sense there. Either the chain is broken by a non-filter
> > driver like qcow2, or it must become a filter tree.
> 
> I have no idea what your point is.  There is no point in making it a
> filter tree at this point, just as we never had a backing tree.
> 
> And the good example is Quorum.  qcow2 is a bad example because there is
> no benefit in marking it an R/W filter for its external data file,
> exactly like is the case for raw.

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

And as I said above, neither is "not useful" a convincing argument for
ignoring filtered children (as I think we're trying to build something
rather generic, not something that works only for what we consider
useful today) nor do I see how qcow2 could be marked or not marked an
R/W filter (as mentioned above).

> > What we're really interested in is iterating the backing chain even
> > across filter nodes, so your implementation achieves the right result.
> > It just feels completely arbitrary, counterintuitive and confusing to
> > call this a (or actually "the") "filter chain" and to pretend that the
> > name tells anyone what it really is.
> 
> So exactly the same as “bs->backing” or “backing chain” for me.
> 
> 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.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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 [this message]
2019-09-10  9:14             ` Max Reitz
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=20190909161314.GJ17606@localhost.localdomain \
    --to=kwolf@redhat.com \
    --cc=mreitz@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