RE: [PATCH v2] scsi: core: pair EH runtime PM get and put

[Fang Hongjie(方洪杰)]

> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 147127fb4db9..a216f56044d9 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -2342,6 +2342,7 @@ static void scsi_unjam_host(struct Scsi_Host
> *shost)
> >  int scsi_error_handler(void *data)
> >  {
> >  	struct Scsi_Host *shost = data;
> > +	bool eh_noresume;
> >
> >  	/*
> >  	 * We use TASK_INTERRUPTIBLE so that the thread is not
> > @@ -2383,7 +2384,8 @@ int scsi_error_handler(void *data)
> >  		 * what we need to do to get it up and online again (if we
> can).
> >  		 * If we fail, we end up taking the thing offline.
> >  		 */
> > -		if (!shost->eh_noresume &&
> scsi_autopm_get_host(shost) != 0) {
> > +		eh_noresume = shost->eh_noresume;
> 
> You should be aware that simply reading a variable does not snapshot its
> value unless the read is protected by a lock or something else.  In this
> case, the compiler is allowed to eliminate the eh_noresume variable and
> use shost->eh_noresume instead, both here and below, under the
> assumption that shost->eh_noresume doesn't change in the meantime.
> 
> To truly snapshot the the value in a way that's immune to changes from
> other threads, you have to use READ_ONCE():
> 
> 		eh_noresume = READ_ONCE(shost->eh_noresume);
> 
> The compiler is then not allowed to assume that shost->eh_noresume
> remains unchanged later on.  This is all explained (along with a _lot_
> of other material -- search for "READ_ONCE") in
> Documentation/memory-barriers.txt.
>

Thanks Alan.
I agree that a plain local variable is not enough for a compiler-level
snapshot.
There is one extra detail here: shost->eh_noresume is currently a
bitfield:
        unsigned eh_noresume:1;
so READ_ONCE(shost->eh_noresume) cannot be used directly because
READ_ONCE() takes the address of its argument.

My plan for v3 is to first make eh_noresume a regular bool, then use
READ_ONCE() in scsi_error_handler() to snapshot it once per EH iteration.
I will also use WRITE_ONCE() for the writer that changes eh_noresume while
SCSI EH may observe it.

Does this approach look reasonable to you?


Best.