The following series implements some additional stats for IO controller.

These stats have helped us debug issues with earlier IO controller
versions and should be useful now as well.
We've been using these stats for monitoring and debugging problems after the
fact as these stats can be collected and stored for later use.

One might argue that most of this information can be exported using blktrace
when debugging. However, blktrace has non-trivial performance impact and
cannot be always turned on. These stats provide a way for continuous monitoring
without losing much performance on rotational disks. We've been able to look
at these stats and debug issues after problems have been reported in the wild
and understand the IO pattern of the affected workloads.
Some of these stats are also a good data source for high-level analysis and
capacity planning.

This patchset adds 4 stats and I will send out another patchset later for
stats like io_merged and some stats that can be turned on only for
debugging - idle_time (total time spent idling for this blkio_group),
wait_time (total time spent by the blkio_group waiting before any one of its
queues got a timeslice). I've tried to breakdown the stats and sent the most
basic ones here.

Changelog from v1 (most based on Vivek Goyal's comments):
o blkio.time now exports in jiffies as before
o Added stats description in patch description and
  Documentation/cgroup/blkio-controller.txt
o Prefix all stats functions with blkio and make them static as applicable
o replace IO_TYPE_MAX with IO_TYPE_TOTAL
o Moved #define constant to top of blk-cgroup.c
o Pass dev_t around instead of char *
o Add note to documentation file about resetting stats
o use BLK_CGROUP_MODULE in addition to BLK_CGROUP config option in #ifdef
  statements
o Avoid struct request specific knowledge in blk-cgroup. blk-cgroup.h now has
  rq_direction() and rq_sync() functions which are used by CFQ and when using
  io-controller at a higher level, bio_* functions can be ...
[ message continues ]

that info at request dispatch with other stats now. This patch removes the
existing support for accounting sectors for a blkio_group. This will be added
back differently in the next two patches.

Signed-off-by: Divyesh Shah<dpshah@google.com>
---

 block/blk-cgroup.c  |    3 +--
 block/blk-cgroup.h  |    6 ++----
 block/cfq-iosched.c |   10 ++--------
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4b686ad..5be3981 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -56,10 +56,9 @@ struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
 EXPORT_SYMBOL_GPL(cgroup_to_blkio_cgroup);
 
 void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
-			unsigned long time, unsigned long sectors)
+						unsigned long time)
 {
 	blkg->time += time;
-	blkg->sectors += sectors;
 }
 EXPORT_SYMBOL_GPL(blkiocg_update_blkio_group_stats);
 
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 8ccc204..fe44517 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -106,7 +106,7 @@ extern int blkiocg_del_blkio_group(struct blkio_group *blkg);
 extern struct blkio_group *blkiocg_lookup_group(struct blkio_cgroup *blkcg,
 						void *key);
 void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
-			unsigned long time, unsigned long sectors);
+						unsigned long time);
 #else
 struct cgroup;
 static inline struct blkio_cgroup *
@@ -123,8 +123,6 @@ blkiocg_del_blkio_group(struct blkio_group *blkg) { return 0; }
 static inline struct blkio_group *
 blkiocg_lookup_group(struct blkio_cgroup *blkcg, void *key) { return NULL; }
 static inline void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
-			unsigned long time, unsigned long sectors)
-{
-}
+						unsigned long time) {}
 #endif
 #endif /* _BLK_CGROUP_H */
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ef1680b..c18e348 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -141,8 +141,6 @@ ...
[ message continues ]

Hi Divyesh,

I had found above message useful when I am looking at blktraces. It helps
to know how many sectors one could transfer in a given time slice.
Especially if you are comparing two queues in two groups. I think we are
going to loose this information now?

--

[ message continues ]

Would printing the number of sectors for each request help? If you
think this is really important we can keep this redundant information
--

[ message continues ]

Printing it for each request does not help, because I was interested total
of once slice. How long was the slice and how many sectors we completed.

Well, for the time being, don't worry about it. If I really need it I 
will put anohter patch to re-introduce it.

--

[ message continues ]

- io_service_time (the actual time in ns taken by the dis to service the IO)
- io_wait_time (the time spent waiting in the IO shceduler queues before
  getting serviced)
- io_serviced (number of IOs serviced from this blkio_group)
- io_service_bytes (Number of bytes served for this cgroup)

These stats are accumulated per operation type helping us to distinguish between
read and write, and sync and async IO. This patch does not increment any of
these stats.

Signed-off-by: Divyesh Shah<dpshah@google.com>
---

 Documentation/cgroups/blkio-controller.txt |   33 +++++
 block/blk-cgroup.c                         |  179 +++++++++++++++++++++++++---
 block/blk-cgroup.h                         |   41 +++++-
 block/cfq-iosched.c                        |    2 
 4 files changed, 229 insertions(+), 26 deletions(-)

diff --git a/Documentation/cgroups/blkio-controller.txt b/Documentation/cgroups/blkio-controller.txt
index 630879c..ededdca 100644
--- a/Documentation/cgroups/blkio-controller.txt
+++ b/Documentation/cgroups/blkio-controller.txt
@@ -75,6 +75,9 @@ CONFIG_DEBUG_BLK_CGROUP
 
 Details of cgroup files
 =======================
+Writing an int to any of the stats files (which excludes weight) will result
+in all stats for that cgroup to be erased.
+
 - blkio.weight
 	- Specifies per cgroup weight.
 
@@ -92,6 +95,36 @@ Details of cgroup files
 	  third field specifies the number of sectors transferred by the
 	  group to/from the device.
 
