Re: [Patch for review] BSD accounting IO stats

Re: [Patch for review] BSD accounting IO stats

[Andi Kleen]

Guillaume Thouvenin <guillaume.thouvenin@bull.net> writes:

> diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
> --- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
> +++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-07-27 09:17:33.149321480 +0200
> @@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
>  
>  	if (rw == READ) {
>  		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
> +		current->rblk += nr_sectors;

This doesn't look very useful, because most writes which
are flushed delayed would get accounted to pdflushd.
Using such inaccurate data for accounting sounds quite dangerous
to me.

If you really wanted to do this i guess you would need to 
track the pid of the process and account it there. But the
process may be already gone, so it would better fit into
some other longer lived data structure (like the uid) 

Overall I don't think this accounting is worth it because
doing it right would be quite some overhead and doing it in
a simple way like this patch is too inaccurate.

-Andi

Re: [Patch for review] BSD accounting IO stats

[Rik van Riel]

On Mon, 2 Aug 2004, Andi Kleen wrote:

> This doesn't look very useful, because most writes which
> are flushed delayed would get accounted to pdflushd.

> If you really wanted to do this i guess you would need to 
> track the pid of the process and account it there.

It may be easier to do this at write(2) time, making the
assumption that most IO done there will eventually hit
the filesystem.

Not sure what to do at read time, the current code may
well be good enough.

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: [Patch for review] BSD accounting IO stats

[Chris Wedgwood]

On Mon, Aug 02, 2004 at 03:11:27PM -0400, Rik van Riel wrote:

> It may be easier to do this at write(2) time, making the
> assumption that most IO done there will eventually hit
> the filesystem.

but for lots of loads that's not true, consider the case of temporary
files which usually never hit the disk before being unlinked


  --cw

Re: [Patch for review] BSD accounting IO stats

[Guillaume Thouvenin]

Andi Kleen wrote:

>>diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
>>--- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
>>+++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-07-27 09:17:33.149321480 +0200
>>@@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
>> 
>> 	if (rw == READ) {
>> 		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
>>+		current->rblk += nr_sectors;
>>    
>>
>
>This doesn't look very useful, because most writes which
>are flushed delayed would get accounted to pdflushd.
>Using such inaccurate data for accounting sounds quite dangerous
>to me.
>  
>
I agree with that. Like you and Andrew said, this metric (write block) 
is just an estimate (quite wrong indeed) of what really occurred in the 
system because some writings are accounted elsewhere (pdflush or 
journaling file system).

I also agree that a rough estimation is not very interesting, therefore 
I'm working on another patch to provide accurate values.

Best,
Guillaume

[Patch for review] BSD accounting IO stats

[Guillaume Thouvenin]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

Hello,

  Here is a small patch for 2.6.8-rc2 that updates information about 
bytes and blocks read and written. Fields are already in the BSD 
accounting files but they are never updated. The patch is based on the 
CSA accounting patch and it doesn't interfere with Tim Schmielau changes 
(BSD process accounting version 3).

 Modifications are:
    - It adds four counters in the task_struct (chars read, chars
      written, blocks read and blocks written). I think it's interesting to
      separate read and write even if this difference is not made in the
      BSD accounting.

    - Those fields are updated in:
        fs/read_write.c for bytes
                 [in sys_read(), sys_readv(), sys_write() and sys_writev]
        drivers/block/ll_rw_blk.c for blocks
                 [in drive_stat_acct()]

 
Hope This Help
Guillaume


[-- Attachment #2: patch-2.6.8-rc2+BSDacct_IO --]
[-- Type: text/plain, Size: 3450 bytes --]

diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
--- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-07-27 09:17:33.149321480 +0200
@@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
 
 	if (rw == READ) {
 		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
+		current->rblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, read_merges);
 	} else if (rw == WRITE) {
 		disk_stat_add(rq->rq_disk, write_sectors, nr_sectors);
+		current->wblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, write_merges);
 	}
diff -uprN -X dontdiff linux-2.6.8-rc2/fs/read_write.c linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c
--- linux-2.6.8-rc2/fs/read_write.c	2004-07-18 06:58:50.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c	2004-07-27 09:20:11.872191936 +0200
@@ -279,6 +279,9 @@ asmlinkage ssize_t sys_read(unsigned int
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar++;
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sys_read);
@@ -295,6 +298,9 @@ asmlinkage ssize_t sys_write(unsigned in
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar++;
+
 	return ret;
 }
 
@@ -517,6 +523,9 @@ sys_readv(unsigned long fd, const struct
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar++;
+
 	return ret;
 }
 
@@ -533,6 +542,9 @@ sys_writev(unsigned long fd, const struc
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar++;
+
 	return ret;
 }
 
diff -uprN -X dontdiff linux-2.6.8-rc2/include/linux/sched.h linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h
--- linux-2.6.8-rc2/include/linux/sched.h	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h	2004-07-27 09:21:08.567572928 +0200
@@ -465,6 +465,8 @@ struct task_struct {
 	char comm[16];
 /* file system info */
 	int link_count, total_link_count;
+/* I/O info: chars read/written, blocks read/written */
+	unsigned long rchar, wchar, rblk, wblk;
 /* ipc stuff */
 	struct sysv_sem sysvsem;
 /* CPU-specific state of this task */
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/acct.c linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c
--- linux-2.6.8-rc2/kernel/acct.c	2004-07-18 06:58:37.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c	2004-07-27 09:22:30.623098592 +0200
@@ -465,8 +465,8 @@ static void do_acct_process(long exitcod
 	}
 	vsize = vsize / 1024;
 	ac.ac_mem = encode_comp_t(vsize);
-	ac.ac_io = encode_comp_t(0 /* current->io_usage */);	/* %% */
-	ac.ac_rw = encode_comp_t(ac.ac_io / 1024);
+	ac.ac_io = encode_comp_t(current->rchar + current->wchar);
+	ac.ac_rw = encode_comp_t(current->rblk + current->wblk);
 	ac.ac_minflt = encode_comp_t(current->min_flt);
 	ac.ac_majflt = encode_comp_t(current->maj_flt);
 	ac.ac_swaps = encode_comp_t(0);
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/fork.c linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c
--- linux-2.6.8-rc2/kernel/fork.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c	2004-07-27 09:23:27.111511048 +0200
@@ -960,6 +960,7 @@ struct task_struct *copy_process(unsigne
 
 	p->utime = p->stime = 0;
 	p->cutime = p->cstime = 0;
+	p->rchar = p->wchar = p->rblk = p->wblk = 0;
 	p->lock_depth = -1;		/* -1 = no lock */
 	p->start_time = get_jiffies_64();
 	p->security = NULL;

Re: [Patch for review] BSD accounting IO stats

[Rik van Riel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 948 bytes --]

On Mon, 2 Aug 2004, Guillaume Thouvenin wrote:

>  Modifications are:
>     - It adds four counters in the task_struct (chars read, chars
>       written, blocks read and blocks written). I think it's interesting to
>       separate read and write even if this difference is not made in the
>       BSD accounting.

Nice.  One question though:

> @@ -295,6 +298,9 @@ asmlinkage ssize_t sys_write(unsigned in
>                 fput_light(file, fput_needed);
>         }
>                                                     
> +       if (ret > 0)
> +               current->wchar++;
> +
>         return ret;
>  }
                                                                                
Shouldn't that be   "current->wchar += ret"  ?

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3450 bytes --]

diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
--- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-07-27 09:17:33.149321480 +0200
@@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
 
 	if (rw == READ) {
 		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
+		current->rblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, read_merges);
 	} else if (rw == WRITE) {
 		disk_stat_add(rq->rq_disk, write_sectors, nr_sectors);
+		current->wblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, write_merges);
 	}
