Re: [PATCH] Retry commands with UNIT_ATTENTION sense codes

[Hannes Reinecke <hare@suse.de>]

James Bottomley wrote:
> On Tue, 2010-05-04 at 15:27 -0500, Mike Christie wrote:
>> On 05/04/2010 03:15 PM, Mike Christie wrote:
>>> On 05/04/2010 11:26 AM, James Bottomley wrote:
>>>> The other patch is fine, but I don't think this is necessary. The
>>>> reason is that even returning SUCCESS here, we go straight into
>>>> scsi_finish_command() (which passes it up to the driver handler) and
>>>> then scsi_io_completion(). There's a catch for UNIT_ATTENTION in
>>>> scsi_io_completion
>>> The request is sent as a REQ_TYPE_BLOCK_PC (this flag is set for the
>>> request in sd_prepare_flush), and scsi_io_completion's blk_pc_request
>>> check() that returns the request upwards is before the UNIT_ATTENTION
>> I was looking at the wrong source.
>>
>> scsi_finish_command checks for REQ_TYPE_BLOCK_PC and sets good_bytes to 
>> scsi_bufflen, so when scsi_io_completion calls scsi_end_request, it 
>> fails the request before we can get to the UNIT_ATTENTION.
> 
> Ah, yes, missed that.
> 
> However there are other problems then.  We can't just eat all unit
> attentions on the BLOCK_PC path because some of the user programs want
> to see them (CD burners for one).  I know the patch allows some to
> proceed upwards, but it's risky making all except device not started do
> a retry.
> 
> I'm a bit stumped on this one ... the intention of BLOCK_PC is that the
> caller simply retries if they get a unit attention (which is what all
> SCSI code does).  The block code doesn't want to look at the sense data
> but at the error return.  I suppose we could make UNIT_ATTENTION
> translate to -EAGAIN and have block do the right thing?
> 
The intention of BLOCK_PC is ok, but the point here is the barrier
request isn't really a BLOCK_PC request. The caller precisely
does _not_ want to look the sense code but rather have the SCSI
layer do all the necessary things.

Why can't we just mark the barrier request as something else than
BLOCK_PC, eg REQ_TYPE_SPECIAL?
Then we'd avoid these pitfalls and everything should work as expected, right?

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index de6c603..b63f84e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1038,7 +1038,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 static void sd_prepare_flush(struct request_queue *q, struct request *rq)
 {
-       rq->cmd_type = REQ_TYPE_BLOCK_PC;
+       rq->cmd_type = REQ_TYPE_SPECIAL;
        rq->timeout = SD_TIMEOUT;
        rq->retries = SD_MAX_RETRIES;
        rq->cmd[0] = SYNCHRONIZE_CACHE;

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html