Re: [PATCH] ide: don't execute the next queued command from the hard-IRQ context

[Elias Oltmanns <eo@nebensachen.de>]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> On Sunday 12 October 2008, Elias Oltmanns wrote:
>> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>
>> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> > Subject: [PATCH] ide: don't execute the next queued command from the hard-IRQ context
>> >
>> > * Tell the block layer that we are not done handling requests by using
>> >   blk_plug_device() in ide_do_request() (request handling function)
>> >   and ide_timer_expiry() (timeout handler) if the queue is not empty.
>> >
>> > * Remove optimization which directly calls ide_do_request() for the next
>> >   queued command from the ide_intr() (IRQ handler) and ide_timer_expiry().
>> >
>> > * Remove no longer needed IRQ masking from ide_do_request() - in case of
>> >   IDE ports needing serialization disable_irq_nosync()/enable_irq() was
>> >   used for the (possibly shared) IRQ of the other IDE port.
>> >
>> > * Put the misplaced comment in the right place in ide_do_request().
>> >
>> > * Drop no longer needed 'int masked_irq' argument from ide_do_request().
>> >
>> > * Merge ide_do_request() into do_ide_request().
>> >
>> > * Remove no longer needed IDE_NO_IRQ define.
>> >
>> > While at it:
>> >
>> > * Don't use HWGROUP() macro in do_ide_request().
>> >
>> > * Use __func__ in ide_intr().
>> >
>> > This patch reduces IRQ hadling latency for IDE and improves the system-wide
>> > handling of shared IRQs (which should result in more timeout resistant and
>> > stable IDE systems).  It also makes it possible to do some further changes
>> > later (i.e. replace some busy-waiting delays with sleeping equivalents).
>> >
>> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> > ---
[...]
>> > Index: b/drivers/ide/ide-io.c
>> > ===================================================================
>> > --- a/drivers/ide/ide-io.c
>> > +++ b/drivers/ide/ide-io.c
[...]
>> >  		startstop = start_request(drive, rq);
>> >  		spin_lock_irq(&hwgroup->lock);
>> > -		if (masked_irq != IDE_NO_IRQ && hwif->irq != masked_irq)
>> > -			enable_irq(hwif->irq);
>> > -		if (startstop == ide_stopped)
>> > +
>> > +		if (startstop == ide_stopped) {
>> >  			hwgroup->busy = 0;
>> > +			goto plug_device;
>> 
>> This goto statement is wrong. Simply set ->busy to zero and be done with
>> it. This way, the loop will start again and either elv_next_request()
>> returns NULL, in which case the queue need not be plugged, or the next
>> request will be processed immediately, which is exactly what we want.
>
> The problem is that the next loop can choose the different drive than
> the current one so we can end up with the situation where we will "lose"
> the blk_plug_device() call.
>
> I fixed it by just inlining "plug_device" code for now.

Right, I had missed that. Still, I'm not really convinced yet that this
is the right way  to handle things. In fact, I believe that the role of
choose_drive() has changed since it is now called directly from the
->request_fn() and it should probably be rewritten and renamed along the
way. However, this needs further discussion and the issue below has some
bearing on this too.

>
>> [...]
>> > @@ -1438,11 +1431,11 @@ irqreturn_t ide_intr (int irq, void *dev
>> >  	if (startstop == ide_stopped) {
>> >  		if (hwgroup->handler == NULL) {	/* paranoia */
>> >  			hwgroup->busy = 0;
>> > -			ide_do_request(hwgroup, hwif->irq);
>> > -		} else {
>> > -			printk(KERN_ERR "%s: ide_intr: huh? expected NULL handler "
>> > -				"on exit\n", drive->name);
>> > -		}
>> > +			if (!elv_queue_empty(drive->queue))
>> > +				blk_plug_device(drive->queue);
>> 
>> This is perhaps not exactly what you really want. It basically means
>> that there will be a delay (q->unplug_delay) after each command which
>> may well hurt I/O performance. Instead, I'd suggest something like the
>> following:
>> 
>> 	if (!elv_queue_empty(drive->queue))
>> 		blk_schedule_queue_run(drive->queue);
>> 
>> and a function
>> 
>> void blk_schedule_queue_run(struct request_queue *q)
>> {
>> 	blk_plug_device(q);
>> 	kblockd_schedule_work(&q->unplug_work);
>> }
>> 
>> in blk-core.c. This can also be used as a helper function in blk-core.c
>> itself.
>
> Care to make a patch adding such helper to blk-core.c?

Thinking about this a bit more, I'd like to raise this issue with Jens
and discuss it in some more generality.

Regards,

Elias