diff -uprN -X dontdiff linux-2.6.8-rc2/fs/read_write.c linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c
--- linux-2.6.8-rc2/fs/read_write.c	2004-07-18 06:58:50.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c	2004-07-27 09:20:11.872191936 +0200
@@ -279,6 +279,9 @@ asmlinkage ssize_t sys_read(unsigned int
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar++;
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sys_read);
@@ -295,6 +298,9 @@ asmlinkage ssize_t sys_write(unsigned in
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar++;
+
 	return ret;
 }
 
@@ -517,6 +523,9 @@ sys_readv(unsigned long fd, const struct
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar++;
+
 	return ret;
 }
 
@@ -533,6 +542,9 @@ sys_writev(unsigned long fd, const struc
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar++;
+
 	return ret;
 }
 
diff -uprN -X dontdiff linux-2.6.8-rc2/include/linux/sched.h linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h
--- linux-2.6.8-rc2/include/linux/sched.h	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h	2004-07-27 09:21:08.567572928 +0200
@@ -465,6 +465,8 @@ struct task_struct {
 	char comm[16];
 /* file system info */
 	int link_count, total_link_count;
+/* I/O info: chars read/written, blocks read/written */
+	unsigned long rchar, wchar, rblk, wblk;
 /* ipc stuff */
 	struct sysv_sem sysvsem;
 /* CPU-specific state of this task */
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/acct.c linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c
--- linux-2.6.8-rc2/kernel/acct.c	2004-07-18 06:58:37.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c	2004-07-27 09:22:30.623098592 +0200
@@ -465,8 +465,8 @@ static void do_acct_process(long exitcod
 	}
 	vsize = vsize / 1024;
 	ac.ac_mem = encode_comp_t(vsize);
-	ac.ac_io = encode_comp_t(0 /* current->io_usage */);	/* %% */
-	ac.ac_rw = encode_comp_t(ac.ac_io / 1024);
+	ac.ac_io = encode_comp_t(current->rchar + current->wchar);
+	ac.ac_rw = encode_comp_t(current->rblk + current->wblk);
 	ac.ac_minflt = encode_comp_t(current->min_flt);
 	ac.ac_majflt = encode_comp_t(current->maj_flt);
 	ac.ac_swaps = encode_comp_t(0);
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/fork.c linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c
--- linux-2.6.8-rc2/kernel/fork.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c	2004-07-27 09:23:27.111511048 +0200
@@ -960,6 +960,7 @@ struct task_struct *copy_process(unsigne
 
 	p->utime = p->stime = 0;
 	p->cutime = p->cstime = 0;
+	p->rchar = p->wchar = p->rblk = p->wblk = 0;
 	p->lock_depth = -1;		/* -1 = no lock */
 	p->start_time = get_jiffies_64();
 	p->security = NULL;

Re: [Patch for review] BSD accounting IO stats

[Guillaume Thouvenin]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

Rik van Riel wrote:

> On Mon, 2 Aug 2004, Guillaume Thouvenin wrote:
>
>>  Modifications are:
>>     - It adds four counters in the task_struct (chars read, chars
>>       written, blocks read and blocks written). I think it's 
>> interesting to
>>       separate read and write even if this difference is not made in the
>>       BSD accounting.
>
>
> Nice.  One question though:
>
>> @@ -295,6 +298,9 @@ asmlinkage ssize_t sys_write(unsigned in
>>                 fput_light(file, fput_needed);
>>         }
>>                                                     +       if (ret > 0)
>> +               current->wchar++;
>> +
>>         return ret;
>>  }
>
>    Shouldn't that be   "current->wchar += ret"  ?


Yes you're right. I attach a new patch with those modifications.
Thank you

Guillaume

[-- Attachment #2: patch-2.6.8-rc2+BSDacct_IO --]
[-- Type: text/plain, Size: 3750 bytes --]

diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
--- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-08-02 14:40:24.406684216 +0200
@@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
 
 	if (rw == READ) {
 		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
+		current->rblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, read_merges);
 	} else if (rw == WRITE) {
 		disk_stat_add(rq->rq_disk, write_sectors, nr_sectors);
+		current->wblk += nr_sectors;
 		if (!new_io)
 			disk_stat_inc(rq->rq_disk, write_merges);
 	}
diff -uprN -X dontdiff linux-2.6.8-rc2/fs/read_write.c linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c
--- linux-2.6.8-rc2/fs/read_write.c	2004-07-18 06:58:50.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/fs/read_write.c	2004-08-02 14:55:13.119579320 +0200
@@ -279,6 +279,9 @@ asmlinkage ssize_t sys_read(unsigned int
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar += ret;
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sys_read);
@@ -295,6 +298,9 @@ asmlinkage ssize_t sys_write(unsigned in
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar += ret;
+
 	return ret;
 }
 
@@ -517,6 +523,9 @@ sys_readv(unsigned long fd, const struct
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->rchar += ret;
+
 	return ret;
 }
 
@@ -533,6 +542,9 @@ sys_writev(unsigned long fd, const struc
 		fput_light(file, fput_needed);
 	}
 
+	if (ret > 0)
+		current->wchar += ret;
+
 	return ret;
 }
 
