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

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

[Tao Ma]

From: Tao Ma <boyu.mt@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.

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

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>
---
 block/blk-cgroup.h   |   26 ++++++++++++++++++++++++++
 block/blk-throttle.c |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 2459730..b1f6f5c 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -413,6 +413,32 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
 }
 
 /**
+ * blkg_rwstat_dec - dec a value to a blkg_rwstat
+ * @rwstat: target blkg_rwstat
+ * @rw: mask of REQ_{WRITE|SYNC}
+ * @val: value to dec
+ *
+ * Dec @val to @rwstat.  The counters are chosen according to @rw.  The
+ * caller is responsible for synchronizing calls to this function.
+ */
+static inline void blkg_rwstat_dec(struct blkg_rwstat *rwstat,
+				   int rw, uint64_t val)
+{
+	u64_stats_update_begin(&rwstat->syncp);
+
+	if (rw & REQ_WRITE)
+		rwstat->cnt[BLKG_RWSTAT_WRITE] -= val;
+	else
+		rwstat->cnt[BLKG_RWSTAT_READ] -= val;
+	if (rw & REQ_SYNC)
+		rwstat->cnt[BLKG_RWSTAT_SYNC] -= val;
+	else
+		rwstat->cnt[BLKG_RWSTAT_ASYNC] -= val;
+
+	u64_stats_update_end(&rwstat->syncp);
+}
+
+/**
  * blkg_rwstat_read - read the current values of a blkg_rwstat
  * @rwstat: blkg_rwstat to read
  *
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 1588c2d..9317d71 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -46,6 +46,8 @@ struct tg_stats_cpu {
 	struct blkg_rwstat		service_bytes;
 	/* total IOs serviced, post merge */
 	struct blkg_rwstat		serviced;
+	/* total IOs queued, not submitted to the underlying device. */
+	struct blkg_rwstat		io_queued;
 };
 
 struct throtl_grp {
@@ -267,6 +269,7 @@ static void throtl_pd_reset_stats(struct blkcg_gq *blkg)
 
 		blkg_rwstat_reset(&sc->service_bytes);
 		blkg_rwstat_reset(&sc->serviced);
+		blkg_rwstat_reset(&sc->io_queued);
 	}
 }
 
@@ -700,6 +703,31 @@ static void throtl_update_dispatch_stats(struct throtl_grp *tg, u64 bytes,
 	local_irq_restore(flags);
 }
 
