Re: [GIT PULL] SCSI queuecommand API change for 2.6.37-rc1

From: Jeff Garzik
Date: Fri Nov 12 2010 - 21:03:21 EST


On 11/12/2010 08:42 PM, Linus Torvalds wrote:
On Fri, Nov 12, 2010 at 3:55 PM, James Bottomley
<James.Bottomley@xxxxxxx> wrote:

This patch set contains a single patch modifying the SCSI queuecommand
host template API to go from being called with the host lock held to
being called locklessly. The transformation is a directly equivalent
one (i.e. the locking is simply pushed into each HBA) but will form the
basis for optimising locking in the driver patch for the next merge
window.

Ok, so we talked about this patch at the KS, but I never saw it.

And now seeing it, I do detest it.

Why? Because if some driver gets missed for any reason (notably if
it's currently out of tree, and gets merged later), afaik there will
be ABSOLUTELY ZERO compiler warnings or anything about it, because you
kept the "queuecommand" function exactly the same. Whether it's a
locked or non-locked one, it always is of type
[...]
So please: when you change the semantics of a function, just change
the function prototype (or function name) at the same time. Especially
when it comes to a driver interface, so that the drivers don't get
taken by surprise.

Type safety and automatic compiler warnings really are our friends.
Especially when the patch was presumably mostly auto-generated, and
maybe the script missed something, and missing some conversion has
such subtle effects.

That is precisely what I mentioned in the original patch:
http://marc.info/?l=linux-ide&m=128891665713984&w=2

And brought this up again when James merged my patch:
http://marc.info/?l=linux-scsi&m=128943169802009&w=2

I even volunteer[ed] to do the work to make it happen...

Jeff


P.S. My patch was not auto-generated. One of the two previous attempts at this change was scripted w/ coccinelle, and it created obvious locking problems.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/