Messages in this thread | | | Date | Wed, 15 May 2002 12:37:48 +0100 | From | Neil Conway <> | Subject | Re: [PATCH] 2.5.15 IDE 61 |
| |
Jens Axboe wrote: > On Tue, May 14 2002, Neil Conway wrote: > > > To really serialize operations the queue _must_ be shared with whoever > > > requires serialiation. > > Why will this help? The hardware can still be doing DMA on hda while > > the queue's request_fn is called quite legitimately for a hdb request - > > and the IDE code MUST impose the serialization here to avoid hitting the > > cable with commands destined for hdb. (For example, by waiting for > > !channel->busy.) > > Current IDE code leaves a request on the list until it has completed > (this is ignoring TCQ of course), so there's no way that you could start > serving a second request before the first one completes.
I didn't understand why this was the case, so I've been reading source code (and sleeping for a few minutes too ;-)). Wow, is it hard for a newbie to follow - if I didn't know better I'd swear the kernel was obfuscated.
I _think_ I now understand what's going on here: you guys already know this stuff but perhaps others can learn from my slog through the code (?) (and/or spot the dumb mistakes). (I did notice what could be a potential problem too, skip to the end if you want to find it.)
On a "totally" idle system, if a process decides to read from a file, the sequence appears to be (with minor simplifications for clarity):
sys_read(), file->fops->read(), usually then into generic_file_read(), page_cache_readahead(), and then the important one: do_page_cache_readahead(). This does a couple of seriously important things. (The name implies it's only used for readahead but the comments show this isn't the case (if one reads them!)).
Firstly, do_page_cache_readahead() invokes the call chain: a_ops->readpage(), block_read_full_page(), submit_bh(), submit_bio(), generic_make_request() (this one finds the right queue for the I/O) and down into __make_request_fcn(q,bio). This routine is also key: it adds requests to the device queue, and a little more: if the queue is empty it "plugs" the device (before adding the request), ("plugging" the device refers to preventing I/O while a request queue is populated), and also queues a task to the tq_disk task-queue which will unplug the device at some later time. The "unplug" routine is central: see below.
Control returns to do_page_cache_readahead() after the call to readpage(). It then calls run_task_queue(&tq_disk), thus starting the call chain: generic_unplug_device(),..., q->request_fn(). This is the end of the line for the block-layer: request_fn() is part of the device code - for IDE it's do_ide_request().
So this call to request_fn()/do_ide_request() is the first time the IDE code is really involved in the loop.
The key bit to notice here is that request_fn() is only called if the device was plugged. This in turn only happens (well almost, see below) if the queue was empty. Thus one concludes that if do_ide_request() is busy servicing requests and thus the queue is non-empty, the block layer will never again call do_ide_request(); it will be allowed to get on with things in its own time.
This I believe, is what you meant Jens when you said "so there's no way that you could start serving a second request before the first one completes". Am I right so far?
Now a possible fly in the ointment: __make_request_fcn() will actually plug a non-empty queue if BIO_RW_BARRIER is set. This appears right now to be impossible because I can't find any code in the entire kernel that uses bio_barrier() or any similar construct. But if it's a work in progress and this bit is set one day, then it seems to me that the block layer could plug a non-empty queue, and then subsequently any call to run_task_queue(&tq_disk) would call the block-device's request_fn(). This would violate the assumption that request_fn() is never called twice without the queue emptying in between. The current IDE code would survive this because it checks for hwgroup->busy.
Anyway, IFF the BARRIER stuff is not an issue, then I guess the block-layer really can do all the serialization we need if we set things up right. I now probably have to retract my assertions that we vitally need hwgroup->busy (or equiv) because there really does appear to be no route into request_fn() from the block layer other than if the queue was empty... In the real world though, as you suggested, it's probably worth having.
all the best, Neil
PS: Errors and Omissions Expected. - 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/
| |