> > >> spin_unlock(q->queue_lock);
> > >> - map_request(ti, rq, md);
> > >> + if (map_request(ti, rq, md))
> > >> + goto requeued;
> > >> spin_lock_irq(q->queue_lock);
> > In the current device-mapper code, I would like to go with
> > spin_unlock/lock here.
> > However, there was a case to enable irq in map_requst() for request
> > allocation, and this spin_lock_irq was a work-around for the case.
> > Now, there is no such case in the device-mapper code, so spin_lock should
> > be enough here. But I'm still using spin_lock_irq for safeness, since
> > there might be some more cases to enable irq during request submission
> > to underlying devices.
Either map_requst() may enable irqs --- then you should enable them with
spin_unlock_irq (then, you'd have to review all the callers of
dm_request_fn that they are fine with enabling irqs).
Or map_request() must not enable irqs --- and then there is already a bug
and there is no point in using "spin_lock_irq" for safeness, because it
doesn't fix the bug (the interrupt may come anyway, before spin_lock_irq).
> I don't understand this.
> Are you saying that sometimes this fn is called with local interrupts disabled
> and other times with them still enabled?
> If they are sometimes left enabled, and the unlock() leaves them alone,
> then the lock_irq disables them, what piece of code then reenables them?
> Surely this code can only be working if local interrupts are always
> disabled prior to entry?
dm_request_fn can be called in an interrupt context, I scanned it for
calling process-context functions and found:
It may call rq_completed (either directly, via
->dm_done->dm_end_request->dm_put) or indirectly, when the request is
completed from host controller interrupt. And dm_put is a process_context
I believe it doesn't cause a real crash, because dm_put is called in
dm_blk_close, thus there is always at least one reference. When the device
is closed with dm_blk_close, there should be no requests on it.
But it is simply a logic error to call a process-context function from
an interrupt context. I'd remove those dm_get/dm_put from
request-based-dm --- they are not needed anyway, as long as there are
requests, the "mapped_device" structure can't disappear.
You can apply this (in 2.6.34-rc1) to catch all the errorneous users of
Catch errorneous calls of dm_put from an interrupt context.