Re: [PATCH V2] block/throttle: Add IO throttled information in blkio.throttle.

[Tejun Heo <tj@kernel.org>]

On Fri, Aug 31, 2012 at 01:15:09PM +0800, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> Currently, if the IO is throttled by io-throttle, the SA has no idea of

What's SA?

> the situation and can't report it to the real application user about
> that he/she has to do something. So this patch adds a new interface

Why does the application user "has to" do something?  There's nothing
the upper layer "must" do.  I'm not necessarily objecting to adding
the stat but the description seems a bit misleading.

> named blkio.throttle.io_queued which indicates how many IOs are
> currently throttled.

Also, the suggested stat is rather lacking for such purposes.  There's
no way other than keeping polling to find out the condition, which is
rather sad.  What's the actual use case here?

> Also another function blkg_rwstat_dec is added since the number of throttled
> IOs can be either added or decreased.

Maybe just make blkg_rwstat_add() to take int64_t instead of uint64_t?

> +static void throtl_update_queued_stats(struct throtl_grp *tg, int rw, int add)
> +{
> +	struct tg_stats_cpu *stats_cpu;
> +	unsigned long flags;
> +
> +	/* If per cpu stats are not allocated yet, don't do any accounting. */
> +	if (tg->stats_cpu == NULL)
> +		return;
> +
> +	/*
> +	 * Disabling interrupts to provide mutual exclusion between two
> +	 * writes on same cpu. It probably is not needed for 64bit. Not
> +	 * optimizing that case yet.
> +	 */
> +	local_irq_save(flags);
> +
> +	stats_cpu = this_cpu_ptr(tg->stats_cpu);
> +	if (add)
> +		blkg_rwstat_add(&stats_cpu->io_queued, rw, 1);
> +	else
> +		blkg_rwstat_dec(&stats_cpu->io_queued, rw, 1);
> +
> +	local_irq_restore(flags);

Adding throttle.io_queued could be a bit more consistent?

Thanks.

-- 
tejun