Re: [PATCH 2/3] scsi: improved eh timeout handler

[James Bottomley <jbottomley@parallels.com>]

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).

James