@@ -607,6 +619,11 @@ static ssize_t do_sendfile(int out_fd, i
 
 	retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor, out_file);
 
+	if (retval > 0) {
+		current->rchar += retval;
+		current->wchar += retval;
+	}
+	
 	if (*ppos > max)
 		retval = -EOVERFLOW;
 
diff -uprN -X dontdiff linux-2.6.8-rc2/include/linux/sched.h linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h
--- linux-2.6.8-rc2/include/linux/sched.h	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/include/linux/sched.h	2004-08-02 14:32:28.877975568 +0200
@@ -465,6 +465,8 @@ struct task_struct {
 	char comm[16];
 /* file system info */
 	int link_count, total_link_count;
+/* I/O info: chars read/written, blocks read/written */
+	unsigned long rchar, wchar, rblk, wblk;
 /* ipc stuff */
 	struct sysv_sem sysvsem;
 /* CPU-specific state of this task */
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/acct.c linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c
--- linux-2.6.8-rc2/kernel/acct.c	2004-07-18 06:58:37.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/acct.c	2004-08-02 14:33:45.902266096 +0200
@@ -465,8 +465,8 @@ static void do_acct_process(long exitcod
 	}
 	vsize = vsize / 1024;
 	ac.ac_mem = encode_comp_t(vsize);
-	ac.ac_io = encode_comp_t(0 /* current->io_usage */);	/* %% */
-	ac.ac_rw = encode_comp_t(ac.ac_io / 1024);
+	ac.ac_io = encode_comp_t(current->rchar + current->wchar);
+	ac.ac_rw = encode_comp_t(current->rblk + current->wblk);
 	ac.ac_minflt = encode_comp_t(current->min_flt);
 	ac.ac_majflt = encode_comp_t(current->maj_flt);
 	ac.ac_swaps = encode_comp_t(0);
diff -uprN -X dontdiff linux-2.6.8-rc2/kernel/fork.c linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c
--- linux-2.6.8-rc2/kernel/fork.c	2004-07-18 06:57:42.000000000 +0200
+++ linux-2.6.8-rc2+BSDacct_IO/kernel/fork.c	2004-08-02 14:33:06.991181480 +0200
@@ -960,6 +960,7 @@ struct task_struct *copy_process(unsigne
 
 	p->utime = p->stime = 0;
 	p->cutime = p->cstime = 0;
+	p->rchar = p->wchar = p->rblk = p->wblk = 0;
 	p->lock_depth = -1;		/* -1 = no lock */
 	p->start_time = get_jiffies_64();
 	p->security = NULL;

Re: [Patch for review] BSD accounting IO stats

[Andrew Morton]

Guillaume Thouvenin <guillaume.thouvenin@bull.net> wrote:
>
> diff -uprN -X dontdiff linux-2.6.8-rc2/drivers/block/ll_rw_blk.c linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c
>  --- linux-2.6.8-rc2/drivers/block/ll_rw_blk.c	2004-07-18 06:57:42.000000000 +0200
>  +++ linux-2.6.8-rc2+BSDacct_IO/drivers/block/ll_rw_blk.c	2004-07-27 09:17:33.149321480 +0200
>  @@ -1949,10 +1949,12 @@ void drive_stat_acct(struct request *rq,
>   
>   	if (rw == READ) {
>   		disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
>  +		current->rblk += nr_sectors;
>   		if (!new_io)
>   			disk_stat_inc(rq->rq_disk, read_merges);
>   	} else if (rw == WRITE) {
>   		disk_stat_add(rq->rq_disk, write_sectors, nr_sectors);
>  +		current->wblk += nr_sectors;
>   		if (!new_io)
>   			disk_stat_inc(rq->rq_disk, write_merges);
>   	}

The `wblk' accounting will be quite wrong - most writes will be accounted
to pdflush, kjournald, etc.