Re: [Qemu-devel] [PATCH v6 04/42] block: Add child access functions

[Kevin Wolf <kwolf@redhat.com>]

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

Am 10.09.2019 um 13:36 hat Max Reitz geschrieben:
> On 10.09.19 12:47, Kevin Wolf wrote:
> > Am 10.09.2019 um 11:14 hat Max Reitz geschrieben:
> >> 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()?
> > 
> > Why not.
> 
> It feels difficult to do in this series because this is a whole new can
> of worms.
> 
> In patch 35, I actually replace the mirror use case by
> is_filtered_child().  So it looks to me as if that should not be done,
> because I should instead fix bdrv_recurse_is_first_non_filter() (and
> rename it), because quorum does allow replacing its children by mirror,
> even if it does not act as a filter for them.
> 
> OTOH, there are other users of bdrv_is_first_non_filter().  Those are
> qmp_block_resize() and external_snapshot_prepare(), who throw an error
> if that returns false.
> 
> I think that’s just wrong.  First of all, I don’t even know why we have
> that restriction anymore (I can imagine why it used to make sense before
> the permission system).  qmp_block_resize() should always work as long
> as it can get BLK_PERM_RESIZE; and I don’t know why the parents of some
> node would care if you take a snapshot of their child.

Hm, doesn't it make sense in a way for qmp_block_resize() at least? It
means that you can't resize just a filter, but you need to resize the
image that actually provides the data for the filter.

Of course, there is no reason for it to be the _first_ non-filter as
long as BLK_PERM_RESIZE is shared, but just some non-filter node.

Two more random observations:

* quorum uses bdrv_filter_default_perms(), which allows BLK_PERM_RESIZE.
  I think this is wrong and quorum should make sure that all children are
  always the same size because otherwise it can't tell what its own size
  is. (Or vote on size...? :-/) Probably not a problem in practice as
  long as we check bdrv_is_first_non_filter().

* child_file and child_backing don't implement .resize. So if you resize
  a non-top-level image, parents (in particular filters) don't get their
  size adjusted. This is probably a bug, too, but one that isn't
  prevented by bdrv_is_first_non_filter() and should be visible today.

> >>> 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.
> > 
> > What do I have to look at to see whether something is an R/W filter or
> > not? qcow2 matches your criteria for an R/W filter.
> 
> No.  Some qcow2 nodes match the criteria.  But not all, which makes the
> qcow2 driver not a filter driver.
> 
> > You say that it's not useful, so it's not an R/W filter anyway. But
> > where in the code could I get this information?
> 
> “Where in the code”?  Do you want to add a comment to every BlockDriver
> structure on why it does or doesn’t set .is_filter?

Never mind, I just didn't understand that .is_filter is the thing that
defines a R/W filter. In fact, I didn't really understand what
.is_filter was supposed to mean at all because I was so confused. For
some reason I was sure it had to mean any kind of filter, but that
assumption just didn't match up with its use at all.

> >>>>> 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.
> > 
> > I was thinking per-child, actually, because you declare one BdrvChild
> > filtered and another not filtered.
> 
> Why don’t you say so from the start then?

Yes, I wrote "nodes", thought "child nodes" and should have said
"children" because edges are not nodes. My bad, sorry.

> (Sorry, but honestly about 30 % of this discussion to me feels like
> you’re playing games with me.  Please don’t take this the wrong way, I
> mean it very neutrally.  It’s just that I feel like I’m explaining
> things to you that you very much know, but you just want me to say them.
>  And that feels unproductive and sometimes indeed frustrating.)

No, certainly not. If my mails seemed confusing or pointless, it just
shows how thoroughly confused I was.

> One thing is that this wouldn’t make the quorum case any easier because
> it actually doesn’t know for which children it acts as a filter and for
> which it doesn’t.
> 
> > But by now I think most of the confusion is really just a result of COW
> > being considered a filter in some respects (mainly just the names of the
> > child access functions), but not in others (like .is_filter).
> 
> I don’t quite see how it’s “by now” when in your first mail you already
> basically wrote that functionally, everything works (leaving out
> quorum), but that you’re confused (or claim to be confused, I have no
> idea what’s real and what’s pretended anymore) by the names.

Well, I saw that the special cases in the patches that I had reviewed so
far seemed to be converted correctly, but I just didn't understand the
whole concept behind it. It's possible to both understand that a
transformation is correct and to fail to grasp the concept behind it.

And your first answer only confused me more because you gave definitions
for R/W and COW filters that honestly ended up a bit misleading,
possibly as a result of your endeavour to make R/W filters and COW
sound like the same thing. (Which made me lose sight of basic facts like
that R/W filters must forward _every_ request without exception to their
filtered child even though COW doesn't.)

> We have come to two results, as far as I can see:
> 
> First, naming COW backing nodes “COW filtered children” clashes with our
> existing use of ”filter”.  There is no point in forcing the ”filter”
> label on everything.  We can just keep calling (R/W) filters filters and
> COW backing children COW children.  The names are succinct enough.
> 
> In some cases, we don’t care whether something is a COW or filtered
> child, in such a case a caller can be bothered to use the slightly
> longer bdrv_cow_or_filtered_child().

Aye.

> Second, most of the time we want a filter node to have a clear and
> unique path to go down.  This is the important property of filters: That
> you can skip them and go to the node that actually has the data.
> 
> Quorum breaks this by having multiple children, and nobody knows which
> of them has the data we will see on the next read operation.
> 
> All “filters” who could have multiple children would have this problem.
>  Hence a filter must always have a single unique data child.  I think.

I agree, and this is the condition that I mentioned somewhere above, but
failed to actually find guaranteed somewhere. We should probably make
this explicit.

Of course, quorum and similar things intend all their children to
provide the same data, but the whole point of the driver is that this is
not always guaranteed, so they aren't actually filters.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]