From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753168AbdGLUtl (ORCPT ); Wed, 12 Jul 2017 16:49:41 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:47986 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751862AbdGLUtj (ORCPT ); Wed, 12 Jul 2017 16:49:39 -0400 Date: Wed, 12 Jul 2017 13:49:38 -0700 From: Andrew Morton To: Dmitry Vyukov Cc: Akinobu Mita , LKML Subject: Re: [PATCH -mm 3/5] fault-inject: make fail-nth read/write interface symmetric Message-Id: <20170712134938.7c954021c6ebe90e609d02a1@linux-foundation.org> In-Reply-To: References: <1491490561-10485-1-git-send-email-akinobu.mita@gmail.com> <1491490561-10485-4-git-send-email-akinobu.mita@gmail.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 7 Apr 2017 22:37:01 +0200 Dmitry Vyukov wrote: > On Thu, Apr 6, 2017 at 4:55 PM, Akinobu Mita wrote: > > The read interface for fail-nth looks a bit odd. Read from this file > > returns "NYYYY..." or "YYYYY..." (this makes me surprise when cat this > > file). Because there is no EOF condition. The first character indicates > > current->fail_nth is zero or not, and then current->fail_nth is reset > > to zero. > > > > Just returning task->fail_nth value is more natural to understand. > > > > Cc: Dmitry Vyukov > > Signed-off-by: Akinobu Mita > > --- > > Documentation/fault-injection/fault-injection.txt | 13 +++++++------ > > fs/proc/base.c | 14 ++++++-------- > > 2 files changed, 13 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt > > index a321905..370ddcb 100644 > > --- a/Documentation/fault-injection/fault-injection.txt > > +++ b/Documentation/fault-injection/fault-injection.txt > > @@ -139,9 +139,9 @@ o proc entries > > - /proc/self/task//fail-nth: > > > > Write to this file of integer N makes N-th call in the task fail. > > - Read from this file returns a single char 'Y' or 'N' > > - that says if the fault setup with a previous write to this file was > > - injected or not, and disables the fault if it wasn't yet injected. > > + Read from this file returns a integer value. A value of '0' indicates > > + that the fault setup with a previous write to this file was injected. > > + A positive integer N indicates that the fault wasn't yet injected. > > Note that this file enables all types of faults (slab, futex, etc). > > This setting takes precedence over all other generic debugfs settings > > like probability, interval, times, etc. But per-capability settings > > @@ -325,13 +325,14 @@ int main() > > write(fail_nth, buf, strlen(buf)); > > res = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds); > > err = errno; > > - read(fail_nth, buf, 1); > > + pread(fail_nth, buf, sizeof(buf), 0); > > if (res == 0) { > > close(fds[0]); > > close(fds[1]); > > } > > - printf("%d-th fault %c: res=%d/%d\n", i, buf[0], res, err); > > - if (buf[0] != 'Y') > > + printf("%d-th fault %c: res=%d/%d\n", i, atoi(buf) ? 'N' : 'Y', > > + res, err); > > + if (atoi(buf)) > > break; > > } > > return 0; > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 42c52e2..9d14215 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -1383,7 +1383,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf, > > size_t count, loff_t *ppos) > > { > > struct task_struct *task; > > - int err; > > + char numbuf[PROC_NUMBUF]; > > + ssize_t len; > > > > task = get_proc_task(file_inode(file)); > > if (!task) > > @@ -1391,13 +1392,10 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf, > > put_task_struct(task); > > if (task != current) > > return -EPERM; > > - if (count < 1) > > - return -EINVAL; > > - err = put_user((char)(current->fail_nth ? 'N' : 'Y'), buf); > > - if (err) > > - return err; > > - current->fail_nth = 0; > > - return 1; > > + len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth); > > If we allow setting this for non current task, then we need to prevent > data races as the task uses task->fail_nth concurrently. Reads then > should use READ_ONCE and writes in fault-inject.c should use > WRITE_ONCE. This remains unresolved?