Re: [PATCH] blk-throttle: fix infinite throttling caused by non-cascading timer wheel

From: Vivek Goyal
Date: Mon Sep 19 2016 - 17:06:55 EST

On Tue, Sep 13, 2016 at 09:46:46AM -0400, Vivek Goyal wrote:
> Hi Hou Tao,
> [ CC Tejun and Thomas ]
> Thanks for the patch. I can reproduce it. I am wondering that why are you
> doing so many checks. Can't we just check if throttle group is empty or
> not. If it is empty and slice has expired, then start a new slice. If
> throttle group is not empty, then we know slice has to be an active slice
> and should be extended (despite the fact that it might have expired
> because timer function got called later than we expected it to be).
> Can you please try following patch. It seems to resolve the issue for me.
> Vivek

Hi Jens,

Can you please pick this patch. It seems to fix the reported issued.
Please let me know if you prefer a separate posting.


> Subject: blk-throttle: Extend slice if throttle group is not empty
> Right now, if slice is expired, we start a new slice. If a bio is
> queued, we keep on extending slice by throtle_slice interval (100ms).
> This worked well as long as pending timer function got executed with-in
> few milli seconds of scheduled time. But looks like with recent changes
> in timer subsystem, slack can be much longer depending on the expiry time
> of the scheduled timer.
> commit 500462a9de65 ("timers: Switch to a non-cascading wheel")
> This means, by the time timer function gets executed, it is possible the
> delay from scheduled time is more than 100ms. That means current code
> will conclude that existing slice has expired and a new one needs to
> be started. New slice will be 100ms by default and that will not be
> sufficient to meet rate requirement of group given the bio size and
> bio will not be dispatched and we will start a new timer function to
> wait. And when that timer expires, same process will repeat and we
> will wait again and this can easily be an infinite loop.
> Solve this issue by starting a new slice only if throttle gropup is
> empty. If it is not empty, that means there should be an active slice
> going on. Ideally it should not be expired but given the slack, it is
> possible that it has expired.
> Reported-by: Hou Tao <houtao1@xxxxxxxxxx>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> block/blk-throttle.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> Index: rhvgoyal-linux/block/blk-throttle.c
> ===================================================================
> --- rhvgoyal-linux.orig/block/blk-throttle.c 2016-09-13 08:55:33.616200176 -0400
> +++ rhvgoyal-linux/block/blk-throttle.c 2016-09-13 09:17:10.664200176 -0400
> @@ -780,9 +780,11 @@ static bool tg_may_dispatch(struct throt
> /*
> * If previous slice expired, start a new one otherwise renew/extend
> * existing slice to make sure it is at least throtl_slice interval
> - * long since now.
> + * long since now. New slice is started only for empty throttle group.
> + * If there is queued bio, that means there should be an active
> + * slice and it should be extended instead.
> */
> - if (throtl_slice_used(tg, rw))
> + if (throtl_slice_used(tg, rw) && !(tg->service_queue.nr_queued[rw]))
> throtl_start_new_slice(tg, rw);
> else {
> if (time_before(tg->slice_end[rw], jiffies + throtl_slice))