On Mon 25 Sep 2017 03:57:35 PM CEST, Stefan Hajnoczi <stefa...@redhat.com> wrote:
Hey Stefan, sorry for the late reply but I've been looking a this for a while trying to understand it. > v3: > * Different approach this time, based on Berto's insight that another > TGM may need to be scheduled now that our TGM's timers have been > detached. > > I realized the problem isn't the detach operation, it's the restart > operation that runs before detach. Timers shouldn't be left active > across restart. I'm not sure if I follow this paragraph, why is the problem in the restart operation? I may have overlooked something after all the recent changes in the throttling code, but the original point of restart was to ensure that -after e.g. destroying and creating a new set of timers for a drive- at least one throttled request would wake up so all I/O could continue normally. Restart itself doesn't care about timers in the ThrottleGroupMember, it just takes a coroutine from the queue and restarts it. If there would be already a timer set in the same tgm it wouldn't cause any trouble, after all the timer callback simply restarts the queue. > void throttle_group_restart_tgm(ThrottleGroupMember *tgm) > { > if (tgm->throttle_state) { > + ThrottleState *ts = tgm->throttle_state; > + ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); > + ThrottleTimers *tt = &tgm->throttle_timers; > + > + /* Cancel pending timers */ > + qemu_mutex_lock(&tg->lock); > + if (timer_pending(tt->timers[0])) { > + timer_del(tt->timers[0]); > + tg->any_timer_armed[0] = false; > + } > + if (timer_pending(tt->timers[1])) { > + timer_del(tt->timers[1]); > + tg->any_timer_armed[1] = false; > + } > + qemu_mutex_unlock(&tg->lock); > + > + /* This also schedules the next request in other TGMs, if necessary > */ > throttle_group_restart_queue(tgm, 0); > throttle_group_restart_queue(tgm, 1); > } I think if you cancel the timers _after_ throttle_group_restart_queue() it would work just the same, because as I said earlier that function doesn't touch tgm->throttle_timers. And if you cancel the timers after throttle_group_restart_queue() then you get something similar to what you had in patch v2 (minus the bdrv_drained_begin/end calls). Berto