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

[Jeff Moyer]

KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> Sugessted-by: Linus Torvalds <torvalds@xxxxxxxx>
>> Cc: Hugh Dickins <hugh@xxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxx>
>> Cc: Nick Piggin <nickpiggin@xxxxxxxxxxxx>
>> Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>> Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
>> Cc: Zach Brown <zach.brown@xxxxxxxxxx>
>> Cc: Andy Grover <andy.grover@xxxxxxxxxx>
>> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
>> Cc: linux-mm@xxxxxxxxx
>> ---
>>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/