[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3] throttle-groups: cancel timers on restart
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [PATCH v3] throttle-groups: cancel timers on restart |
Date: |
Thu, 28 Sep 2017 23:36:26 +0200 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Mon 25 Sep 2017 03:57:35 PM CEST, Stefan Hajnoczi <address@hidden> 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