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

[Alberto Garcia]

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.

Berto