Re: [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed

[Hannes Reinecke <hare@suse.de>]

On 11/04/2014 07:36 PM, Christoph Hellwig wrote:
> On Tue, Nov 04, 2014 at 01:11:11PM +0100, Hannes Reinecke wrote:
>> BLK_EH_HANDLED does not work with scsi commands, as we need
>> to release the associated buffers correctly.
> 
> Which particular error do you see?  The ->eh_timed_out routines can
> return BLK_EH_HANDLED, and libata, iscsi and fc actually do, so we'll
> need to fix these issues either way.
> 
Hmm. Looking closer it seem that you are correct.
However, the main point still stands; there is no way
scsi_eh_scmd_add() can legitimately fail.

>> And scsi_eh_scmd_add() currently only will fail if no
>> error handler thread is started (which will never be the
>> case) 
> 
> Yes.
> 
>> or if the state machine encounters an illegal transition.
>> As state machine transitions don't have any real meaning
>> we can also force setting of the new state and
>> make scsi_dh_scmd_add() a void function.
>> With that we'll never have to resort to return
>> BLK_EH_HANDLED.
> 
> Which kind of transition did you see rejected?  Neither a CREATED or
> DEL_(RECOVERY) state should be possible when invoking the error handler,
> so I'd rather see asserts here than overriding the state machine.
> te RECOVERY state from a RUNNING or existing RECOVERY state, which
> leaves CREATED, CANCEL, DEL, CANCEL_RECOVERY or DEL_RECOVERY.
> 
I did not see any rejection so far.
However, we're calling scsi_host_set_state(), which _might_ fail.
And we need to have a way of either
a) make sure that it cannot fail prior to calling it
or
b) handling the failure.
So I'm fine with using asserts here.

I'll be updating the patch (and description)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (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