Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling

[Kevin Wolf]

Am 08.09.2017 um 17:44 hat Manos Pitsidianakis geschrieben:
> On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote:
> > We shouldn't really need any throttling code in
> > blk_root_drained_begin/end any more now because the throttle node will
> > be drained. If this code is necessary, a bdrv_drain() on an explicit
> > throttle node will work differently from one on an implicit one.
> > 
> > Unfortunately, this seems to be true about the throttle node. Implicit
> > throttle nodes will keep ignoring the throttle limit in order to
> > complete the drain request quickly, where as explicit throttle nodes
> > will process their requests at the configured speed before the drain
> > request can be completed.
> > 
> > This doesn't feel right to me, both should behave the same.
> > 
> > Kevin
> > 
> 
> I suppose we can implement bdrv_co_drain and increase io_limits_disabled
> from inside the driver. And then remove the implicit filter logic from
> blk_root_drained_begin. But there's no _end callback equivalent so we can't
> decrease io_limits_disabled at the end of the drain. So I think there are
> two options:
> 
> - make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all
> children to call it. Old behavior of I/O bursts (?) during a drain is  kept.

This is the solution I was thinking of. It was always odd to have a
drained_begin/end pair in the external interface and in BdrvChildRole,
but not in BlockDriver. So it was to be expected that we'd need this
sooner or later.

> - remove io_limits_disabled and let throttled requests obey limits  during a
> drain

This was discussed earlier (at least when the disable code was
introduced in BlockBackend, but I think actually more than once), and
even though everyone agreed that ignoring the limits is ugly, we
seem to have come to the conclusion that it's the least bad option.
blk_drain() blocks and makes everything else hang, so we don't want it
to wait for several seconds.

Kevin

signature.asc
Description: PGP signature