+- blkio.io_service_bytes
+	- Number of bytes transferred to/from the disk by the group. These
+	  are further divided by the type of operation - read or write, sync
+	  or async. First two fields specify the major and minor number of the
+	  device, third field specifies the operation type and the fourth field
+	  specifies the number of bytes.
+
+- blkio.io_serviced
+	- Number of IOs completed to/from the disk by the group. These
+	  are further divided by the type of operation - read or write, sync
+	  or async. First two fields ...
[ message continues ]

Missed the v2 tag on this patch. Resending. Apologies for the spam.

--

[ message continues ]

Hi Divyesh,

Looking the third patch where you acutally increment the stats, this is
how service time is calculated.

- Save start_time in rq, when driver actually removes the request from
  request queue.
- at request completion time calculate service time.
  service_time = now - start_time.

This works very well if driver/device does not have NCQ and process one
request at a time. But with NCQ, we can multiple requests in the driver
queue at the same time then we can run into issues.

- With NCQ, time becomes cumulative. So if three requests rq1, rq2 and rq3
  are in disk queue, and if requests are processed in the order rq1,rq2,rq3 by
  the disk (no parallel channles), then rq1 and rq2's completion time is
  added in rq3. So total service time of the group can be much more than
  actual time elapsed. That does not seem right. Did you face this issue in
  your testing?


Same is the case with io_wait_time, Because time is cumulative, actual
io_wait_time, can be more than real time elapsed.

--

[ message continues ]

You are right. With NCQ, the io_service_time numbers aren't exact.
I've only tested this without NCQ for which these stats are accurate
and very useful. With NCQ turned on I don't see a good way of getting
this information accurately since only the disk knows when it actually
starts to service a given request. With NCQ, io_service_time will have
cumulative time as you pointed out which may still be useful for some
high-level analysis if you also have the average queue depth for the
device.

io_wait_time stat makes sense with or w/o NCQ since its defined to
only include the scheduler queueing time for each IO and is cumulative
by design. I should probably reword the definition of io_service_time
to indicate time after dispatch to the driver and request completion
and also add a comment of how NCQ affects this stat. Sounds good?

--

[ message continues ]

Most of the modern disks now seem to be be NCQ. But I guess we can keep

I am not sure if io_wait_time is cumulative by design. Think of this,
somebody says that this cgroup has waited for 1s on CFQ queue but actually
only .5 seconds have elapsed since the observation started.

So I would say that please document it very properly in blkio-controller.txt
so that it is clear what exactly these numbers represent and what to expect on
NCQ disks.

Thanks
--

[ message continues ]

--

[ message continues ]

On Fri, Apr 02, 2010 at 06:56:35PM -0700, Divyesh Shah wrote:


What is the significance of limiting major:minor string to 10 characters?
I see BDEVT_SIZE being used in genhd.c. But it looks like it is valid
only if we printing numbers in hex.

In genhd.c:diskstats_show(), we seem to be using %4d and %7d for major
minor numbers. In extended minor allocation scheme, we seem to be having
20 bits for minor numbers, so 7 decimal places for representing max minor
number will make sense. Not sure about major number though.

So it might be safe to just to follow diskstats_show convention and at
least keep the buffer size as 12 (4:7).

--

[ message continues ]

--

[ message continues ]

We also add start_time_ns and io_start_time_ns fields to struct request
here to record the time when a request is created and when it is
dispatched to device. We use ns uints here as ms and jiffies are
not very useful for non-rotational media.

Signed-off-by: Divyesh Shah<dpshah@google.com>
---

 block/blk-cgroup.c     |   49 ++++++++++++++++++++++++++++++++++++++++++++++--
 block/blk-cgroup.h     |   33 +++++++++++++++++++++++++++++---
 block/blk-core.c       |    6 ++++--
 block/cfq-iosched.c    |    6 +++++-
 include/linux/blkdev.h |   38 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 123 insertions(+), 9 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index cac10b2..21c0d28 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -15,6 +15,7 @@
 #include <linux/kdev_t.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/blkdev.h>
 #include "blk-cgroup.h"
 
 #define MAX_KEY_LEN 100
@@ -57,6 +58,16 @@ struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
 }
 EXPORT_SYMBOL_GPL(cgroup_to_blkio_cgroup);
 
+/*
+ * Add to the appropriate stat variable depending on the request type.
+ * This should be called with the blkg->stats_lock held.
+ */
+void io_add_stat(uint64_t *stat, uint64_t add, int direction, int sync)
+{
+	stat[direction] += add;
+	stat[sync] += add;
+}
+
 void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
 {
 	unsigned long flags;
@@ -67,6 +78,40 @@ void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
 }
 EXPORT_SYMBOL_GPL(blkiocg_update_timeslice_used);
 
+void blkiocg_update_dispatch_stats(struct blkio_group *blkg,
+				uint64_t bytes, int direction, int sync)
+{
+	struct blkio_group_stats *stats;
+	unsigned long flags;
+
+	spin_lock_irqsave(&blkg->stats_lock, flags);
+	stats = &blkg->stats;
+	stats->sectors += bytes >> 9;
+	io_add_stat(stats->io_serviced, 1, direction, sync);
+	io_add_stat(stats->io_service_bytes, ...
[ message continues ]

static? blkio_add_stat()?

how about using "bool" for direction and sync? This is true throughout the

blkdev.h offers rq_data_dir(). Let mapping of 0 or 1 to correct enum
happen in blkio-cgroup.c. This is not rq property. This is internal to


blkdev.h already offers rq_is_sync(). We can use that. Mapping of 1 or 0
to corrent enum number IO_SYNC, IO_ASYNC can be done by blkio-cgroup.c


--

[ message continues ]