[patch 2/6] 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

[Bartlomiej Zolnierkiewicz]

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.

 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;

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[David Miller]

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;

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[Bartlomiej Zolnierkiewicz]

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;
> 

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[David Miller]

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.

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[Bartlomiej Zolnierkiewicz]

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.

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[David Miller]

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.

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Re: [patch 2/6] ide: fix ide_kill_rq() for special ide-{floppy,tape} driver requests

[David Miller]

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!