2004-09-28 15:24:05

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH 2.6.9-rc2 1/2] enhanced I/O accounting data collection

Hi,

> Index: linux/drivers/block/ll_rw_blk.c
> ===================================================================
> --- linux.orig/drivers/block/ll_rw_blk.c 2004-09-12 22:31:31.000000000 -0700
> +++ linux/drivers/block/ll_rw_blk.c 2004-09-27 12:37:04.374234677 -0700
> @@ -1741,6 +1741,7 @@
> {
> DEFINE_WAIT(wait);
> struct request *rq;
>+ unsigned long start_wait = jiffies;
>
> generic_unplug_device(q);
> do {
>@@ -1769,6 +1770,7 @@
> finish_wait(&rl->wait[rw], &wait);
> } while (!rq);
>
>+ current->bwtime += (unsigned long) jiffies - start_wait;
> return rq;
> }

What is the purpose of this hunk alone as block io accounting? It
doesn't make any sense to me - you are accounting the time a process
spends sleeping on a congested queue, it has nothing to do with the
bandwidth time it used. Which, btw, isn't so easy to account on queueing
hardware.

Just curious on what you are trying to achieve here.

--
Jens Axboe


2004-09-29 23:03:06

by Jay Lan

[permalink] [raw]
Subject: Re: [PATCH 2.6.9-rc2 1/2] enhanced I/O accounting data collection

You are right, Jens. In our earlier posting, we also included block
device read/write counters. The block read/write counts are not very
accurate but it fits our customers' needs since they used that
information sort of for performance analysis than for accounting
purpose.

Thus the block read/write counters were removed from our patch so
that we can concentrate on the accounting needs. This bwtime (block
wait time) should have been pulled together with block read/write
counters.

Regards,
- jay


Jens Axboe wrote:
> Hi,
>
>
>>Index: linux/drivers/block/ll_rw_blk.c
>>===================================================================
>>--- linux.orig/drivers/block/ll_rw_blk.c 2004-09-12 22:31:31.000000000 -0700
>>+++ linux/drivers/block/ll_rw_blk.c 2004-09-27 12:37:04.374234677 -0700
>>@@ -1741,6 +1741,7 @@
>>{
>> DEFINE_WAIT(wait);
>> struct request *rq;
>>+ unsigned long start_wait = jiffies;
>>
>> generic_unplug_device(q);
>> do {
>>@@ -1769,6 +1770,7 @@
>> finish_wait(&rl->wait[rw], &wait);
>> } while (!rq);
>>
>>+ current->bwtime += (unsigned long) jiffies - start_wait;
>> return rq;
>>}
>
>
> What is the purpose of this hunk alone as block io accounting? It
> doesn't make any sense to me - you are accounting the time a process
> spends sleeping on a congested queue, it has nothing to do with the
> bandwidth time it used. Which, btw, isn't so easy to account on queueing
> hardware.
>
> Just curious on what you are trying to achieve here.
>