Messages in this thread | | | Subject | Re: [PATCH 2/2] scsi: don't use execute_in_process_context() | From | James Bottomley <> | Date | Wed, 15 Dec 2010 14:33:30 -0500 |
| |
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
| |