Re: [PATCH, 3.7-rc7, RESEND] fs: revert commit bbdd6808 to fallocateUAPI

[Dave Chinner]

On Fri, Dec 07, 2012 at 02:16:28AM +0100, Ingo Molnar wrote:
> 
> * Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 
> > No, the problem is that the thing is not just a) wrong, but b) 
> > only made it in through sneaky ways.
> 
> People disagree with a), and b) only really matters if a) is 
> true.

Wow.

Let me paraphrase that logic:

If (maintainer thinks their patch is right) {
	patch doesn't need review
} else {
	/* maintainer thinks the patch is wrong. */
	/* XXX: why would you think your own patch is wrong? */
	patch needs review
}

Ok, if you were to disagree with the maintainer of a *different
subsystem* about whether the patch is right or not, how would you
know that the maintainer has made the change in the first place? It
hasn't been posted anywhere public like everyone who is not a
maintainer does....

Indeed, it is Ted's application of your logic that is the *exact
problem* that started this thread. Restating it as justification
for what has happened is a circular argument.

> You never gave a technical reason for why protecting against 
> future ABI clashes is 'wrong'. It looks like a marginally 
> useful, practical patch to me.

The concept isn't wrong - but the implementation is definitely
sub-optimal. We document syscall API changes, write tests for them,
and explain when and where filesystems need to implement
functionality, etc, and none of this has been done. I'd call that a
good technical argument for reverting the change, especially as
review would have picked this up and it would have been corrected
before proceeding with the change.

Simply put:

(concept == good) != (implementation == good)

And that's why we have review - to make sure the implementation is
as good as it can possibly be.

When you bypass review with the justification of "the concept is
good", then you are basically saying code review is irrelevant to the
implementation quality. Nothing could be further from the truth, and
that's why b) matters more than a). And in this case, we haven't
even got past the question of whether the concept is good or not
because it hasn't even been asked....

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx
--
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/