On Friday 11 June 2004 11:48 am, Kevin Corry wrote: > Upon further review of dm-io.c::sync_io(), I have a couple more comments. > > First, I'm not certain whether the signal handling needs to be there or > not. Joe, perhaps you can comment on that. If we don't need it, then we can > use Dave's suggestion and just get rid of the signal_pending() call. If we > do need it, then instead of declaring a struct io on the stack in > sync_io(), we need to allocate one (async_io() already does this using a > mempool) and use dec_count() to deallocate it. This ought to prevent the > stack corruption Dave described. I'll put together a patch in a bit to > demonstrate how this would work. After talking to Alasdair a bit, it seems like there still might be a reason for using the interruptible wait in sync_io(). The point he brought up was that if you are using dm-io to do I/O to another DM device, and that DM device is suspended, then the I/O may never complete if the device doesn't get unsuspended (for some reason). In this case, you'd like to be able to interrupt the process that's waiting for that I/O so it isn't stuck in D-state forever. So this means we need to allocate a struct io from the mempool in sync_io() instead of declaring it on the stack. Here's a patch that should accomplish this. It ain't pretty, but I think it gets the job done. It requires some changes in dec_count(), and I've also replaced the io_schedule() loop with a call to wait_event_interruptible(). Now everyone take a look and tell me how ugly this is. :) And if anyone can think of something more elegant, please let me know. -- Kevin Corry kevcorry@xxxxxxxxxx http://evms.sourceforge.net/ --- diff/drivers/md/dm-io.c 2004-06-11 12:16:24.729145072 +0000 +++ source/drivers/md/dm-io.c 2004-06-11 13:56:02.626366560 +0000 @@ -317,17 +317,19 @@ set_bit(region, &io->error); if (atomic_dec_and_test(&io->count)) { - if (io->sleeper) - wake_up_process(io->sleeper); + int r = io->error; + io_notify_fn fn = io->callback; + void *context = io->context; - else { - int r = io->error; - io_notify_fn fn = io->callback; - void *context = io->context; + mempool_free(io, _io_pool); - mempool_free(io, _io_pool); + if (fn) { fn(r, context); } + } else { + struct task_struct *sleeper = io->sleeper; + if (sleeper) + wake_up_process(sleeper); } } @@ -535,33 +537,34 @@ static int sync_io(unsigned int num_regions, struct io_region *where, int rw, struct dpages *dp, unsigned long *error_bits) { - struct io io; + DECLARE_WAIT_QUEUE_HEAD(wq); + struct io *io; + int rc; if (num_regions > 1 && rw != WRITE) { return -EIO; } - io.error = 0; - atomic_set(&io.count, 1); /* see dispatch_io() */ - io.sleeper = current; - - dispatch_io(rw, num_regions, where, dp, &io, 1); - - while (1) { - set_current_state(TASK_UNINTERRUPTIBLE); - - if (!atomic_read(&io.count) || signal_pending(current)) - break; - - io_schedule(); + io = mempool_alloc(_io_pool, GFP_NOIO); + memset(io, 0, sizeof(*io)); + atomic_set(&io->count, 2); /* One for us, one for dispatch_io(). */ + io->sleeper = current; + + dispatch_io(rw, num_regions, where, dp, io, 1); + + wait_event_interruptible(wq, (atomic_read(&io->count) == 1)); + + if (atomic_read(&io->count) > 1) { + rc = -EINTR; + } else { + *error_bits = io->error; + rc = io->error ? -EIO : 0; } - set_current_state(TASK_RUNNING); - if (atomic_read(&io.count)) - return -EINTR; + io->sleeper = NULL; + dec_count(io, 0, rc); - *error_bits = io.error; - return io.error ? -EIO : 0; + return rc; } static int async_io(unsigned int num_regions, struct io_region *where, int rw,