+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);
+}
+
 static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
 {
 	bool rw = bio_data_dir(bio);
@@ -715,6 +743,8 @@ static void throtl_add_bio_tg(struct throtl_data *td, struct throtl_grp *tg,
 			struct bio *bio)
 {
 	bool rw = bio_data_dir(bio);
+	struct tg_stats_cpu *stats_cpu;
+	unsigned long flags;
 
 	bio_list_add(&tg->bio_lists[rw], bio);
 	/* Take a bio reference on tg */
@@ -722,6 +752,7 @@ static void throtl_add_bio_tg(struct throtl_data *td, struct throtl_grp *tg,
 	tg->nr_queued[rw]++;
 	td->nr_queued[rw]++;
 	throtl_enqueue_tg(td, tg);
+	throtl_update_queued_stats(tg, bio->bi_rw, 1);
 }
 
 static void tg_update_disptime(struct throtl_data *td, struct throtl_grp *tg)
@@ -762,6 +793,7 @@ static void tg_dispatch_one_bio(struct throtl_data *td, struct throtl_grp *tg,
 	bio->bi_rw |= REQ_THROTTLED;
 
 	throtl_trim_slice(td, tg, rw);
+	throtl_update_queued_stats(tg, bio->bi_rw, 0);
 }
 
 static int throtl_dispatch_tg(struct throtl_data *td, struct throtl_grp *tg,
@@ -1090,6 +1122,11 @@ static struct cftype throtl_files[] = {
 		.private = offsetof(struct tg_stats_cpu, serviced),
 		.read_seq_string = tg_print_cpu_rwstat,
 	},
+	{
+		.name = "throttle.io_queued",
+		.private = offsetof(struct tg_stats_cpu, io_queued),
+		.read_seq_string = tg_print_cpu_rwstat,
+	},
 	{ }	/* terminate */
 };
 
-- 
1.7.0.4

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

[Tejun Heo]

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

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

[Tao Ma]

Hi Tejun,
On 09/01/2012 09:05 AM, Tejun Heo wrote:
> 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?
system admin.
> 
>> 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?
Vivek and I have talked about its usage in my first try. See the thread
here. https://lkml.org/lkml/2012/5/22/81
And I am OK to say it again here. In our case, we use flashcache as a
block device and the bad thing is that flashcache is a bio-based dm
target and we can't use block io controller here to control the weight
of different cgroups. So io throttle is chosen. But as io throttle can
only set a hard upper limit for different instances, it makes the
control not flexible enough. Say with io controller, if there is no
requests form the cgroup with weight 1000, a cgroup with 500 can use the
whole bandwidth of the underlying device. But if we set 1000 iops for
cgroup A and 500 iops for cgroup B in io throttle, cgroup B can't exceed
its limit even if cgroup A has no request pending. So if we can export
the io_queued information out to the system admin, they can write some
daemon and in the above case, increase the upper limit of cgroup B to
some number say 1000. It helps us to utilize the device more
efficiently. Does it make sense to you?


> 
>> 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?
sure, will change it in the later version.
> 
>> +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?
sorry, I don't know what is your meaning here. You mean some codes like
	blkg_rwstat_add(&stats_cpu->throttle.io_queude, rw, 1)?

Thanks
Tao

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

[Vivek Goyal]

On Fri, Aug 31, 2012 at 01:15:09PM +0800, Tao Ma wrote:

[..]
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 1588c2d..9317d71 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -46,6 +46,8 @@ struct tg_stats_cpu {
>  	struct blkg_rwstat		service_bytes;
>  	/* total IOs serviced, post merge */
>  	struct blkg_rwstat		serviced;
> +	/* total IOs queued, not submitted to the underlying device. */
> +	struct blkg_rwstat		io_queued;
>  };

Couple of questions.

- blkg_rwstat is "unsigned" and io_queued can go negative too (Because
  throttled bio can very well be dispatched from other cpu from a worker
  thread). So is it a good idea to represent a negative number with 
  unsingned type?

- As this stat is per cpu, a reader might very well see negative (or a
  huge unsigned value) as number of io_queued. Not sure if that is acceptable.
  How would user space come to know whether it is a valid value or not. I
  thought per cpu stats are good for continuously increasing values but
  not necessarily for values which can increase as well as decrease.

Thanks
Vivek

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

[Tao Ma]

On 09/04/2012 09:35 PM, Vivek Goyal wrote:
> On Fri, Aug 31, 2012 at 01:15:09PM +0800, Tao Ma wrote:
> 
> [..]
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 1588c2d..9317d71 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -46,6 +46,8 @@ struct tg_stats_cpu {
>>  	struct blkg_rwstat		service_bytes;
>>  	/* total IOs serviced, post merge */
>>  	struct blkg_rwstat		serviced;
>> +	/* total IOs queued, not submitted to the underlying device. */
>> +	struct blkg_rwstat		io_queued;
>>  };
> 
> Couple of questions.
> 
> - blkg_rwstat is "unsigned" and io_queued can go negative too (Because
>   throttled bio can very well be dispatched from other cpu from a worker
>   thread). So is it a good idea to represent a negative number with 
>   unsingned type?
> 
> - As this stat is per cpu, a reader might very well see negative (or a
>   huge unsigned value) as number of io_queued. Not sure if that is acceptable.
>   How would user space come to know whether it is a valid value or not. I
>   thought per cpu stats are good for continuously increasing values but
>   not necessarily for values which can increase as well as decrease.
You are right. So I should just use throtl_grp->nr_queued to display the
total numbers of ios being throttled and I guess a rcu_read_lock should
be enough for me to access that data.

Thanks for the review.

Thanks
Tao

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

[Vivek Goyal]

On Tue, Sep 04, 2012 at 10:12:49PM +0800, Tao Ma wrote:
> On 09/04/2012 09:35 PM, Vivek Goyal wrote:
> > On Fri, Aug 31, 2012 at 01:15:09PM +0800, Tao Ma wrote:
> > 
> > [..]
> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> >> index 1588c2d..9317d71 100644
> >> --- a/block/blk-throttle.c
> >> +++ b/block/blk-throttle.c
> >> @@ -46,6 +46,8 @@ struct tg_stats_cpu {
> >>  	struct blkg_rwstat		service_bytes;
> >>  	/* total IOs serviced, post merge */
> >>  	struct blkg_rwstat		serviced;
> >> +	/* total IOs queued, not submitted to the underlying device. */
> >> +	struct blkg_rwstat		io_queued;
> >>  };
> > 
> > Couple of questions.
> > 
> > - blkg_rwstat is "unsigned" and io_queued can go negative too (Because
> >   throttled bio can very well be dispatched from other cpu from a worker
> >   thread). So is it a good idea to represent a negative number with 
> >   unsingned type?
> > 
> > - As this stat is per cpu, a reader might very well see negative (or a
> >   huge unsigned value) as number of io_queued. Not sure if that is acceptable.
> >   How would user space come to know whether it is a valid value or not. I
> >   thought per cpu stats are good for continuously increasing values but
> >   not necessarily for values which can increase as well as decrease.
> You are right. So I should just use throtl_grp->nr_queued to display the
> total numbers of ios being throttled and I guess a rcu_read_lock should
> be enough for me to access that data.

Not sure how rcu_read_lock() is going to help. Can you explain a bit more.

For 64bit, we should not require any locking as updation always happens
under queue_lock(for io_queued). And lockless reading should be just fine
as updates to 64bit values will be atomic.

Only problem is reading of 64bit io_queued on 32bit platforms.

As updation always happens under queue_lock, we don't gain anything by making
this stat per cpu. And for 32bit, we probably can updation/reading using
sequence counter to make sure we don't get invalid values and read will 
still be lockless.

Thanks
Vivek

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

[Vivek Goyal]

On Tue, Sep 04, 2012 at 10:12:49PM +0800, Tao Ma wrote:
> On 09/04/2012 09:35 PM, Vivek Goyal wrote:
> > On Fri, Aug 31, 2012 at 01:15:09PM +0800, Tao Ma wrote:
> > 
> > [..]
> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> >> index 1588c2d..9317d71 100644
> >> --- a/block/blk-throttle.c
> >> +++ b/block/blk-throttle.c
> >> @@ -46,6 +46,8 @@ struct tg_stats_cpu {
> >>  	struct blkg_rwstat		service_bytes;
> >>  	/* total IOs serviced, post merge */
> >>  	struct blkg_rwstat		serviced;
> >> +	/* total IOs queued, not submitted to the underlying device. */
> >> +	struct blkg_rwstat		io_queued;
> >>  };
> > 
> > Couple of questions.
> > 
> > - blkg_rwstat is "unsigned" and io_queued can go negative too (Because
> >   throttled bio can very well be dispatched from other cpu from a worker
> >   thread). So is it a good idea to represent a negative number with 
> >   unsingned type?
> > 
> > - As this stat is per cpu, a reader might very well see negative (or a
> >   huge unsigned value) as number of io_queued. Not sure if that is acceptable.
> >   How would user space come to know whether it is a valid value or not. I
> >   thought per cpu stats are good for continuously increasing values but
> >   not necessarily for values which can increase as well as decrease.
> You are right. So I should just use throtl_grp->nr_queued to display the
> total numbers of ios being throttled and I guess a rcu_read_lock should
> be enough for me to access that data.

In last mail I missed the point that you are planning to read
tg->nr_queued directly.

thrtl_grp->nr_queued is protected by queue_lock. So to be preceise, you
will have to take queue lock in read path. Current design is that we
don't take queue lock while we are holding blkcg lock(to avoid deadlock).
So in read path, reading tg->nr_queued under queue_lock is not possible.

Reading a bit, nr_queued[] is of type "unsinged int". So update to it
are atomic both at 32bit and 64bit platforms. If that's the case, I 
think we should be able to just read tg->nr_queued only under blkcg->lock
(like rest of the stats). As we are holding blkcg->lock, none of the
tg/blkg can go away, so no rcu stuff should be required.

Thanks
Vivek

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

[Tejun Heo]

Hello, Tao Ma.

On Sat, Sep 01, 2012 at 09:58:43PM +0800, Tao Ma wrote:
> Vivek and I have talked about its usage in my first try. See the thread
> here. https://lkml.org/lkml/2012/5/22/81
> And I am OK to say it again here. In our case, we use flashcache as a
> block device and the bad thing is that flashcache is a bio-based dm
> target and we can't use block io controller here to control the weight
> of different cgroups. So io throttle is chosen. But as io throttle can
> only set a hard upper limit for different instances, it makes the
> control not flexible enough. Say with io controller, if there is no
> requests form the cgroup with weight 1000, a cgroup with 500 can use the
> whole bandwidth of the underlying device. But if we set 1000 iops for
> cgroup A and 500 iops for cgroup B in io throttle, cgroup B can't exceed
> its limit even if cgroup A has no request pending. So if we can export
> the io_queued information out to the system admin, they can write some
> daemon and in the above case, increase the upper limit of cgroup B to
> some number say 1000. It helps us to utilize the device more
> efficiently. Does it make sense to you?

Somewhat, in a pretty twisted way. :P

> > Adding throttle.io_queued could be a bit more consistent?
>
> sorry, I don't know what is your meaning here. You mean some codes like
> 	blkg_rwstat_add(&stats_cpu->throttle.io_queude, rw, 1)?

So, there already is io_dispatched, so if you have io_queued, you can
read the two and calculate the difference from userland (reading
io_queued first would probably be better to avoid triggering the
throttled condition spuriously).  That way, you don't have to worry
about synchronizing stats across cpus and it's a simple addition of a
stat conter.

Thanks.

-- 
tejun