linux-kernel - Re: [RFC][PATCH v3 2/6] mm, directio: fix fork vs direct-io race (read(2) side IOW gup(write) side)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x49ab6jyyiy.fsf@segfault.boston.devel.redhat.com>
Date:	Tue, 14 Apr 2009 12:45:41 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...l.org>,
	Andrew Morton <akpm@...l.org>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Andrea Arcangeli <aarcange@...hat.com>, linux-mm@...ck.org,
	linux-fsdevel@...r.kernel.org, Hugh Dickins <hugh@...itas.com>,
	Zach Brown <zach.brown@...cle.com>,
	Andy Grover <andy.grover@...cle.com>
Subject: Re: [RFC][PATCH v3 2/6] mm, directio: fix fork vs direct-io race (read(2) side IOW gup(write) side)

KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com> writes:

> Oops, I forgot some cc. resend it.
>
>> Subject: [PATCH] mm, directio: fix fork vs direct-io race
>> 
>> 
>> ChangeLog:
>> V2 -> V3
>>    o remove early decow logic
>> 
>> V1 -> V2
>>    o add dio+aio logic
>> 

[snip test programs]

>> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
>> Sugessted-by: Linus Torvalds <torvalds@...l.org>
>> Cc: Hugh Dickins <hugh@...itas.com>
>> Cc: Andrew Morton <akpm@...l.org>
>> Cc: Nick Piggin <nickpiggin@...oo.com.au>
>> Cc: Andrea Arcangeli <aarcange@...hat.com>
>> Cc: Jeff Moyer <jmoyer@...hat.com>
>> Cc: Zach Brown <zach.brown@...cle.com>
>> Cc: Andy Grover <andy.grover@...cle.com>
>> Cc: linux-fsdevel@...r.kernel.org
>> Cc: linux-mm@...ck.org
>> ---
>>  fs/direct-io.c            |   16 ++++++++++++++++
>>  include/linux/init_task.h |    1 +
>>  include/linux/mm_types.h  |    6 ++++++
>>  kernel/fork.c             |    3 +++
>>  4 files changed, 26 insertions(+)
>> 
>> Index: b/fs/direct-io.c
>> ===================================================================
>> --- a/fs/direct-io.c	2009-04-13 00:24:01.000000000 +0900
>> +++ b/fs/direct-io.c	2009-04-13 01:36:37.000000000 +0900
>> @@ -131,6 +131,9 @@ struct dio {
>>  	int is_async;			/* is IO async ? */
>>  	int io_error;			/* IO error in completion path */
>>  	ssize_t result;                 /* IO result */
>> +
>> +	/* fork exclusive stuff */
>> +	struct mm_struct *mm;
>>  };
>>  
>>  /*
>> @@ -244,6 +247,12 @@ static int dio_complete(struct dio *dio,
>>  		/* lockdep: non-owner release */
>>  		up_read_non_owner(&dio->inode->i_alloc_sem);
>>  
>> +	if (dio->rw == READ) {
>> +		BUG_ON(!dio->mm);
>> +		up_read_non_owner(&dio->mm->mm_pinned_sem);
>> +		mmdrop(dio->mm);
>> +	}
>> +
>>  	if (ret == 0)
>>  		ret = dio->page_errors;
>>  	if (ret == 0)
>> @@ -942,6 +951,7 @@ direct_io_worker(int rw, struct kiocb *i
>>  	ssize_t ret = 0;
>>  	ssize_t ret2;
>>  	size_t bytes;
>> +	struct mm_struct *mm;
>>  
>>  	dio->inode = inode;
>>  	dio->rw = rw;
>> @@ -960,6 +970,12 @@ direct_io_worker(int rw, struct kiocb *i
>>  	spin_lock_init(&dio->bio_lock);
>>  	dio->refcount = 1;
>>  
>> +	if (rw == READ) {
>> +		mm = dio->mm = current->mm;
>> +		atomic_inc(&mm->mm_count);
>> +		down_read_non_owner(&mm->mm_pinned_sem);
>> +	}
>> +

So, if you're continuously submitting async read I/O, you will starve
out the fork() call indefinitely.  I agree that you want to allow
multiple O_DIRECT I/Os to go on at once, but I'm not sure this is the
right way forward.

I have to weigh in and say I much prefer the patches posted by Nick and
Andrea.  They were much more contained and had negligible performance
impact.

Have you done any performance measurements on this patch series?

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/