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

[James Bottomley <jbottomley@parallels.com>]

On Mon, 2013-11-04 at 16:43 +0100, Hannes Reinecke wrote:
> On 11/04/2013 03:50 PM, James Bottomley wrote:
> > 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.
> > 
> So, this is what I've ended up with; sadly I had to use 'volatile'
> here which checkpatch doesn't like.

Why?  Volatile has no real meaning on a local variable.  You can just do
an ordinary eh_deadline = shost->eh_deadline; and it will see either the
before or after value.

> I _could_ move eh_deadline to be atomic, that would avoid the
> 'volatile' setting. Feels like an overkill, though.

Please dump the recheck loop and just check for zero again.  The race is
acceptable because we're not trying to mediate it in a meaningful way.
As long as the result is consistent with either the before or after
value, that's fine.

James