Hi,
I have brought this issue once already in 2.5.70 context (see uppermost
post in http://marc.theaimsgroup.com/?t=105410790500005&r=1&w=2), but it
apparently slipped through. So I've decided to bring it up again, this
time describing [hopefully] better how does this *generic* problem with
ide-cd.c manifests itself. In the nutshell the problem is that [as it is
now] every failed SG_IO request is replayed second time without data
transfer. E.g. if WRITE(10) SCSI command fails with sense code X, ide-cd
immediately resends the command block descriptor one more time, this time
without programming for any associated I/O payload, which [normally]
results in *different* sense code. And the catch is that user-land gets
this second sense code instead of the real/original one. Suggested patch
overcomes this problem by immediately purging the failed SG_IO request
from the request queue.
P.S. Non-essential "rant." One can wonder how come the originally
discussed patch, the one which made to 2.5.71, was sufficient for DVD+RW
units, but not DVD-RW. Are commands sent to DVD-RW unit so to say bound
to fail? In a sense yes! Majority of DVD-RW units terminate WRITE
requests with "LONG WRITE IN PROGRESS" sense code wherever internal
buffer gets full. The idea behind this LUN behaviour is to minimize
IDE-bus monopolization periods. The sense code is commonly treated as
"estimate how long does it take to purge say 1/2 of internal buffer,
hibernate for that while and retry the failed WRITE command." Needless
to say that if original sense code is effectively discarded and replaced
by another code [apparently by "INVALID ADDRESS FOR WRITE"], it makes it
impossible to treat this essentially *non-error* condition appropriately
For whatever reason DVD+RW units don't terminate WRITE requests in same
manner and IDE-bus remains monopilized till there is buffer space
available to meet the outstanding WRITE request. Well, there're commands
which get replayed even in DVD+RW case, those are data-less commands,
most notably TEST UNIT READY, which are "safe" to replay.
Cheers. A.
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig Tue Jun 3 12:21:56 2003
+++ ./drivers/ide/ide-cd.c Thu Jun 5 22:26:56 2003
@@ -749,12 +749,17 @@
by transferring the semaphore from the packet
command request to the request sense request. */
+ rq->flags |= REQ_FAILED;
if ((stat & ERR_STAT) != 0) {
wait = rq->waiting;
rq->waiting = NULL;
+ if (rq->flags&REQ_BLOCK_PC) {
+ cdrom_queue_request_sense(drive, wait,
+ rq->sense, rq);
+ return 1; /* REQ_BLOCK_PC self-cares */
+ }
}
- rq->flags |= REQ_FAILED;
cdrom_end_request(drive, 0);
if ((stat & ERR_STAT) != 0)
@@ -1657,13 +1662,14 @@
dma = info->dma;
if (dma) {
info->dma = 0;
- if ((dma_error = HWIF(drive)->ide_dma_end(drive))) {
- printk("ide-cd: dma error\n");
- HWIF(drive)->ide_dma_off(drive);
- }
+ dma_error = HWIF(drive)->ide_dma_end(drive);
}
if (cdrom_decode_status(drive, 0, &stat)) {
+ if (stat&ERR_STAT)
+ { end_that_request_chunk(rq, 0, rq->data_len);
+ goto end_request; /* purge the whole thing... */
+ }
end_that_request_chunk(rq, 1, rq->data_len);
return ide_stopped;
}
@@ -1672,8 +1678,11 @@
* using dma, transfer is complete now
*/
if (dma) {
- if (dma_error)
+ if (dma_error) {
+ printk("ide-cd: dma error\n");
+ HWIF(drive)->ide_dma_off(drive);
return DRIVER(drive)->error(drive, "dma error", stat);
+ }
end_that_request_chunk(rq, 1, rq->data_len);
rq->data_len = 0;
On Tue, Jun 17 2003, Andy Polyakov wrote:
> Hi,
>
> I have brought this issue once already in 2.5.70 context (see uppermost
> post in http://marc.theaimsgroup.com/?t=105410790500005&r=1&w=2), but it
> apparently slipped through. So I've decided to bring it up again, this
> time describing [hopefully] better how does this *generic* problem with
> ide-cd.c manifests itself. In the nutshell the problem is that [as it is
> now] every failed SG_IO request is replayed second time without data
> transfer. E.g. if WRITE(10) SCSI command fails with sense code X, ide-cd
> immediately resends the command block descriptor one more time, this time
> without programming for any associated I/O payload, which [normally]
> results in *different* sense code. And the catch is that user-land gets
> this second sense code instead of the real/original one. Suggested patch
> overcomes this problem by immediately purging the failed SG_IO request
> from the request queue.
Patch looks fine, care to resend actually trying to follow the style in
the file in question?
--
Jens Axboe
> > ... In the nutshell the problem is that [as it is
> > now] every failed SG_IO request is replayed second time without data
> > transfer. ... Suggested patch
> > overcomes this problem by immediately purging the failed SG_IO request
> > from the request queue.
>
> Patch looks fine, care to resend actually trying to follow the style in
> the file in question?
Revised to my best ability for follow the coding style of file in
question. A:-)
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig Tue Jun 17 06:20:00 2003
+++ ./drivers/ide/ide-cd.c Thu Jun 19 15:56:39 2003
@@ -749,12 +749,17 @@
by transferring the semaphore from the packet
command request to the request sense request. */
+ rq->flags |= REQ_FAILED;
if ((stat & ERR_STAT) != 0) {
wait = rq->waiting;
rq->waiting = NULL;
+ if ((rq->flags & REQ_BLOCK_PC) != 0) {
+ cdrom_queue_request_sense(drive, wait,
+ rq->sense, rq);
+ return 1; /* REQ_BLOCK_PC self-cares */
+ }
}
- rq->flags |= REQ_FAILED;
cdrom_end_request(drive, 0);
if ((stat & ERR_STAT) != 0)
@@ -1657,13 +1662,14 @@
dma = info->dma;
if (dma) {
info->dma = 0;
- if ((dma_error = HWIF(drive)->ide_dma_end(drive))) {
- printk("ide-cd: dma error\n");
- HWIF(drive)->ide_dma_off(drive);
- }
+ dma_error = HWIF(drive)->ide_dma_end(drive);
}
if (cdrom_decode_status(drive, 0, &stat)) {
+ if ((stat & ERR_STAT) != 0) {
+ end_that_request_chunk(rq, 0, rq->data_len);
+ goto end_request; /* purge the whole thing... */
+ }
end_that_request_chunk(rq, 1, rq->data_len);
return ide_stopped;
}
@@ -1672,8 +1678,11 @@
* using dma, transfer is complete now
*/
if (dma) {
- if (dma_error)
+ if (dma_error) {
+ printk("ide-cd: dma error\n");
+ HWIF(drive)->ide_dma_off(drive);
return DRIVER(drive)->error(drive, "dma error", stat);
+ }
end_that_request_chunk(rq, 1, rq->data_len);
rq->data_len = 0;
On Thu, Jun 19 2003, Andy Polyakov wrote:
> > > ... In the nutshell the problem is that [as it is
> > > now] every failed SG_IO request is replayed second time without data
> > > transfer. ... Suggested patch
> > > overcomes this problem by immediately purging the failed SG_IO request
> > > from the request queue.
> >
> > Patch looks fine, care to resend actually trying to follow the style in
> > the file in question?
>
> Revised to my best ability for follow the coding style of file in
> question. A:-)
Much better, thanks :)
--
Jens Axboe
> > > > ... In the nutshell the problem is that [as it is
> > > > now] every failed SG_IO request is replayed second time without data
> > > > transfer. ... Suggested patch
> > > > overcomes this problem by immediately purging the failed SG_IO request
> > > > from the request queue.
> > >
> > > Patch looks fine, care to resend actually trying to follow the style in
> > > the file in question?
> >
> > Revised to my best ability for follow the coding style of file in
> > question. A:-)
>
> Much better, thanks :)
And it slipped through again:-( Sharpen up! BTW, the proposed patch
fixes even http://marc.theaimsgroup.com/?t=105634805100001&r=1&w=2,
where induced kernel crash occurs upon packet replay. Applicable to
up to 2.5.75. A.
8<--------8<--------8<--------8<--------8<--------8<--------8<--------
--- ./drivers/ide/ide-cd.c.orig Tue Jun 17 06:20:00 2003
+++ ./drivers/ide/ide-cd.c Thu Jun 19 15:56:39 2003
@@ -749,12 +749,17 @@
by transferring the semaphore from the packet
command request to the request sense request. */
+ rq->flags |= REQ_FAILED;
if ((stat & ERR_STAT) != 0) {
wait = rq->waiting;
rq->waiting = NULL;
+ if ((rq->flags & REQ_BLOCK_PC) != 0) {
+ cdrom_queue_request_sense(drive, wait,
+ rq->sense, rq);
+ return 1; /* REQ_BLOCK_PC self-cares */
+ }
}
- rq->flags |= REQ_FAILED;
cdrom_end_request(drive, 0);
if ((stat & ERR_STAT) != 0)
@@ -1657,13 +1662,14 @@
dma = info->dma;
if (dma) {
info->dma = 0;
- if ((dma_error = HWIF(drive)->ide_dma_end(drive))) {
- printk("ide-cd: dma error\n");
- HWIF(drive)->ide_dma_off(drive);
- }
+ dma_error = HWIF(drive)->ide_dma_end(drive);
}
if (cdrom_decode_status(drive, 0, &stat)) {
+ if ((stat & ERR_STAT) != 0) {
+ end_that_request_chunk(rq, 0, rq->data_len);
+ goto end_request; /* purge the whole thing... */
+ }
end_that_request_chunk(rq, 1, rq->data_len);
return ide_stopped;
}
@@ -1672,8 +1678,11 @@
* using dma, transfer is complete now
*/
if (dma) {
- if (dma_error)
+ if (dma_error) {
+ printk("ide-cd: dma error\n");
+ HWIF(drive)->ide_dma_off(drive);
return DRIVER(drive)->error(drive, "dma error", stat);
+ }
end_that_request_chunk(rq, 1, rq->data_len);
rq->data_len = 0;
On Fri, Jul 11 2003, Andy Polyakov wrote:
> > > > > ... In the nutshell the problem is that [as it is
> > > > > now] every failed SG_IO request is replayed second time without data
> > > > > transfer. ... Suggested patch
> > > > > overcomes this problem by immediately purging the failed SG_IO request
> > > > > from the request queue.
> > > >
> > > > Patch looks fine, care to resend actually trying to follow the style in
> > > > the file in question?
> > >
> > > Revised to my best ability for follow the coding style of file in
> > > question. A:-)
> >
> > Much better, thanks :)
>
> And it slipped through again:-( Sharpen up! BTW, the proposed patch
Calm down, I pass the fixes on and sometimes the just fall through the
cracks (and not always just my cracks).
> fixes even http://marc.theaimsgroup.com/?t=105634805100001&r=1&w=2,
> where induced kernel crash occurs upon packet replay. Applicable to
> up to 2.5.75. A.
It looks reasonable, I'm wondering whether the ->ide_dma_off() isn't too
harsh in the first place. But we can look at that later.
--
Jens Axboe