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

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

[Robin Dong]

From: Robin Dong <sanbai@taobao.com>

Currently, if the IO is throttled by io-throttle, the SA has no idea of
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
named blkio.throttle.io_queued which indicates how many IOs are
currently throttled.

The nr_queued[] of struct throtl_grp is of type "unsigned int" and updates
to it are atomic both at 32bit and 64bit platforms, so we could just
read tg->nr_queued only under blkcg->lock.

Changelog from v2:
	Use nr-queued[] of struct throtl_grp for stats instaed of adding new blkg_rwstat.

Cc: Tejun Heo <tj@kernel.org>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
Signed-off-by: Robin Dong <sanbai@taobao.com>
---
 block/blk-throttle.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index a9664fa..e410448 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -953,6 +953,32 @@ static u64 tg_prfill_cpu_rwstat(struct seq_file *sf,
 	return __blkg_prfill_rwstat(sf, pd, &rwstat);
 }
 
+static u64 tg_prfill_io_queued(struct seq_file *sf,
+				struct blkg_policy_data *pd, int off)
+{
+	static const char *rwstr[] = {
+		[READ]	= "Read",
+		[WRITE]	= "Write",
+	};
+	struct throtl_grp *tg = pd_to_tg(pd);
+	const char *dname = NULL;
+	unsigned int v;
+	int i;
+
+	if (pd->blkg->q->backing_dev_info.dev)
+		dname = dev_name(pd->blkg->q->backing_dev_info.dev);
+
+	if (!dname)
+		return 0;
+
+	for (i = 0; i <= WRITE; i++)
+		seq_printf(sf, "%s %s %u\n", dname, rwstr[i], tg->nr_queued[i]);
+
+	v = tg->nr_queued[READ] + tg->nr_queued[WRITE];
+	seq_printf(sf, "%s Total %u\n", dname, v);
+	return v;
+}
+
 static int tg_print_cpu_rwstat(struct cgroup *cgrp, struct cftype *cft,
 			       struct seq_file *sf)
 {
@@ -963,6 +989,16 @@ static int tg_print_cpu_rwstat(struct cgroup *cgrp, struct cftype *cft,
 	return 0;
 }
 
+static int tg_print_io_queued(struct cgroup *cgrp, struct cftype *cft,
+			       struct seq_file *sf)
+{
+	struct blkcg *blkcg = cgroup_to_blkcg(cgrp);
+
+	blkcg_print_blkgs(sf, blkcg, tg_prfill_io_queued, &blkcg_policy_throtl,
+			  cft->private, true);
+	return 0;
+}
+
 static u64 tg_prfill_conf_u64(struct seq_file *sf, struct blkg_policy_data *pd,
 			      int off)
 {
@@ -1085,6 +1121,10 @@ static struct cftype throtl_files[] = {
 		.private = offsetof(struct tg_stats_cpu, serviced),
 		.read_seq_string = tg_print_cpu_rwstat,
 	},
+	{
+		.name = "throttle.io_queued",
+		.read_seq_string = tg_print_io_queued,
+	},
 	{ }	/* terminate */
 };
 
-- 
1.7.1

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

[Vivek Goyal]

On Tue, Oct 09, 2012 at 02:53:45PM +0800, Robin Dong wrote:
> From: Robin Dong <sanbai@taobao.com>
> 
> Currently, if the IO is throttled by io-throttle, the SA has no idea of
> 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
> named blkio.throttle.io_queued which indicates how many IOs are
> currently throttled.
> 
> The nr_queued[] of struct throtl_grp is of type "unsigned int" and updates
> to it are atomic both at 32bit and 64bit platforms, so we could just
> read tg->nr_queued only under blkcg->lock.
> 
> Changelog from v2:
> 	Use nr-queued[] of struct throtl_grp for stats instaed of adding new blkg_rwstat.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> Signed-off-by: Robin Dong <sanbai@taobao.com>
> ---
>  block/blk-throttle.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index a9664fa..e410448 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -953,6 +953,32 @@ static u64 tg_prfill_cpu_rwstat(struct seq_file *sf,
>  	return __blkg_prfill_rwstat(sf, pd, &rwstat);
>  }
>  
> +static u64 tg_prfill_io_queued(struct seq_file *sf,
> +				struct blkg_policy_data *pd, int off)
> +{
> +	static const char *rwstr[] = {
> +		[READ]	= "Read",
> +		[WRITE]	= "Write",
> +	};
> +	struct throtl_grp *tg = pd_to_tg(pd);
> +	const char *dname = NULL;
> +	unsigned int v;
> +	int i;
> +
> +	if (pd->blkg->q->backing_dev_info.dev)
> +		dname = dev_name(pd->blkg->q->backing_dev_info.dev);
> +
> +	if (!dname)
> +		return 0;
> +
> +	for (i = 0; i <= WRITE; i++)
> +		seq_printf(sf, "%s %s %u\n", dname, rwstr[i], tg->nr_queued[i]);

You are printing only READ/WRITE stats and not the SYNC/ASYNC stats. This
is not inline with rest of the stats like throttle.io_serviced and
throttle.io_service_bytes.

I guess it is better to maintain rwstat for io_queued and display it
according to io_serviced.

Thanks
Vivek

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

[Tejun Heo]

Hello, Robin.

On Tue, Oct 09, 2012 at 02:53:45PM +0800, Robin Dong wrote:
> Currently, if the IO is throttled by io-throttle, the SA has no idea of
> the situation and can't report it to the real application user about

Please don't use "SA" in the commit message.  Just write it out as
"system admin" or "userspace" or whatever.  It's only gonna confuse
people trying to understand the commit later on.

> that he/she has to do something. So this patch adds a new interface
> named blkio.throttle.io_queued which indicates how many IOs are
> currently throttled.
> 
> The nr_queued[] of struct throtl_grp is of type "unsigned int" and updates
> to it are atomic both at 32bit and 64bit platforms, so we could just
> read tg->nr_queued only under blkcg->lock.
> 
> Changelog from v2:
> 	Use nr-queued[] of struct throtl_grp for stats instaed of adding new blkg_rwstat.

As I wrote last time, I would prefer exposing the total number queued
to blk-throttle rather than exposing the number of bios being
currently held and let userland calculate from the difference from
throttle.io_serviced.  That is simpler and more inline with all other
stats.

Thanks.

-- 
tejun

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

[Vivek Goyal]

On Tue, Oct 16, 2012 at 04:27:06PM -0700, Tejun Heo wrote:

[..]
> > Changelog from v2:
> > 	Use nr-queued[] of struct throtl_grp for stats instaed of adding new blkg_rwstat.
> 
> As I wrote last time, I would prefer exposing the total number queued
> to blk-throttle rather than exposing the number of bios being
> currently held and let userland calculate from the difference from
> throttle.io_serviced.  That is simpler and more inline with all other
> stats.

Hi Tejun,

Can you explain a bit more. Whe do you mean by "total number queued". I
think throttle.io_queued will total number of bios queued in the cgroup
at the time of query.

Thanks
Vivek

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

[Tejun Heo]

Hello, Vivek.

On Wed, Oct 17, 2012 at 09:49:45AM -0400, Vivek Goyal wrote:
> Can you explain a bit more. Whe do you mean by "total number queued". I
> think throttle.io_queued will total number of bios queued in the cgroup
> at the time of query.

Instead of exposing the number that blk-throttle currently has
deferred, we can expose the number of bios that have been sent to
blk-throttle and the number of bios which left blk-throttle, both
monotically increasing and the difference indicating the number being
deferred.  That way we can stick to the usual stats facility.

Thanks.

-- 
tejun

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

[Vivek Goyal]

On Thu, Oct 18, 2012 at 04:24:04PM -0700, Tejun Heo wrote:
> Hello, Vivek.
> 
> On Wed, Oct 17, 2012 at 09:49:45AM -0400, Vivek Goyal wrote:
> > Can you explain a bit more. Whe do you mean by "total number queued". I
> > think throttle.io_queued will total number of bios queued in the cgroup
> > at the time of query.
> 
> Instead of exposing the number that blk-throttle currently has
> deferred, we can expose the number of bios that have been sent to
> blk-throttle and the number of bios which left blk-throttle, both
> monotically increasing and the difference indicating the number being
> deferred.

Ok, I see it now. So we currently already maintain the number of IOs
dispatched from blk-throttle  in throttle.io_serviced. Now you are 
suggesting that maintain another counter which keeps track of total
number IOs submitted to blk-throttle, say throttle.io_submitted? I think
using throttle.io_queued will be little confusing because in CFQ we
already use blkio.io_queued to represent number of IOs currently queued
and it is not monotonically increasing value.

> That way we can stick to the usual stats facility.

So how does this help? Because it is a monotonically increasing value
we can use per cpu stats without extra locking? Or somthing else?

Thanks
Vivek

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

[Tejun Heo]

Hello, Vivek.

On Fri, Oct 19, 2012 at 11:00:11AM -0400, Vivek Goyal wrote:
> > That way we can stick to the usual stats facility.
> 
> So how does this help? Because it is a monotonically increasing value
> we can use per cpu stats without extra locking? Or somthing else?

It's generally much simpler to expose dumb increasing counters.  You
don't have to worry about mismatching decrements (for whatever reason,
percpu or segmented counters), so unless there's a pretty good reason
to deviate, why deviate?

Thanks.

-- 
tejun

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

[Vivek Goyal]

On Fri, Oct 19, 2012 at 12:36:00PM -0700, Tejun Heo wrote:
> Hello, Vivek.
> 
> On Fri, Oct 19, 2012 at 11:00:11AM -0400, Vivek Goyal wrote:
> > > That way we can stick to the usual stats facility.
> > 
> > So how does this help? Because it is a monotonically increasing value
> > we can use per cpu stats without extra locking? Or somthing else?
> 
> It's generally much simpler to expose dumb increasing counters.  You
> don't have to worry about mismatching decrements (for whatever reason,
> percpu or segmented counters), so unless there's a pretty good reason
> to deviate, why deviate?

I really can't think of a reason to deviate. I was just curious of
advantages. I think it does make sense to not get into decrement business
and let user space subtract two values and figure out how much IO from
the group is throttled.

Thanks
Vivek