[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090103191706.GA2002@parisc-linux.org>
Date: Sat, 3 Jan 2009 12:17:06 -0700
From: Matthew Wilcox <matthew@....cx>
To: Andi Kleen <andi@...stfloor.org>
Cc: Chris Mason <chris.mason@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-btrfs <linux-btrfs@...r.kernel.org>
Subject: Re: Btrfs for mainline
On Fri, Jan 02, 2009 at 08:05:50PM +0100, Andi Kleen wrote:
> Some items I remember from my last look at the code that should
> be cleaned up before mainline merge (that wasn't a full in depth review):
>
> - locking.c needs a lot of cleanup.
> If combination spinlocks/mutexes are really a win they should be
> in the generic mutex framework. And I'm still dubious on the hardcoded
> numbers.
I don't think this needs to be cleaned up before merge. I've spent
an hour or two looking at it, and while we can do a somewhat better
job as part of the generic mutex framework, it's quite tricky (due to
the different <asm/mutex.h> implementations). It has the potential to
introduce some hard-to-hit bugs in the generic mutexes, and there's some
API discussions to have.
It's no worse than XFS (which still has its own implementation of
'synchronisation variables', a (very thin) wrapper around mutexes, a
(thin) wrapper around rwsems, and wrappers around kmalloc and kmem_cache.
> - compat.h needs to go
Later. It's still there for XFS.
> - there's various copy'n'pasted code from the VFS (like may_create)
> that needs to be cleaned up.
No urgency here.
> - there should be manpages for all the ioctls and other interfaces.
I wonder if Michael Kerrisk has time to help with that. Cc'd.
> - ioctl.c was not explicitely root protected. security issues?
This does need auditing.
> - some code was severly undercommented.
> e.g. each file should at least have a one liner
> describing what it does (ideally at least a paragraph). Bad examples
> are export.c or free-space-cache.c, but also others.
Nice to have, but generally not required.
> - ENOMEM checks are still missing all over (e.g. with most of the
> btrfs_alloc_path callers). If you keep it that way you would need
> at least XFS style "loop for ever" alloc wrappers, but better just
> fix all the callers. Also there used to be a lot of BUG_ON()s on
> memory allocation failure even.
> - In general BUG_ONs need review I think. Lots of externally triggerable
> ones.
Agreed on these two.
> - various checkpath.pl level problems I think (e.g. printk levels)
Can be fixed up later.
> - the printks should all include which file system they refer to
Ditto.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists