linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>
Cc: Jens Axboe <axboe@suse.de>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	list linux-ide <linux-ide@vger.kernel.org>
Subject: IDE issues with  "choose_drive"
Date: Thu, 06 Oct 2005 10:36:59 +1000	[thread overview]
Message-ID: <1128559019.22073.19.camel@gaston> (raw)

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.



             reply	other threads:[~2005-10-06  0:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-10-06  0:36 Benjamin Herrenschmidt [this message]
2005-10-06  1:02 ` IDE issues with "choose_drive" Benjamin Herrenschmidt
2005-10-08  1:15   ` Benjamin Herrenschmidt
2005-10-08  8:29     ` Bartlomiej Zolnierkiewicz
2005-10-08 22:18       ` Benjamin Herrenschmidt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1128559019.22073.19.camel@gaston \
    --to=benh@kernel.crashing.org \
    --cc=B.Zolnierkiewicz@elka.pw.edu.pl \
    --cc=axboe@suse.de \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).