[lkml]   [2009]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: 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 level problems I think (e.g. printk levels)

Can be fixed up later.

> - the printks should all include which file system they refer to


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."

 \ /
  Last update: 2009-01-03 20:19    [W:0.252 / U:4.876 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site