Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

On Thu, Jan 10, 2008 at 12:03:24PM -0700, Matthew Wilcox wrote:
> On Thu, Jan 10, 2008 at 07:59:44PM +0100, Andi Kleen wrote:
> > > Really, all this is doing is open coding what the ioctl handler is doing
> > > anyway, isn't it? in which case, why bother to change it at all?
> > 
> > Because once it's open coded it is visible and can then be eliminated.
> > Does SCSI need the BKL at all?
> > 
> > But perhaps for such a long ioctl handler it would be better to move
> > the lock/unlock_kernel()s into the individual case ...: statements;
> > then it could be eliminated step by step.
> 
> This style of conversion is going to cause a lot of churn --

If he pushes the lock_kernels() into the cases then not.

But even a few refactorings are not that bad if they result in
a BKL less SCSI.

> re-architecting this function to be single-exit, then presumably when
> the lock_kernel calls are pushed further down or eliminated, turning it
> back into a multiple-exit function.
> 
> I suggest that for complex ioctl handlers, it be left to the maintainers

Which have had years to convert their drivers, but they mostly didn't
do it. They need more incentive.

> to handle, and handle it properly all at once, rather than a gradual
> pushdown.
> 
> You could argue that unlocked_ioctl has been around for a long time and
> people haven't made that move yet.  But there's been no pressure before

Yes I argue exactly that.

> now to do so, and I think people would rather convert their own code
> than have somebody else do it.

Has been proven to not work.

I don't really blame the maintainers too much. The implicit BKL in ioctl
is actually pretty obscure. But making it explicit will force their
hand.

So in short I'm sorry, but you're wrong on that.

-Andi
--
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/