Re: [PATCH 2/2] scsi: don't use execute_in_process_context()

[James Bottomley <James.Bottomley@suse.de>]

On Wed, 2010-12-15 at 20:19 +0100, Tejun Heo wrote:
> Hello,
> 
> On 12/15/2010 08:10 PM, James Bottomley wrote:
> >> Yes, it would do, but we were already too far with the existing
> >> implementation and I don't agree we need more when replacing it with
> >> usual workqueue usage would remove the issue.  So, when we actually
> >> need them, let's consider that or any other way to do it, please.
> >> A core API with only a few users which can be easily replaced isn't
> >> really worth keeping around.  Wouldn't you agree?
> > 
> > Not really ... since the fix is small and obvious.
> 
> IMHO, it's a bit too subtle to be a good API.  The callee is called
> under different (locking) context depending on the callsite and I've
> been already bitten enough times from implicit THIS_MODULEs.  Both
> properties increase possbility of introducing problems which can be
> quite difficult to detect and reproduce.

Both have subtleties ... see below.

> > Plus now it can't be moved into SCSI because I need the unremovable
> > call chain.
> 
> Yes, with the proposed change, it cannot be moved to SCSI.
> 
> > Show me how you propose to fix it differently first, since we both agree
> > the initial attempt doesn't work, and we can take the discussion from
> > there.
> 
> Given that the structures containing the work items are dynamically
> allocated, I would introduce a scsi_wq, unconditionally schedule
> release works on them and flush them before unloading.  Please note
> that workqueues no longer require dedicated threads, so it's quite
> cheap.

A single flush won't quite work.  The target is a parent of the device,
both of which release methods have execute_in_process_context()
requirements.  What can happen here is that the last put of the device
will release the target (from the function).  If both are moved to
workqueues, a single flush could cause the execution of the device work,
which then queues up target work (and makes it still pending).  A double
flush will solve this (because I think our nesting level doesn't go
beyond 2) but it's a bit ugly ...

execute_in_process_context() doesn't have this problem because the first
call automatically executes the second inline (because it now has
context).

James