RE: [PATCH v3] target: core: remove from tmr_list at lun unlink

[Dmitriy Bogdanov <d.bogdanov@yadro.com>]

Hi Mike,

> > @@ -234,6 +225,7 @@ static void core_tmr_drain_tmr_list(
> >  		}
> >  
> >  		list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
> > +		tmr_p->tmr_dev = NULL;
>
> Is this patch now adding a way to hit:
>
> if (!tmr->tmr_dev)
>	WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd) < 0);                      
>
> in core_tmr_abort_task?
>
> You have the abort and lun reset works running on different CPUs.
> The lun reset hits the above code first and clears tmr_dev.
> The abort then hits the tmr->tmr_dev check and tries to do
> transport_lookup_tmr_lun.
>
> For the case where the lun is not removed, it looks like
> transport_lookup_tmr_lun will add the tmr to the dev_tmr_list
> but it would also be on the drain_tmr_list above so we would
> hit list corruption.

Yes, there is a such race. I think,  I can solve it by changing the order of
draining the tmr_list and state_list at LUN Reset to make the raced lines 
be under the same lock.

Especially SAM-5 describes(but does not require) aborting commands
before tmfs:
| When responding to a logical unit reset condition, the logical unit shall:
|	a) abort all commands as described in 5.6;
|	b) abort all copy operations (see SPC-4);
|	c) terminate all task management functions;


> For the case where the lun is getting removed, percpu_ref_tryget_live
> would fail in transport_lookup_tmr_lun and we hit the WARN_ON_ONCE.
> I think though with your patch, we would be ok and don't want
> the WARN_ON_ONCE, right? The lun reset would just wait for the
> abort. When it completes the abort and reset complete as expected.

I don’t understand the meaning of that transport_lookup_tmr_lun there.
Every TMF Abort has already executed transport_lookup_tmr_lun at the very
beginning of its handling. 
Eliminating the race will eliminate the impact of my patch on this case too. 

BR,
 Dmitry