Re: [PATCH 3/3] blkio: Increment the blkio cgroup stats for real now

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Apr 02, 2010 at 04:36:34PM -0700, Divyesh Shah wrote:
> On Fri, Apr 2, 2010 at 12:10 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Apr 01, 2010 at 03:01:41PM -0700, Divyesh Shah wrote:
> >> 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     |   60 ++++++++++++++++++++++++++++++++++++++++++++++--
> >>  block/blk-cgroup.h     |   14 +++++++++--
> >>  block/blk-core.c       |    6 +++--
> >>  block/cfq-iosched.c    |    4 ++-
> >>  include/linux/blkdev.h |   20 +++++++++++++++-
> >>  5 files changed, 95 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> >> index ad6843f..9af7257 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"
> >>
> >>  static DEFINE_SPINLOCK(blkio_list_lock);
> >> @@ -55,6 +56,26 @@ 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, unsigned int flags)
> >> +{
> >> +     if (flags & REQ_RW)
> >> +             stat[IO_WRITE] += add;
> >> +     else
> >> +             stat[IO_READ] += add;
> >> +     /*
> >> +      * Everywhere in the block layer, an IO is treated as sync if it is a
> >> +      * read or a SYNC write. We follow the same norm.
> >> +      */
> >> +     if (!(flags & REQ_RW) || flags & REQ_RW_SYNC)
> >> +             stat[IO_SYNC] += add;
> >> +     else
> >> +             stat[IO_ASYNC] += add;
> >> +}
> >> +
> >
> > Hi Divyesh,
> >
> > Can we have any request based information limited to cfq and not put that
> > in blkio-cgroup. The reason being that I am expecting that some kind of
> > max bw policy interface will not necessarily be implemented at CFQ
> > level. We might have to implement it at higher level so that it can
> > work with all dm/md devices. If that's the case, then it might very well
> > be either a bio based interface also.
> >
> > So just keeping that possibility in mind, can we keep blk-cgroup as
> > generic as possible and not necessarily make it dependent on "struct
> > request".
> 
> Ok. I do understand the motivation for keeping the request related
> info out of blk-cgroup. Everything except the rq->cmd_flags can be
> easily done away with. Maybe I'll need to have CFQ send the sync and
> direction bits as args to the functions that need it. Not ideal coz
> we'll have functions with many args but I guess its not that bad too.
> 
> >
> > If you implement, two dimensional arrays for stats then we can have
> > following function.
> >
> > blkio_add_stat(enum stat_type var enum stat_sub_type var_type, u64 val)
> 
> I would want to avoid calls like these from CFQ into the blkcg code
> because many CFQ events trigger update for multiple stats (you'll see
> more with stats in later patchsets) and doing these calls
> independently for each stat would mean that we would also need to grab
> the stats_lock multiple times when we could've avoided that.

I understand the need to club the updates and reduce the need of taking
stats_lock multiple times. I was thinking of any of following.

- Get rid of reset interface per cgroup. Rely on changing ioscheduler on
  request queue and that will get rid of stats_lock entirely.

- Can we use a function blkio_add_stat() with variable number of arguments
  so that more than one stat can be updated in a single call?

If you have other ideas to implement it without assuming "struct rq" in
blk-cgroup, please do that.

Thanks
Vivek