Re: [RFC] Deferred disk spinup during system resume

[Jeff Garzik <jgarzik@pobox.com>]

On 01/07/2011 02:06 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Jan 06, 2011 at 05:27:06PM -0800, maksim.rayskiy@gmail.com wrote:
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 7f77c67..0a9d400 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -4978,6 +4978,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>>   	struct ata_link *link = qc->dev->link;
>>   	u8 prot = qc->tf.protocol;
>>
>> +	if (unlikely(link->eh_info.dev_action[qc->dev->devno]&
>> +			ATA_EH_VERIFY)) {
>> +		ata_port_schedule_eh(ap);
>> +		qc->scsidone(qc->scsicmd);
>> +		ata_qc_free(qc);
>> +		return;
>
> Hmm... this is weird.  You'll probably want to define a qc flag to
> indicate that the command is for verify and then schedule EH from
> ata_qc_issue().  That said, I'm not quite sure whether that's any
> better than scheduling EH from libata-scsi.  It's not like the
> boundary is clearly defined.  We already play with EH for other stuff
> from libata-scsi.  Jeff, do you think going through QCFLAG is
> important?

The boundary is clearly defined.  It's just not a file boundary :) 
libata-scsi contains EH callouts, but all of those are (a) sysfs 
attribute handling, (b) for callers located outside libata-scsi, or (c) 
the SCSI host scan.

The command translation and execution hot path is notably not in that list.

It is a clear laying violation to call out from the middle of a command 
translation, to directly execute some EH actions.  Remember, this is the 
stage at which we are QUEUEING commands.  There may be other commands in 
the queue, or currently executing.  If someone sends a READ VERIFY, we 
do not want to IMMEDIATELY schedule EH, we want to execute an EH action 
when the READ VERIFY command is ready to be executed.

It is quite incorrect to assume that READ VERIFY will only be sent to 
libata for execution during system resume.  The removal of READ VERIFY 
translation from ata_scsi_start_stop_xlat() is clearly wrong.

	Jeff