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
