Re: SCSI eats error from flush failure during hot plug

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2014-06-25 at 06:13 -0700, Christoph Hellwig wrote:
> On Thu, Jun 19, 2014 at 11:05:59AM -0700, James Bottomley wrote:
> > That's not really a good idea either ... I did think of it.  We'll end
> > up with a cmd_type of REQ_TYPE_FS which because of REQ_FLUSH (or REQ_FUA
> > or REQ_DISCARD or any number of other things) we have to treat as though
> > it were REQ_TYPE_BLOCK_PC.  It's much better to tune handling
> > expectations according to req->cmd_type because that's what we already
> > do.  These commands are actually set up by our handlers, so it's up to
> > us to mark the request type correctly.
> 
> Looking at the places where the SCSI midlayer cares about the request
> type:
> 
>  - scsi_finish_command to call ->done for non-PC requests.  Given that
>    we called into the driver to setup flush/discard/etc we should also
>    call into the driver on request completion
>  - scsi_eh_action: ditto for error handling
>  - scsi_noretry_cmd: I don't see why we'd want to treat flush request
> 	as having an implicit failfast flag
>  - scsi_io_completion: this mostly opts out of all kinds of error
> 	handling and retries, not really what we'd want either
>  - scsi_unprep_fn: calls ->uninit_command only for !PC request,
>    	so your patch introduces a leak for discard requests

Yes, but think what you're proposing.  Every block command with a
special setup goes via scsi_setup_blk_pc_cmnd() because they usually
have to translate to something special.  If we did what you propose,
every time we add one, we'd have to modify these five places in the code
plus the setup ... it's a bit insane plus a maintenance nightmare.

Since block gave us the command to set up as we please, we're entitled
to change the fields including cmd_type.

James