lkml.org 
[lkml]   [2003]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2.6.0-test9-mm5] aio-dio-fallback-bio_count-race.patch
On Tue, Nov 25, 2003 at 03:49:31PM -0800, Daniel McNeil wrote:
> Suparna,
>
> Yes your patch did help. I originally had CONFIG_DEBUG_SLAB=y which
> was helping me see problems because the the freed dio was getting
> poisoned. I also tested with CONFIG_DEBUG_PAGEALLOC=y which is
> very good at catching these.

Ah I see - perhaps that explains why neither Janet nor I could
recreate the problem that you were hitting so easily. So we
should probably try running with CONFIG_DEBUG_SLAB and
CONFIG_DEBUG_PAGEALLOC as well.

>
> I updated your AIO fallback patch plus your AIO race plus I fixed
> the bio_count decrement fix. This patch has all three fixes and
> it is working for me.
>
> I fixed the bio_count race, by changing bio_list_lock into bio_lock
> and using that for all the bio fields. I changed bio_count and
> bios_in_flight from atomics into int. They are now proctected by
> the bio_lock. I fixed the race, by in finished_one_bio() by
> leaving the bio_count at 1 until after the dio_complete()
> and then do the bio_count decrement and wakeup holding the bio_lock.
>
> Take a look, give it a try, and let me know what you think.

I had been trying a slightly different kind of fix -- appended is
the updated version of the patch I last posted. It uses the bio_list_lock
to protect the dio->waiter field, which finished_one_bio sets back
to NULL after it has issued the wakeup; and the code that waits for
i/o to drain out checks the dio->waiter field instead of bio_count.
This might not seem very obvious given the nomenclature of the
bio_list_lock, so I was holding back wondering if it could be
improved.

Your approach looks clearer in that sense -- its pretty unambiguous
about what lock protects what fields. The only thing that bothers me (and
this is what I was trying to avoid in my patch) is the increased
use of spin_lock_irq 's (overhead of turning interrupts off and on)
instead of simple atomic inc/dec in most places.

Thoughts ?

Regards
Suparna

--
Suparna Bhattacharya (suparna@in.ibm.com)
Linux Technology Center
IBM Software Labs, India

------------------------------------

Don't access dio fields if its possible that the dio could
already have been freed asynchronously during i/o completion.
The dio->bio_list_lock protects the dio->waiter field as in
the case of synchronous i/o.

--- pure-mm3/fs/direct-io.c 2003-11-24 13:00:33.000000000 +0530
+++ linux-2.6.0-test9-mm3/fs/direct-io.c 2003-11-25 14:08:26.000000000 +0530
@@ -231,8 +231,17 @@
aio_complete(dio->iocb, dio->result, 0);
kfree(dio);
} else {
- if (dio->waiter)
- wake_up_process(dio->waiter);
+ struct task_struct *waiter;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dio->bio_list_lock, flags);
+ waiter = dio->waiter;
+ if (waiter) {
+ dio->waiter = NULL;
+ wake_up_process(waiter);
+ }
+ spin_unlock_irqrestore(&dio->bio_list_lock,
+ flags);
}
}
}
@@ -994,26 +1004,35 @@
* reflect the number of to-be-processed BIOs.
*/
if (dio->is_async) {
- if (ret == 0)
- ret = dio->result;
- if (ret > 0 && dio->result < dio->size && rw == WRITE) {
+ int should_wait = 0;
+
+ if (dio->result < dio->size && rw == WRITE) {
dio->waiter = current;
+ should_wait = 1;
}
+ if (ret == 0)
+ ret = dio->result;
finished_one_bio(dio); /* This can free the dio */
blk_run_queues();
- if (dio->waiter) {
+ if (should_wait) {
+ unsigned long flags;
/*
* Wait for already issued I/O to drain out and
* release its references to user-space pages
* before returning to fallback on buffered I/O
*/
+ spin_lock_irqsave(&dio->bio_list_lock, flags);
set_current_state(TASK_UNINTERRUPTIBLE);
- while (atomic_read(&dio->bio_count)) {
+ while (dio->waiter) {
+ spin_unlock_irqrestore(&dio->bio_list_lock,
+ flags);
io_schedule();
set_current_state(TASK_UNINTERRUPTIBLE);
+ spin_lock_irqsave(&dio->bio_list_lock, flags);
}
set_current_state(TASK_RUNNING);
- dio->waiter = NULL;
+ spin_unlock_irqrestore(&dio->bio_list_lock, flags);
+ kfree(dio);
}
} else {
finished_one_bio(dio);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2005-03-22 13:58    [W:0.065 / U:22.776 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site