Messages in this thread Patch in this message | | | Date | Wed, 26 Nov 2003 13:25:13 +0530 | From | Suparna Bhattacharya <> | Subject | Re: [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/
| |