[PATCH 1/1] task IO accounting: correctly account threads IO statistics

[PATCH 1/1] task IO accounting: correctly account threads IO statistics

[Andrea Righi]

Oleg Nesterov points out that we should check that the task is still alive
before we iterate over the threads. This patch includes a fixup for this.

Also simplify do_io_accounting() implementation.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
---
 fs/proc/base.c |   56 ++++++++++++++++++++++----------------------------------
 1 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a891fe4..346500d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2381,35 +2381,18 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 	u64 rchar, wchar, syscr, syscw;
 	struct task_io_accounting ioac;
 
-	if (!whole) {
-		rchar = task->rchar;
-		wchar = task->wchar;
-		syscr = task->syscr;
-		syscw = task->syscw;
-		memcpy(&ioac, &task->ioac, sizeof(ioac));
-	} else {
-		unsigned long flags;
-		struct task_struct *t = task;
-		rchar = wchar = syscr = syscw = 0;
-		memset(&ioac, 0, sizeof(ioac));
+	rchar = task->rchar;
+	wchar = task->wchar;
+	syscr = task->syscr;
+	syscw = task->syscw;
+	memcpy(&ioac, &task->ioac, sizeof(ioac));
 
-		rcu_read_lock();
-		do {
-			rchar += t->rchar;
-			wchar += t->wchar;
-			syscr += t->syscr;
-			syscw += t->syscw;
-
-			ioac.read_bytes += t->ioac.read_bytes;
-			ioac.write_bytes += t->ioac.write_bytes;
-			ioac.cancelled_write_bytes +=
-					t->ioac.cancelled_write_bytes;
-			t = next_thread(t);
-		} while (t != task);
-		rcu_read_unlock();
+	if (whole) {
+		unsigned long flags;
 
 		if (lock_task_sighand(task, &flags)) {
 			struct signal_struct *sig = task->signal;
+			struct task_struct *t = task;
 
 			rchar += sig->rchar;
 			wchar += sig->wchar;
@@ -2420,11 +2403,20 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			ioac.write_bytes += sig->ioac.write_bytes;
 			ioac.cancelled_write_bytes +=
 					sig->ioac.cancelled_write_bytes;
-
+			while_each_thread(task, t) {
+				rchar += t->rchar;
+				wchar += t->wchar;
+				syscr += t->syscr;
+				syscw += t->syscw;
+
+				ioac.read_bytes += t->ioac.read_bytes;
+				ioac.write_bytes += t->ioac.write_bytes;
+				ioac.cancelled_write_bytes +=
+					t->ioac.cancelled_write_bytes;
+			}
 			unlock_task_sighand(task, &flags);
 		}
 	}
-
 	return sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
@@ -2433,13 +2425,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			"read_bytes: %llu\n"
 			"write_bytes: %llu\n"
 			"cancelled_write_bytes: %llu\n",
-			(unsigned long long)rchar,
-			(unsigned long long)wchar,
-			(unsigned long long)syscr,
-			(unsigned long long)syscw,
-			(unsigned long long)ioac.read_bytes,
-			(unsigned long long)ioac.write_bytes,
-			(unsigned long long)ioac.cancelled_write_bytes);
+			rchar, wchar, syscr, syscw,
+			ioac.read_bytes, ioac.write_bytes,
+			ioac.cancelled_write_bytes);
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
-- 
1.5.4.3

Re: [PATCH 1/1] task IO accounting: correctly account threads IO statistics

[Oleg Nesterov]

On 07/26, Andrea Righi wrote:
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2381,35 +2381,18 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  	u64 rchar, wchar, syscr, syscw;
>  	struct task_io_accounting ioac;
>  
> -	if (!whole) {
> -		rchar = task->rchar;
> -		wchar = task->wchar;
> -		syscr = task->syscr;
> -		syscw = task->syscw;
> -		memcpy(&ioac, &task->ioac, sizeof(ioac));
> -	} else {
> -		unsigned long flags;
> -		struct task_struct *t = task;
> -		rchar = wchar = syscr = syscw = 0;
> -		memset(&ioac, 0, sizeof(ioac));
> +	rchar = task->rchar;
> +	wchar = task->wchar;
> +	syscr = task->syscr;
> +	syscw = task->syscw;
> +	memcpy(&ioac, &task->ioac, sizeof(ioac));
>  
> -		rcu_read_lock();
> -		do {
> -			rchar += t->rchar;
> -			wchar += t->wchar;
> -			syscr += t->syscr;
> -			syscw += t->syscw;
> -
> -			ioac.read_bytes += t->ioac.read_bytes;
> -			ioac.write_bytes += t->ioac.write_bytes;
> -			ioac.cancelled_write_bytes +=
> -					t->ioac.cancelled_write_bytes;
> -			t = next_thread(t);
> -		} while (t != task);
> -		rcu_read_unlock();
> +	if (whole) {
> +		unsigned long flags;
>  
>  		if (lock_task_sighand(task, &flags)) {
>  			struct signal_struct *sig = task->signal;
> +			struct task_struct *t = task;
>  
>  			rchar += sig->rchar;
>  			wchar += sig->wchar;
> @@ -2420,11 +2403,20 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  			ioac.write_bytes += sig->ioac.write_bytes;
>  			ioac.cancelled_write_bytes +=
>  					sig->ioac.cancelled_write_bytes;
> -
> +			while_each_thread(task, t) {
> +				rchar += t->rchar;
> +				wchar += t->wchar;
> +				syscr += t->syscr;
> +				syscw += t->syscw;
> +
> +				ioac.read_bytes += t->ioac.read_bytes;
> +				ioac.write_bytes += t->ioac.write_bytes;
> +				ioac.cancelled_write_bytes +=
> +					t->ioac.cancelled_write_bytes;
> +			}
>  			unlock_task_sighand(task, &flags);
>  		}
>  	}
> -
>  	return sprintf(buffer,
>  			"rchar: %llu\n"
>  			"wchar: %llu\n"
> @@ -2433,13 +2425,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
>  			"read_bytes: %llu\n"
>  			"write_bytes: %llu\n"
>  			"cancelled_write_bytes: %llu\n",
> -			(unsigned long long)rchar,
> -			(unsigned long long)wchar,
> -			(unsigned long long)syscr,
> -			(unsigned long long)syscw,
> -			(unsigned long long)ioac.read_bytes,
> -			(unsigned long long)ioac.write_bytes,
> -			(unsigned long long)ioac.cancelled_write_bytes);
> +			rchar, wchar, syscr, syscw,
> +			ioac.read_bytes, ioac.write_bytes,
> +			ioac.cancelled_write_bytes);
>  }

Looks good!

And thanks for correcting me, of course we should initialize t
before while_each_thread(task, t).

Oleg.