Re: [Qemu-block] [PATCH 00/12] block: Move I/O throttling to BlockBackend

[Kevin Wolf]

[ Cc: Stefan ]

Am 23.03.2016 um 10:28 hat Paolo Bonzini geschrieben:
> On 23/03/2016 10:03, Kevin Wolf wrote:
> > Am 22.03.2016 um 22:33 hat Paolo Bonzini geschrieben:
> >> On 22/03/2016 16:33, Kevin Wolf wrote:
> >>> This is another feature that was "logically" part of the BlockBackend, but
> >>> implemented as a BlockDriverState feature. It was always kept on top using
> >>> swap_feature_fields().
> >>>
> >>> This series moves it to be actually implemented in the BlockBackend, 
> >>> removing
> >>> another obstacle for removing bs->blk and allowing multiple BBs per BDS.
> >>>
> >>> Depends on 'block: Implement writethrough in BlockBackend'.
> >>
> >> This series would mess up my own I/O throttling cleanups that have been
> >> posted in February and have hardly seen a review for one month.
> > 
> > Which cleanups? The ones that you hid in an I/O path locking series?
> > Whose v2 didn't even include qemu-block? I noticed only now that they
> > exist.
> > 
> > I can take a look, but nobody told me (or Berto, for that matter) that
> > this is a series we should have a look at. block/io.c is maintained by
> > Stefan, throttling isn't.
> 
> If you actually look at the series, all of the changes are in block/io.c
> except for:
> 
> - moving some code from block/io.c to block/throttle-groups.c, with zero
> semantic change.
> 
> - adding exactly three lines of code to block/throttle-groups.c:
> 
>     if (bs->io_limits_disabled) {
>         return false;
>     }
> 
> I think it's okay to assume that the block/io.c maintainer can make a
> fair judgment on these changes.

I'm less worried about the actual code, but about the architectural
part. This example shows that dividing maintainership at source file
boundaries doesn't always make sense.

> On the other hand, this series touches block/io.c much more heavily,
> adding code to a function that we want to remove altogether
> (bdrv_flush_io_queue).  Yet, Stefan wasn't even CCed.  If you intend to
> override the block/io.c maintainer, you are _expected_ to take a look at
> pending block/io.c patches and see how they interact with yours.
> 
> Sure, v2 of my series was not CCed to qemu-block, but v1 was.  Even just
> reading the 00/16 part of the thread would have provided the status of
> the series without any doubt.  Again, I'd only expect you to take this
> additional burden because you didn't CC the submaintainer on this
> series.  The alternative is to do that, explain him why you are sending
> such an intrusive fix close to soft freeze, and letting him make a decision.

Yes, both of us messed up their CC list, thanks for pointing it out.
Adding Stefan to the thread now.

> >> I expect the rules for soft freeze to apply to maintainers as well.
> >> These patches and the removal of bs->blk are about one month late and
> >> shouldn't be included in 2.6.
> > 
> > This is not a feature. It's a series that brings actual behaviour in
> > line with the promised API, in other words a bug fix.
> > 
> > Admittedly, it's a very heavy fix, but if we decide that we can't do a
> > massive bug fix that late in the cycle (I admit that it got a bit late,
> > even though the initial patches were on time before the soft freeze), we
> > need to carefully check which other features we must revert in order to
> > keep the API changes in 2.7 minimal. At least, I think, we'd have to
> > disallow referencing backing files by node-name in 2.6.
> 
> Let me rephrase that.  There is a feature, "referencing backing files by
> node-name", which should have had patches on the list by the soft freeze
> date; you committed the API without having everything else posted or
> even written, and now everything else qualifies as bug fix.

More like the feature was merged without noticing that this has even
more implications that we saw. This was at the very beginning of the 2.6
cycle, posted during the freeze of 2.5 (don't remember whether soft or
hard). So at least we're lucky enough that we can still revert it if we
need. It had been almost a year before we decided that it can finally go
in, so I don't think I have to take blame for rushing things there.

The problem is that it allows you to create configurations where it
becomes visible that things are implemented different from the design
that we envision and for which the QMP APIs are tailored (essentially
implemented in BDS + bdrv_swap() instead of implemented in BB).

Features that are implemented in the BB can only ever be enabled on the
top level. As long as they are implemented in the BDS, however, you can
also enable them in intermediate layers, which will break as soon as we
implement the real thing. So in order to avoid API breakage we either
need to restrict user control over intermediate layers or we do the real
thing now.

User control over backing files is new in 2.6, so we can revert that if
we have to, but doing the real thing might be preferable.

User control over bs->file is older, though, and in principle affected
by the same problem. Of course, it's less likely that people actually
use the problematic features there and removing them now is already an
API change, so it's a less good reason for fixing things in 2.6 rathe
than 2.7.

> Since the damage is done, I guess you actually *should* go on and commit
> these patches during hard freeze.  But please give priority to patches
> that were sent according to the rules.

I'll have a look at your series and check how serious the conflicts are.
At the first sight I'm afraid it's not obvious and we need to discuss in
more detail how we can achieve both goals before any of the series can
be merged.

Kevin