| Message ID | 200906232326.06830.bzolnier@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Date: Tue, 23 Jun 2009 23:26:06 +0200 > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > 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. > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > --- > 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! > drivers/ide/ide-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: b/drivers/ide/ide-io.c > =================================================================== > --- a/drivers/ide/ide-io.c > +++ b/drivers/ide/ide-io.c > @@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, str > > if ((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; -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 24 June 2009 01:16:11 David Miller wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Date: Tue, 23 Jun 2009 23:26:06 +0200 > > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > 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. > > > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > --- > > 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. /* * 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. > Borislov/Tejun, can either of you test this code path with this > change applied? I'd very much appreciate it. > > Thanks! > > > drivers/ide/ide-io.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Index: b/drivers/ide/ide-io.c > > =================================================================== > > --- a/drivers/ide/ide-io.c > > +++ b/drivers/ide/ide-io.c > > @@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, str > > > > if ((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; > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Date: Tue, 23 Jun 2009 23:26:06 +0200 > @@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, str > > if ((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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 23, 2009 at 04:16:11PM -0700, David Miller wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Date: Tue, 23 Jun 2009 23:26:06 +0200 > > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > 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. > > > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > --- > > 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. Yep, I'll give the whole series a whirl in a while.
On Wednesday 24 June 2009 08:49:29 David Miller wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Date: Tue, 23 Jun 2009 23:26:06 +0200 > > > @@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, str > > > > if ((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. It would be pretty illogical behavior for driver to intentionally not let user know about the failed requests.. > 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. 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. > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Date: Wed, 24 Jun 2009 12:09:53 +0200 > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 24, 2009 at 03:39:09AM -0700, David Miller wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Date: Wed, 24 Jun 2009 12:09:53 +0200 > > > 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. 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.
From: Borislav Petkov <petkovbb@googlemail.com> Date: Wed, 24 Jun 2009 19:44:11 +0200 > On Wed, Jun 24, 2009 at 03:39:09AM -0700, David Miller wrote: >> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> >> Date: Wed, 24 Jun 2009 12:09:53 +0200 >> >> > 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. > > 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. Ok, thanks Borislav. My intention is to apply patches #2 and #3, thanks everyone! -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: b/drivers/ide/ide-io.c =================================================================== --- a/drivers/ide/ide-io.c +++ b/drivers/ide/ide-io.c @@ -152,7 +152,7 @@ void ide_kill_rq(ide_drive_t *drive, str if ((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;