IDE issues with "choose_drive"

IDE issues with "choose_drive"

[Benjamin Herrenschmidt]

There seem to be a certain amount of problems with ide_do_request() and
more specifically choose_drive() to pick which drive to service, when
more than one drive is on a given hwgroup (typically, when you have a
slave drive)

The first one is the one I'm trying to fix, it's basically a hang on
wakeup from sleep. What happens is that both drives are blocked
(suspended, drive->blocked is set). Their IO queues contains some
requests that haven't been serviced yet. We receive the resume()
callback for one of them. We react by inserting a wakeup request at the
head of the queue and waiting for it to complete. However, when we reach
ide_do_request(), choose_drive() may return the other drive (the one
that is still sleeping). In this case, we hit the test for blocked queue
and just break out of the loop. We end up never servicing the other
drive queue which is the one we are trying to wakeup, thus we hang.

In general, that uncovers a serie of problems with that code. That is,
the 2 other cases where we "break" out of the loop may trigger a similar
problem where we do not service the "other" drive, and thus unless
something else happens to "kick" the hwgroup, a request pending on the
other drive will be stuck there and the machine may hang.

In a similar vein, if we hit the codepath that does 
ide_stall_queue() (which is meant as doing some fairness by blocking the
slave for some time) we may end up also "missing" the opportunity to
service at all on the next pass, thus hanging forever.  

Also, I think there may be another problem with choose_drive() itself,
though I'm not 100% sure, with the code that tests blk_queue_flushing()
since we do the test on hwgroup->drive only, and thus we never test that
for the slave drive.

I'm not sure what is the best way to fix those issues except by
completely rewriting that code. I was thinking about turning the "break"
into a "continue" in the PM case to service the "other" drive but
nothing guarantees that I'll make any forward progress since
choose_drive() may just return the same drive over and over (since the
queue is not serviced).

Ben.

Re: IDE issues with "choose_drive"

[Benjamin Herrenschmidt]

> The first one is the one I'm trying to fix, it's basically a hang on
> wakeup from sleep. What happens is that both drives are blocked
> (suspended, drive->blocked is set). Their IO queues contains some
> requests that haven't been serviced yet. We receive the resume()
> callback for one of them. We react by inserting a wakeup request at the
> head of the queue and waiting for it to complete. However, when we reach
> ide_do_request(), choose_drive() may return the other drive (the one
> that is still sleeping). In this case, we hit the test for blocked queue
> and just break out of the loop. We end up never servicing the other
> drive queue which is the one we are trying to wakeup, thus we hang.

Oh, and here's the ugly workaround beeing tested by the users who are
having the problem so far. Not really a proper fix though...

--- linux-work.orig/drivers/ide/ide-io.c	2005-09-22 14:06:31.000000000 +1000
+++ linux-work/drivers/ide/ide-io.c	2005-10-06 10:49:53.000000000 +1000
@@ -1101,6 +1101,7 @@
 	ide_hwif_t	*hwif;
 	struct request	*rq;
 	ide_startstop_t	startstop;
+	int             loops = 0;
 
 	/* for atari only: POSSIBLY BROKEN HERE(?) */
 	ide_get_lock(ide_intr, hwgroup);
@@ -1153,6 +1154,7 @@
 			/* no more work for this hwgroup (for now) */
 			return;
 		}
