On Thu 17 Sep 2015 03:48:17 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. Berto