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

[Boaz Harrosh <bharrosh@panasas.com>]

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