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

[James Bottomley <jbottomley@parallels.com>]

On Mon, 2013-11-04 at 15:46 +0100, Hannes Reinecke wrote:
> 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?

Yes (except that there should be a comment explaining why we do the read
twice), I think the cost of the extra read check is much less than the
spinlock on all of our platforms.

James