On 11/04/2013 03:25 PM, James Bottomley wrote:
> On Mon, 2013-11-04 at 14:36 +0100, Hannes Reinecke wrote:
>> On 10/31/2013 04:49 PM, Christoph Hellwig wrote:
>>> Looks reasonable to me, but a few minor nitpicks:
>>>
>>>> +	spin_lock_irqsave(sdev->host->host_lock, flags);
>>>> +	if (scsi_host_eh_past_deadline(sdev->host)) {
>>>
>>> I don't have the implementation of scsi_host_eh_past_deadline in my
>>> local tree, but do we really need the host lock for it?
>>>
>> Yes. The eh_deadline variable might be set from an interrupt context
>> or from userland, so we need to protect access to it.
> 
> That's not really true.  on all our supported architectures 32 bit
> reads/writes are atomic, which means that if one CPU writes a word at
> the same time another reads one, the reader is guaranteed to see either
> the old or the new data.  Given the expense of lock cache line bouncing
> on the newer architectures, we really want to avoid a spinlock where
> possible.
> 
> In this case, the problem with the implementation is that the writer
> might set eh_deadline to zero, but this is fixable in
> scsi_host_eh_past_deadline() by checking for zero before and after the
> time_before (for the zero to non-zero and non-zero to zero cases).
> 
IE you mean something like that attached patch?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)