Re: [PATCH] Separate target visibility from reaped state information

[Sebastian Herbszt <herbszt@gmx.de>]

James Bottomley wrote:
> On Wed, 2016-02-03 at 23:38 +0100, Sebastian Herbszt wrote:
> > James Bottomley wrote:
> > > On Mon, 2016-02-01 at 19:43 -0800, Bart Van Assche wrote:
> > > > On 01/19/16 17:03, James Bottomley wrote:
> > > > > On Tue, 2016-01-19 at 19:30 -0500, Martin K. Petersen wrote:
> > > > > > > > > > > "Bart" == Bart Van Assche <
> > > > > > > > > > > bart.vanassche@sandisk.com>
> > > > > > > > > > > writes:
> > > > > > 
> > > > > > Bart> Instead of representing the states "visible in sysfs"
> > > > > > and
> > > > > > "has
> > > > > > Bart> been removed from the target list" by a single state
> > > > > > variable,
> > > > > > use
> > > > > > Bart> two variables to represent this information.
> > > > > > 
> > > > > > James: Are you happy with the latest iteration of this?
> > > > > > Should I
> > > > > > queue
> > > > > > it?
> > > > > 
> > > > > Well, I'm OK with the patch: it's a simple transformation of
> > > > > the
> > > > > enumerated state to a two bit state.  What I can't see is how
> > > > > it
> > > > > fixes
> > > > > any soft lockup.
> > > > > 
> > > > > The only change from the current workflow is that the DEL
> > > > > transition
> > > > > (now the reaped flag) is done before the spin lock is dropped
> > > > > which
> > > > > would fix a tiny window for two threads both trying to remove
> > > > > the
> > > > > same
> > > > > target, but there's nothing that could possibly fix an
> > > > > iterative
> > > > > soft
> > > > > lockup caused by restarting the loop, which is what the
> > > > > changelog
> > > > > says.
> > > > 
> > > > Hello James,
> > > > 
> > > > scsi_remove_target() doesn't lock the scan_mutex which means that
> > > > concurrent SCSI scanning activity is not prohibited. Such
> > > > scanning 
> > > > activity can postpone the transition of the state of a SCSI
> > > > target 
> > > > into STARGET_DEL. I think if the scheduler decides to run the
> > > > thread 
> > > > that executes scsi_remove_target() on the same CPU as the
> > > > scanning 
> > > > code after the scanning code has obtained a reap ref and before
> > > > the 
> > > > scanning code has released the reap ref again that the soft
> > > > lockup 
> > > > can be triggered that has been reported by Sebastian Herbszt.
> > > 
> > > OK, I finally understand the scenario;  I'm not sure I understand
> > > how
> > > we're getting concurrent scanning and removal from a simple rmmod
> > > ... I
> > > take it this is insmod rmmod in a tight loop?
> > 
> > I am able to trigger the soft lockup with this test case run once:
> > 
> > modprobe lpfc
> > run fio for 10 seconds
> > rmmod lpfc
> > 
> > My test setup involves running qla2xxx in target mode (SCST) and
> > lpfc as initiator on the same system with one exported volume.
> 
> Hm, that doesn't seem to involve scanning colliding with removal. 
>  Probably something else is pinning the target for some reason ...
> unless there's some instability or constant change on the FC fabric
> itself?  FC is hotplug, so if devices continually appear and disappear,
> they'd effectively cause a scan of the added device to take place even
> during the rmmod.

No FC fabric involved; there is a direct connection between both HBAs.

> Anyway, does the last_scan patch fix the issue?

Will try to test it soon.

> James

Sebastian