Am 18.09.2015 um 15:31 hat Alberto Garcia geschrieben:
> On Fri 18 Sep 2015 02:24:21 PM CEST, Kevin Wolf wrote:
>
> >> > +static void swap_feature_fields(BlockDriverState *bs_top,
> >> > + BlockDriverState *bs_new)
> >> > +{
> >> > + BlockDriverState tmp;
> >> > +
> >> > + bdrv_move_feature_fields(&tmp, bs_top);
> >> > + bdrv_move_feature_fields(bs_top, bs_new);
> >> > + bdrv_move_feature_fields(bs_new, &tmp);
> >> > +
> >> > + assert(!bs_new->io_limits_enabled);
> >> > + if (bs_top->io_limits_enabled) {
> >> > + bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
> >> > + bdrv_io_limits_disable(bs_top);
> >> > + }
> >> > +}
> >>
> >> I would use 'if (bs_top->throttle_state != NULL)' instead. That pointer
> >> is set if and only if the BDS has I/O limits set.
> >>
> >> bs->io_limits_enabled can be set to false in order to bypass the limits
> >> temporarily without removing the BDS's throttling settings (e.g in
> >> bdrv_read_unthrottled()).
> >>
> >> I actually think that we can get rid of io_limits_enabled in several
> >> places (if not completely), but I will take care of that in a separate
> >> patch.
> >>
> >> The only scenario that could cause problems is if bs->throttle_state is
> >> set but bs->io_limits_enabled is false when swap_feature_fields() is
> >> called, but I don't think that's possible in this case.
> >
> > So something like this? (Specifically, is the assertion right?)
> >
> > assert(!bs_new->throttle_state);
> > if (bs_top->throttle_state) {
> > assert(bs_top->io_limits_enabled);
> > bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
> > bdrv_io_limits_disable(bs_top);
> > }
>
> Yes, I think that's fine.
>
> > If the assertion doesn't hold true, I guess we would need to call
> > throttle_group_register() directly for the cases where
> > io_limits_enabled wasn't true.
>
> If io_limits_enabled is not true but throttle_state is set it means that
> there's an ongoing request that needs to bypass the I/O limits.
>
> There's two places where that can happen:
>
> 1) bdrv_start_throttled_reqs(), that's either a result of
> bdrv_io_limits_disable() or bdrv_flush_io_queue()
> (i.e. bdrv_drain()).
>
> 2) blk_read_unthrottled() (that comes from hd_geometry_guess()).
>
> Is there any scenario where the feature fields can be swapped in the
> middle of one of these two operations? I understand that the BDS must be
> drained first, that's why I don't think there's any risk.
Yes, I'm asserting !bdrv_requests_pending() for both BDSes, so that
should be fine. (But I neglect to actually drain requests in mirror.c.
That's probably a bug.)
Kevin
