Re: [Qemu-block] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Kevin Wolf]

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