Re: [Cbe-oss-dev] [regression/bisected] corrupt CD data after media change and delay

Boaz Harrosh
2008-06-10T15:49:48Z

James Bottomley wrote:
> [resend, managed to drop linux-scsi first time around]
>
> On Mon, 2008-06-09 at 17:27 +0200, Geert Uytterhoeven wrote:
>> Hi James,
>>
>> On Mon, 9 Jun 2008, James Bottomley wrote:
>>> On Mon, 2008-06-09 at 15:54 +0200, Geert Uytterhoeven wrote:
>>>> I managed to reproduce it on my laptop (Core 2 Duo, SATA DVD-RAM, running
>>>> Ubuntu 8.04 for amd64), by booting Debian's 2.6.25 kernel into recovery mode.
>>>> So the problem is not PS3-specific.
>>>>
>>>> Worse, I never got an updated /sys/block/sr0/size for the second CD, not even
>>>> when mounting it ASAP (which is ca. 15-20 seconds after inserting it). It
>>>> always stayed at the value for the first CD.
>>> Well, we have the taxonomy. It's something to do with the media change
>>> trigger. Could you try getting the output of this patch and correlate
>>> the prints with your success and failure cases?
>> Sure!
>>
>> Inserting first CD, mounting:
>>
>> | +0+ the_result = 0x8000002 Sense Key : 0x0 [current] [descriptor]
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +11+ Return forcing update is 1
>> ^
>> OK
>>
>> | ISO 9660 Extensions: RRIP_1991A
>>
>> Unmounting first CD:
>>
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +11+ Return forcing update is 0
>>
>> Ejecting first CD:
>>
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +11+ Return forcing update is 0
>>
>> Inserting second CD, mounting after 30 seconds:
>>
>> | +0+ the_result = 0x8000002 Sense Key : 0x0 [current] [descriptor]
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +1+ CDS_DISC_OK
>> | +0+ the_result = 0x0 Sense Key : 0x0 [current] [descriptor]
>> | +11+ Return forcing update is 0
>> ^
>> Not updated!
>> | ISO 9660 Extensions: Microsoft Joliet Level 3
>> | ISO 9660 Extensions: RRIP_1991A
>>
>> => failed!
>
> Great! confirms the theory of the what but doesn't tell us how this is
> happening.
>
> My best guess is that it's something to do with eaten UNIT_ATTENTION
> keys. I theorise that in the successful case, they get processed in
> scsi_io_completion which sets the changed bit. In the delay case, the
> unit attention is eaten by sr_test_unit_ready so the changed bit is
> always kept at zero. If this is the case, then the patch below should
> fix it. If not, we're back to more debugging ...
>
> James
>
> ---
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7ee86d4..c82df8b 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -178,6 +178,9 @@ int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
> the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
> 0, sshdr, SR_TIMEOUT,
> retries--);
> + if (scsi_sense_valid(sshdr) &&
> + sshdr->sense_key == UNIT_ATTENTION)
> + sdev->changed = 1;
>
> } while (retries > 0 &&
> (!scsi_status_is_good(the_result) ||
>
>

I think, I recall that, any issued command, not only unit_ready, might be appended
with UNIT_ATTENTION sense. So maybe as a deeper fix (with lots of testing) we want
to add this logic into the scsi_check_sense() processing. But I'm not sure either way.

Boaz