Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe <jens.axboe@oracle.com>]

On Wed, Jul 18 2007, Linus Torvalds wrote:
> 
> 
> On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > On Tuesday 17 July 2007, Giacomo Catenazzi wrote:
> > > 
> > > last git changes to git give me the following error (repead very quickly):
> > > 
> > > sector 14657019, nr/cmr 0/0
> > > bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
> > > ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8
> > 
> > ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well
> 
> Hmm. I started wondering about this. 
> 
> Sure, ide-disk doesn't like non-fs requests, but I'm wondering why the 
> error apparently keeps repeating? We should error out once, and that 
> should be it.
> 
> So I'm wondering whether the
> 
> 	ide_end_request(drive, 0, 0);
> 	return ide_stopped;
> 
> is the real bug.
> 
> The thing is, non-fs requests won't have nr_sectors set, so the 0 will 
> stay as a zero (the IDE layer has some "turn zero into current sector 
> number", but that doesn't work for commands that aren't sector-based!), 
> and as a result, we'll "complete" zero bytes, and the bio will stay 
> active!
> 
> So the code should probably at the _least_ say that it's finished one 
> whole sector, and it would probably be much better to make it be based on 
> "rq->data_len", which should be reliable.
> 
> Of course, since the "ide_end_request()" takes the data in sectors, but 
> the request layer does it in bytes, we have to round the thing up and the 
> ide_end_request() thing will turn it into too many bytes, but that's ok - 
> "end_that_request_first()" doesn't care if we complete "too many" bytes.
> 
> So a diff something like this is, I think, the right thing to do, and 
> should make the IDE disk driver handle non-fs requests gracefully (it will 
> *fail* them, of course, but that's another issue).
> 
> Bartlomiej? Am I missing soemthing?

I think your analysis is pretty good, however you'd probably want to
incorporate that direct in ide_end_request(). Passing in 0 means end the
first chunk of the request, for pc type requests that should just be the
full thing as we cannot just move forward like in a read/write request.

Better still would be to make __ide_end_request() take a byte count
instead and use end_that_request_chunk(). Then you can get rid of the
rounding as well.

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c5b5011..71b317a 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -114,8 +114,12 @@ int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 	spin_lock_irqsave(&ide_lock, flags);
 	rq = HWGROUP(drive)->rq;
 
-	if (!nr_sectors)
-		nr_sectors = rq->hard_cur_sectors;
+	if (!nr_sectors) {
+		if (!blk_fs_request(rq))
+			nr_sectors = (rq->data_len + 511) >> 9;
+		else
+			nr_sectors = rq->hard_cur_sectors;
+	}
 
 	ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
 

-- 
Jens Axboe