ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests
PATCH 2/6 ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests
Such requests should be failed with -EIO (like all other requests in this function) instead of being completed successfully.
---
Untested, you may try pinging Borislav and/or Tejun about possible testing if you would like to verify the patch before applying.
drivers/ide/ide-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ide/ide-io.c +1 -1
@@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, strif ((media == ide_floppy || media == ide_tape) && drv_req) { rq->errors =0 ; -ide_complete_rq (drive,0 ,blk_rq_bytes (rq)); +ide_complete_rq (drive, -EIO ,blk_rq_bytes (rq)); }else {if (media == ide_tape) rq->errors =IDE_DRV_ERROR_GENERAL ;
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:26:06 +0200
Quoted 10 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: fixide_kill_rq()for special ide-{floppy,tape} driver requestsSuch requests should be failed with
-EIO(like all other requests in this function) instead of being completed successfully.Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>---
Untested, you may try pinging Borislav and/or Tejun about possible testing if you would like to verify the patch before applying.
This must be tested in some way. I can see this potentially breaking something.
Especially this is true because ide_complete_rq() does it's "complete whole request right now" logic for error <= 0.
Borislov/Tejun, can either of you test this code path with this change applied? I'd very much appreciate it.
Thanks!
Quoted 16 lines
drivers/ide/ide-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/drivers/ide/ide-io.c
===================================================================drivers/ide/ide-io.c +1
-1@@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, strif ((media == ide_floppy || media == ide_tape) && drv_req) { rq->errors =0 ; -ide_complete_rq (drive,0 ,blk_rq_bytes (rq)); +ide_complete_rq (drive, -EIO ,blk_rq_bytes (rq)); }else {if (media == ide_tape) rq->errors =IDE_DRV_ERROR_GENERAL ;
Quoted 14 lines · On Wednesday 24 June 2009 01:16:11 David Miller wrote:
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:26:06 +0200Quoted 10 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: fixide_kill_rq()for special ide-{floppy,tape} driver requestsSuch requests should be failed with
-EIO(like all other requests in this function) instead of being completed successfully.Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>---
Untested, you may try pinging Borislav and/or Tejun about possible testing if you would like to verify the patch before applying.
Quoted 5 lines
This must be tested in some way. I can see this potentially breaking something.
Especially this is true because
ide_complete_rq()does it's "complete whole request right now" logic for error <= 0.
/*
* if failfast is set on a request, override number of sectors
* and complete the whole request right now
*/
if (blk_noretry_request(rq) && error <= 0)
nr_bytes = blk_rq_sectors(rq) << 9;It is a historical leftover from the days that we did partial request completions. Please notice that the patch doesn't affect this chunk.
Quoted 22 lines
Borislov/Tejun, can either of you test this code path with this change applied? I'd very much appreciate it.
Thanks!
Quoted 16 lines
drivers/ide/ide-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/drivers/ide/ide-io.c
===================================================================drivers/ide/ide-io.c +1
-1@@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, strif ((media == ide_floppy || media == ide_tape) && drv_req) { rq->errors =0 ; -ide_complete_rq (drive,0 ,blk_rq_bytes (rq)); +ide_complete_rq (drive, -EIO ,blk_rq_bytes (rq)); }else {if (media == ide_tape) rq->errors =IDE_DRV_ERROR_GENERAL ;
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:26:06 +0200
drivers/ide/ide-io.c void ide_kill_rq(ide_drive_t *drive, str · line 158
Quoted 9 lines
drivers/ide/ide-io.c +1
-1@@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, strif ((media == ide_floppy || media == ide_tape) && drv_req) { rq->errors =0 ; -ide_complete_rq (drive,0 ,blk_rq_bytes (rq)); +ide_complete_rq (drive, -EIO ,blk_rq_bytes (rq)); }else {if (media == ide_tape) rq->errors =IDE_DRV_ERROR_GENERAL ;
I've done some research and this logic of returning "0" appears to be intentional.
It keeps the block layer from printing the "I/O error" kernel log message during completion of the request.
IDE tape as one example, seems to have it's own system of passing errors back up to the special command completion, via rq->errors and IDE_DRV_ERROR_GENERAL.
See idetape_queue_rw_tail() and ide_tape_callback() for example.
IDE floppy has similar pieces of logic, and possibly similar desires wrt. emission of the block layer I/O error log message during special requests.
When something sticks out like an eyesore (as this -EIO thing does) and seems to make no sense at all, there often is some obscure reason.
Quoted 14 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:26:06 +0200Quoted 10 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: fixide_kill_rq()for special ide-{floppy,tape} driver requestsSuch requests should be failed with
-EIO(like all other requests in this function) instead of being completed successfully.Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>---
Untested, you may try pinging Borislav and/or Tejun about possible testing if you would like to verify the patch before applying.
Quoted 8 lines
This must be tested in some way. I can see this potentially breaking something.
Especially this is true because
ide_complete_rq()does it's "complete whole request right now" logic for error <= 0.Borislov/Tejun, can either of you test this code path with this change applied? I'd very much appreciate it.
Yep, I'll give the whole series a whirl in a while.
Signature · 2 lines
Regards/Gruss,
Boris.Quoted 16 lines · On Wednesday 24 June 2009 08:49:29 David Miller wrote:
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:26:06 +0200drivers/ide/ide-io.c void ide_kill_rq(ide_drive_t *drive, str · line 158
Quoted 9 lines
drivers/ide/ide-io.c +1
-1@@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, strif ((media == ide_floppy || media == ide_tape) && drv_req) { rq->errors =0 ; -ide_complete_rq (drive,0 ,blk_rq_bytes (rq)); +ide_complete_rq (drive, -EIO ,blk_rq_bytes (rq)); }else {if (media == ide_tape) rq->errors =IDE_DRV_ERROR_GENERAL ;I've done some research and this logic of returning "0" appears to be intentional.
Quoted 2 lines
It keeps the block layer from printing the "I/O error" kernel log message during completion of the request.
It would be pretty illogical behavior for driver to intentionally not let user know about the failed requests..
Quoted 3 lines
IDE tape as one example, seems to have it's own system of passing errors back up to the special command completion, via
rq->errorsandIDE_DRV_ERROR_GENERAL.
Please look at the patch/code:
rq->errors = 0;
- ide_complete_rq(drive, 0, blk_rq_bytes(rq));
+ ide_complete_rq(drive, -EIO, blk_rq_bytes(rq));
and notice rq->errors line.
Quoted 9 lines
See
idetape_queue_rw_tail()andide_tape_callback()for example.IDE floppy has similar pieces of logic, and possibly similar desires wrt. emission of the block layer I/O error log message during special requests.
When something sticks out like an eyesore (as this
-EIOthing does) and seems to make no sense at all, there often is some obscure reason.
The obscure reasons is just the fact that both ide-floppy and ide-tape had a they own duplicated/buggy request completion routines.
While they were being unified the whole bunch of similar class of bugs were fixed so I agree that there may still be more issues to deal with there.
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 24 Jun 2009 12:09:53 +0200
Quoted 6 lines
The obscure reasons is just the fact that both ide-floppy and ide-tape had a they own duplicated/buggy request completion routines.
While they were being unified the whole bunch of similar class of bugs were fixed so I agree that there may still be more issues to deal with there.
I see, the ->pc_callback() of these things set rq->errors and then ide_pc_intr() happily overrides whatever was set there, either on it's own or via ide_complete_rq().
I'll look over this some more while I wait for the testing results from Borislav.
Quoted 10 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 24 Jun 2009 12:09:53 +0200Quoted 6 lines
The obscure reasons is just the fact that both ide-floppy and ide-tape had a they own duplicated/buggy request completion routines.
While they were being unified the whole bunch of similar class of bugs were fixed so I agree that there may still be more issues to deal with there.
Quoted 6 lines
I see, the ->pc_callback() of these things set
rq->errorsand thenide_pc_intr()happily overrides whatever was set there, either on it's own or viaide_complete_rq().I'll look over this some more while I wait for the testing results from Borislav.
Ok, let's see now, I did some testing but can't seem to hit that particular path because it is taken relatively rarely - it can be reached from start_request() and ide_atapi_error() via ide_error for ide-floppy and ide-tape.
If my code staring doesn't mislead me, the start_request()-one has to happen during an IDE reset operation, while we're polling for reset completion _and_ when drive->failures has reached drive->max_failures since this is the only site the drive->failures counter is increased.
The ide_atapi_error() cannot happen because (!blk_fs_request(rq)) requests are being completed in ide_error() with the respective error status.
It is possible that I've missed something so feel free to correct me and/or suggest a different testing approach.
Thanks.
Signature · 2 lines
Regards/Gruss,
Boris.From: Borislav Petkov <[email protected]>
Date: Wed, 24 Jun 2009 19:44:11 +0200
Quoted 28 lines · David Miller
Quoted 10 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 24 Jun 2009 12:09:53 +0200Quoted 6 lines
The obscure reasons is just the fact that both ide-floppy and ide-tape had a they own duplicated/buggy request completion routines.
While they were being unified the whole bunch of similar class of bugs were fixed so I agree that there may still be more issues to deal with there.
Quoted 6 lines
I see, the ->pc_callback() of these things set
rq->errorsand thenide_pc_intr()happily overrides whatever was set there, either on it's own or viaide_complete_rq().I'll look over this some more while I wait for the testing results from Borislav.
Ok, let's see now, I did some testing but can't seem to hit that particular path because it is taken relatively rarely - it can be reached from
start_request()andide_atapi_error()viaide_errorfor ide-floppy and ide-tape.If my code staring doesn't mislead me, the start_request()-one has to happen during an IDE reset operation, while we're polling for reset completion _and_ when
drive->failureshas reacheddrive->max_failuressince this is the only site thedrive->failurescounter is increased.
Quoted 6 lines
The
ide_atapi_error()cannot happen because (!blk_fs_request(rq)) requests are being completed inide_error()with the respective error status.It is possible that I've missed something so feel free to correct me and/or suggest a different testing approach.
Ok, thanks Borislav.
My intention is to apply patches #2 and #3, thanks everyone!