I agree 110%. I've been bitching about it since around 2.1.97 when it
went in. Unfortunately, there were attempts made to make the locking
intelligent, and short of a re-write of significant portions of code,
they resulted in deadlocks.
> I was trying to understand why we need it hold for a so long time.
>
> Starting from unplug_device and add_request everything in the lowlevel
> block device path holds the lock. So a request path could be something
> like:
>
> add_request->do_sd_request->(thehell
> of)requeue_sd_request->scsi_do_cmd->internal_cmnd->and finally
> queuecommand (that in the worst case could loop on the bus)
>
> I had not the time to look at the path of the irq handler that tell us
> about I/O completation yet (any hint is welcome ;)).
The mid-level scsi_done() expects to be able to freely play with any
command that is returned to it. Because of the way request structures
are allocated, because of the way scsi_malloc() and scsi_free() operate,
and because of how scsi_times_out() operates, these functions all race
against each other in thinking that they each have exclusive control of
the scsi subsystem in general and doing all sorts of nasty things when
that isn't forced true by the spinlocks. The most common result is that
a request_list gets destroyed or you get corrupted data.
> My question is: exactly which is _the_ race (or the race_s_) are we
> avoiding with this spinlock?
The races include do_scsi_cmnd() vs. scsi_done() vs. scsi_times_out().
The secondary race is to make sure that we never call hostt->queue() or
hostt->queuecommand() or hostt->reset() or hostt->abort() or any other
hostt-> function pointer while already in another host driver function
(aka, we have to guarantee single entrancy to the low-level drivers,
this includes being single threaded with respect to the low level
drivers interrupt routine which the mid-level code has no direct control
over).
> If the point is to have only an add_request() path running at once we
> could use a simpler down() in add_request (supposing that the request
> can't be done from an irq handler, or play with down_trylock() if
> in_interrupt() == 1), and an up() at I/O completation time. And we could
> still held the spinlock _only_ to protect the
> request-data-structure handling.
>
> And btw since many places just does spin_unlock_irq in the scsi path,
> it's just possible that you'll have two request path running at the same
> time.
I wouldn't say "many" places do a spin_unlock_irq() in the scsi code. I
seem to recall there are only 2 or 3 in the common code path and they
are in very short lived sections of code that will re-aquire the lock
after they are done waiting which means you still effectively single
thread that entire subsystem. All you've really done there is creating
a rest area on the highway for code execution to take a break at.
--
Doug Ledford <dledford@redhat.com>
Opinions expressed are my own, but
they should be everybody's.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/