+	again:
 		hwif = HWIF(drive);
 		if (hwgroup->hwif->sharing_irq &&
 		    hwif != hwgroup->hwif &&
@@ -1192,8 +1194,14 @@
 		 * though. I hope that doesn't happen too much, hopefully not
 		 * unless the subdriver triggers such a thing in its own PM
 		 * state machine.
+		 *
+		 * We count how many times we loop here to make sure we service
+		 * all drives in the hwgroup without looping for ever
 		 */
 		if (drive->blocked && !blk_pm_request(rq) && !(rq->flags & REQ_PREEMPT)) {
+			drive = drive->next ? drive->next : hwgroup->drive;
+			if (loops++ < 4 && !blk_queue_plugged(drive->queue))
+				goto again;
 			/* We clear busy, there should be no pending ATA command at this point. */
 			hwgroup->busy = 0;
 			break;

Re: IDE issues with "choose_drive"

[Benjamin Herrenschmidt]

On Thu, 2005-10-06 at 11:02 +1000, Benjamin Herrenschmidt wrote:
> > The first one is the one I'm trying to fix, it's basically a hang on
> > wakeup from sleep. What happens is that both drives are blocked
> > (suspended, drive->blocked is set). Their IO queues contains some
> > requests that haven't been serviced yet. We receive the resume()
> > callback for one of them. We react by inserting a wakeup request at the
> > head of the queue and waiting for it to complete. However, when we reach
> > ide_do_request(), choose_drive() may return the other drive (the one
> > that is still sleeping). In this case, we hit the test for blocked queue
> > and just break out of the loop. We end up never servicing the other
> > drive queue which is the one we are trying to wakeup, thus we hang.
> 
> Oh, and here's the ugly workaround beeing tested by the users who are
> having the problem so far. Not really a proper fix though...

No reply ... it's a bit urgent as it may bite any system trying to
suspend with a slave IDE disk at least (not including the other possible
problems I've spotted  with this code).

I'm tempted to just send my workaround patch to Linus & Andrew (might
still make it into 2.6.14). That would at least fix the bug with resume
from sleep. What do you think ?

Ben.

Re: IDE issues with "choose_drive"

[Bartlomiej Zolnierkiewicz]

On 10/8/05, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Thu, 2005-10-06 at 11:02 +1000, Benjamin Herrenschmidt wrote:
> > > The first one is the one I'm trying to fix, it's basically a hang on
> > > wakeup from sleep. What happens is that both drives are blocked
> > > (suspended, drive->blocked is set). Their IO queues contains some
> > > requests that haven't been serviced yet. We receive the resume()
> > > callback for one of them. We react by inserting a wakeup request at the
> > > head of the queue and waiting for it to complete. However, when we reach
> > > ide_do_request(), choose_drive() may return the other drive (the one
> > > that is still sleeping). In this case, we hit the test for blocked queue
> > > and just break out of the loop. We end up never servicing the other
> > > drive queue which is the one we are trying to wakeup, thus we hang.
> >
> > Oh, and here's the ugly workaround beeing tested by the users who are
> > having the problem so far. Not really a proper fix though...
>
> No reply ... it's a bit urgent as it may bite any system trying to
> suspend with a slave IDE disk at least (not including the other possible
> problems I've spotted  with this code).

It is a old problem from what I understand.
However it would still be nice to have it fixed for 2.6.14.

> I'm tempted to just send my workaround patch to Linus & Andrew (might
> still make it into 2.6.14). That would at least fix the bug with resume
> from sleep. What do you think ?

It seems we need internal ide_dev_do_request(ide_drive_t *, int)
which will explicitly state which device we want to service as I see
no sane way to fix the problem in choose_drive().

Your workaround is OK for 2.6.14 given that you will document it
now and later fix it properly for 2.6.15.

Bartlomiej

Re: IDE issues with "choose_drive"

[Benjamin Herrenschmidt]

> It seems we need internal ide_dev_do_request(ide_drive_t *, int)
> which will explicitly state which device we want to service as I see
> no sane way to fix the problem in choose_drive().

Not only that, but if you read my blurb, I wonder how even the
non-targetted case can work properly if we ever hit a couple of the code
path in there that either early exit because the elevator returned no
request or the case where we "sleep" a drive to give more time to the
other... I have the feeling that we may "miss" an opportunity to servive
a drive, and thus this drive will stick around with a pending request
not beeing serviced... I reckon those are corner cases, but I feel the
whole thing need some serious revisiting.

> Your workaround is OK for 2.6.14 given that you will document it
> now and later fix it properly for 2.6.15.

Ok. Well, I'm not sure what is the right fix at the moment given the
other issues I described above, but I'm definitely up to doign a proper
fix for 2.6.15 with your help ;)

Ben.