Re: Btrfs for mainline

Re: Btrfs for mainline

[Roland]

>I've done some testing against Linus' git tree from last night and the
>current btrfs trees still work well.

i`d like to confirm that it`s already quite stable, even if used with 
compression.

happy new year to everyone !

roland


>There are a few bug fixes that I need to include from while I was on
>vacation but I haven't made any large changes since early in December:

>Btrfs details and usage information can be found:

>http://btrfs.wiki.kernel.org/

>The btrfs kernel code is here:

>http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=summary

>And the utilities are here:

>http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-progs-unstable.git;a=summary

>-chris

Re: Btrfs for mainline

[Chris Mason]

On Mon, 2009-01-12 at 15:14 +0000, Miguel Figueiredo Mascarenhas Sousa
Filipe wrote:
> Hi,
> 
> On Mon, Jan 12, 2009 at 1:58 PM, Chris Mason <chris.mason@oracle.com> wrote:
> > On Sun, 2009-01-11 at 15:34 -0800, Andrew Morton wrote:
> >> http://bugzilla.kernel.org/show_bug.cgi?id=12435
> >>
> 
> This is by far the biggest issue btrfs has for simple/domestic users.
> Its probably the most tested miss-feature of btrfs, something almost
> all new testers encounter & report.
> 
> Is fixing this having maximum priority?
> The sooner this is fixed, less energy is spend by everyone
> (entusiasts, testers, bug reporters, bug triagers, mailing list
> campers..etc) and we might save some whales, dolphins and pinguins by
> reducing our carbon footprint. :)
> 

Yes, this definitely at the top of my list.  Along with better
documentation of the project as a whole.

-chris

Re: Btrfs for mainline

[Miguel Figueiredo Mascarenhas Sousa Filipe]

Hi,

On Mon, Jan 12, 2009 at 1:58 PM, Chris Mason <chris.mason@oracle.com> wrote:
> On Sun, 2009-01-11 at 15:34 -0800, Andrew Morton wrote:
>> http://bugzilla.kernel.org/show_bug.cgi?id=12435
>>

This is by far the biggest issue btrfs has for simple/domestic users.
Its probably the most tested miss-feature of btrfs, something almost
all new testers encounter & report.

Is fixing this having maximum priority?
The sooner this is fixed, less energy is spend by everyone
(entusiasts, testers, bug reporters, bug triagers, mailing list
campers..etc) and we might save some whales, dolphins and pinguins by
reducing our carbon footprint. :)

Kind regards.

>> Congratulations ;)
>
> Rejected documented, that's the best buzilla tag ever ;)
>
> Thanks,
> Chris
>



-- 
Miguel Sousa Filipe

Re: Btrfs for mainline

[Chris Mason]

On Sun, 2009-01-11 at 15:34 -0800, Andrew Morton wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=12435
> 
> Congratulations ;)

Rejected documented, that's the best buzilla tag ever ;)

Thanks,
Chris

Re: Btrfs for mainline

[Andrew Morton]

http://bugzilla.kernel.org/show_bug.cgi?id=12435

Congratulations ;)

Re: Btrfs for mainline

[jim owens]

Chris Mason wrote:
> 
> Unresolved from this reviewing thread:
> 
> * Should it be named btrfsdev?  My vote is no, it is extra work for the
> distros when we finally do rename it, and I don't think btrfs really has
> the reputation for stability right now.  But if Linus or Andrew would
> prefer the dev on there, I'll do it.

We know who has the last word on this.

This is just additional background for those who commented.

Using tricks such as btrfsdev, mount "unsafe", or kernel
messages won't provide a guarantee that btrfs is only used
in appropriate and risk-free ways.  Those tricks also won't
prevent all problems caused by booting a broken old (or new)
release.  And the perceived quality and performance of any
filesystem release, even between stable versions, depends
very much on individual system configuration and use.

Before Chris posted the code, we had some btrfs concall
discussions about the best way to set user expectations on
btrfs mainline 1.0.  Consensus was that the best way we
could do this was to warn them when they did mkfs.btrfs.

Today I sent a patch to Chris (which he may ignore/change)
so mkfs.btrfs will say:

WARNING! - Btrfs v0.16-39-gf9972b4 IS EXPERIMENTAL
WARNING! - see http://btrfs.wiki.kernel.org before using

with a blank line before and after.  They don't have to
confirm since they can just mkfs a different filesystem
if I scared them away.  The version is auto-generated.

jim

Re: Btrfs for mainline

[Chris Mason]

On Wed, 2009-01-07 at 09:33 +0000, David Woodhouse wrote:
> On Tue, 2009-01-06 at 14:41 -0500, Chris Mason wrote:

> One more thing I'd suggest is removing the INSTALL file. The parts about
> having to build libcrc32c aren't relevant when it's part of the kernel
> tree and you have 'select LIBCRC32C', and the documentation on the
> userspace utilities probably lives _with_ the userspace repo. Might be
> worth adding a pointer to the userspace utilities though, in
> Documentation/filesystems/btrfs.txt
> 
> I think you can drop your own copy of the GPL too.
> 

I've pushed out your patch for this, thanks.

-chris

Re: Btrfs for mainline

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Jan 07, 2009 at 02:07:42PM +0100, Ingo Molnar wrote:
> > * Chris Mason <chris.mason@oracle.com> wrote:
> > > All of this is a long way of saying the btrfs locking scheme is far from 
> > > perfect.  I'll look harder at the loop and ways to get rid of it.
> > 
> > <ob'plug>
> > 
> > adaptive spinning mutexes perhaps? Such as:
> 
> Um, I don't know how your mail client does threading, but mine shows 
> Peter's message introducing the adaptive spinning mutexes as a reply to 
> one of Chris' messages in the btrfs thread.
>
> Chris is just saying he'll look at other ways to not need the spinning 
> mutexes.

But those are not the same spinning mutexes. Chris wrote his mail on Jan 
05, Peter his first mail about spin-mutexes on Jan 06, as a reaction to 
Chris's mail.

My reply links back the discussion to the original analysis from Chris 
pointing out that it would be nice to try BTRFS with plain mutexes plus 
Peter's patch - instead of throwing away BTRFS's locking design or 
anything intrusive like that.

Where's the problem? :)

	Ingo

Re: Btrfs for mainline

[Matthew Wilcox]

On Wed, Jan 07, 2009 at 02:07:42PM +0100, Ingo Molnar wrote:
> * Chris Mason <chris.mason@oracle.com> wrote:
> > All of this is a long way of saying the btrfs locking scheme is far from 
> > perfect.  I'll look harder at the loop and ways to get rid of it.
> 
> <ob'plug>
> 
> adaptive spinning mutexes perhaps? Such as:

Um, I don't know how your mail client does threading, but mine shows
Peter's message introducing the adaptive spinning mutexes as a reply to
one of Chris' messages in the btrfs thread.

Chris is just saying he'll look at other ways to not need the spinning
mutexes.

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

Re: Btrfs for mainline

[Ingo Molnar]

* Chris Mason <chris.mason@oracle.com> wrote:

> On Tue, 2009-01-06 at 01:47 +1100, Nick Piggin wrote:
> 
> [ adaptive locking in btrfs ]
> 
> > adaptive locks have traditionally (read: Linus says) indicated the locking
> > is suboptimal from a performance perspective and should be reworked. This
> > is definitely the case for the -rt patchset, because they deliberately
> > trade performance by change even very short held spinlocks to sleeping locks. 
> > 
> > So I don't really know if -rt justifies adaptive locks in mainline/btrfs.
> > Is there no way for the short critical sections to be decoupled from the
> > long/sleeping ones?
> 
> Yes and no.  The locks are used here to control access to the btree
> leaves and nodes.  Some of these are very hot and tend to stay in cache
> all the time, while others have to be read from the disk.
> 
> As the btree search walks down the tree, access to the hot nodes is best 
> controlled by a spinlock.  Some operations (like a balance) will need to 
> read other blocks from the disk and keep the node/leaf locked.  So it 
> also needs to be able to sleep.
> 
> I try to drop the locks where it makes sense before sleeping operatinos, 
> but in some corner cases it isn't practical.
> 
> For leaves, once the code has found the item in the btree it was looking 
> for, it wants to go off and do something useful (insert an inode etc 
> etc). Those operations also tend to block, and the lock needs to be held 
> to keep the tree block from changing.
> 
> All of this is a long way of saying the btrfs locking scheme is far from 
> perfect.  I'll look harder at the loop and ways to get rid of it.

<ob'plug>

adaptive spinning mutexes perhaps? Such as:

   http://lkml.org/lkml/2009/1/7/119

(also pullable via the URI below)

If you have a BTRFS performance test where you know such details matter 
you might want to try Peter's patch and send us the test results.

	Ingo

------------->

You can pull the latest core/locking git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core/locking

------------------>
Peter Zijlstra (1):
      mutex: implement adaptive spinning


 include/linux/mutex.h   |    4 +-
 include/linux/sched.h   |    2 +
 kernel/mutex-debug.c    |   10 +------
 kernel/mutex-debug.h    |    8 -----
 kernel/mutex.c          |   46 ++++++++++++++++++++++--------
 kernel/mutex.h          |    2 -
 kernel/sched.c          |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched_debug.c    |    2 +
 kernel/sched_features.h |    1 +
 9 files changed, 115 insertions(+), 33 deletions(-)

Re: Btrfs for mainline

[David Woodhouse]

On Tue, 2009-01-06 at 14:41 -0500, Chris Mason wrote:
> Hello everyone,
> 
> Thanks for all of the comments so far.  I've pushed out a number of
> fixes for btrfs mainline, covering most of the comments from this
> thread.
> 
> * All LINUX_KERNEL_VERSION checks are gone.
> * checkpatch.pl fixes
> * Extra permission checks on the ioctls
> * Some important bug fixes from the btrfs list
> * Andi found a buggy use of kmap_atomic during checksum verification
> * Drop EXPORT_SYMBOLs from extent_io.c

Looking good...

> Unresolved from this reviewing thread:
> 
> * Should it be named btrfsdev?  My vote is no, it is extra work for the
> distros when we finally do rename it, and I don't think btrfs really has
> the reputation for stability right now.  But if Linus or Andrew would
> prefer the dev on there, I'll do it.

I agree; I don't think there's any particular need for the 'dev' suffix.
It's already dependent on CONFIG_EXPERIMENTAL, after all.

> * My ugly mutex_trylock spin.  It's a hefty performance gain so I'm
> hoping to keep it until there is a generic adaptive lock.

If a better option is forthcoming, by all means use it -- but I wouldn't
see the existing version as a barrier to merging.


One more thing I'd suggest is removing the INSTALL file. The parts about
having to build libcrc32c aren't relevant when it's part of the kernel
tree and you have 'select LIBCRC32C', and the documentation on the
userspace utilities probably lives _with_ the userspace repo. Might be
worth adding a pointer to the userspace utilities though, in
Documentation/filesystems/btrfs.txt

I think you can drop your own copy of the GPL too.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: Btrfs for mainline

[Jamie Lokier]

J. Bruce Fields wrote:
> Old kernel versions may still get booted after brtfs has gotten a
> reputation for stability.  E.g. if I move my / to brtfs in 2.6.34, then
> one day need to boot back to 2.6.30 to track down some regression, the
> reminder that I'm moving back to some sort of brtfs dark-ages might be
> welcome.

Require a mount option "allow_unstable_version" until it's stable?
The stable version can ignore the option.

In your example, you wouldn't use the option, and in the btrfs dark
ages it would refuse to mount.

-- Jamie

Re: Btrfs for mainline

[Chris Mason]

Hello everyone,

Thanks for all of the comments so far.  I've pushed out a number of
fixes for btrfs mainline, covering most of the comments from this
thread.

* All LINUX_KERNEL_VERSION checks are gone.
* checkpatch.pl fixes
* Extra permission checks on the ioctls
* Some important bug fixes from the btrfs list
* Andi found a buggy use of kmap_atomic during checksum verification
* Drop EXPORT_SYMBOLs from extent_io.c

Unresolved from this reviewing thread:

* Should it be named btrfsdev?  My vote is no, it is extra work for the
distros when we finally do rename it, and I don't think btrfs really has
the reputation for stability right now.  But if Linus or Andrew would
prefer the dev on there, I'll do it.

* My ugly mutex_trylock spin.  It's a hefty performance gain so I'm
hoping to keep it until there is a generic adaptive lock.

The full kernel tree is here:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=summary


The standalone tree just has the btrfs files and commits, reviewers may
find it easier:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable-standalone.git;a=summary

The utilities can be found here:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-progs-unstable.git;a=summary

-chris

Re: Btrfs for mainline

[Ryusuke Konishi]

On Mon, 05 Jan 2009 09:14:56 -0500, Chris Mason <chris.mason@oracle.com> wrote:
> On Sat, 2009-01-03 at 18:44 +0900, Ryusuke Konishi wrote:
> > On Fri, 02 Jan 2009 14:38:07 -0500, Chris Mason 
> >
> > Btrfs seems to have other helpful code including pages/bio compression
> > which may become separable, too.  And, this may be the same for
> > pages/bio encryption/decryption code which would come next.  (I don't
> > mention about the volume management/raid feature here to avoid getting
> > off the subject, but it's likewise).
> > 
> 
> The compression code is somewhat tied to the btrfs internals, but it
> could be pulled out without too much trouble.  The big question there is
> if other filesystems are interested in transparent compression support.

It was so attractive to me ;)
though I don't know if it's applicable.

Anyway, making this common can be said to be the exercise of someone
who try to reuse it, and I don't adhere to it in order not to get off
the subject.  Just I wanted to add presence of other candicates.

> But, at the end of the day, most of the work is still done by the zlib
> code.  The btrfs bits just organize pages to send down to zlib.
> 
> -chris

Yes, but that's the interesting point; it provides ways to apply
compression through array of pages, or bio - I like it.

Ryusuke

Re: Btrfs for mainline

[J. Bruce Fields]

On Mon, Jan 05, 2009 at 08:18:58AM -0500, Chris Mason wrote:
> On Mon, 2009-01-05 at 21:07 +1100, Chris Samuel wrote:
> > On Sat, 3 Jan 2009 8:01:04 am Andi Kleen wrote:
> > 
> > > When it's in mainline I suspect people will start using it for that.
> > 
> > Some people don't even wait for that. ;-)
> > 
> > Seriously though, if that is a concern can I suggest taking the btrfsdev route 
> > and, if you want a real belt and braces approach, perhaps require it to have a 
> > mandatory mount option specified to successfully mount, maybe "eat_my_data" ?
> 
> I think ext4dev made more sense for ext4 because people generally expect
> ext* to be stable.  Btrfs doesn't quite have the reputation for
> stability yet, so I don't feel we need a special -dev name for it.

Old kernel versions may still get booted after brtfs has gotten a
reputation for stability.  E.g. if I move my / to brtfs in 2.6.34, then
one day need to boot back to 2.6.30 to track down some regression, the
reminder that I'm moving back to some sort of brtfs dark-ages might be
welcome.

(Not that I have particularly strong feelings about this.)

--b.

> 
> But, if Andrew/Linus prefer that unstable filesystems are tagged with
> -dev, I'm happy to do it.
> 
> -chris
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Btrfs for mainline

[Chris Mason]

On Tue, 2009-01-06 at 01:47 +1100, Nick Piggin wrote:

[ adaptive locking in btrfs ]

> adaptive locks have traditionally (read: Linus says) indicated the locking
> is suboptimal from a performance perspective and should be reworked. This
> is definitely the case for the -rt patchset, because they deliberately
> trade performance by change even very short held spinlocks to sleeping locks. 
> 
> So I don't really know if -rt justifies adaptive locks in mainline/btrfs.
> Is there no way for the short critical sections to be decoupled from the
> long/sleeping ones?

Yes and no.  The locks are used here to control access to the btree
leaves and nodes.  Some of these are very hot and tend to stay in cache
all the time, while others have to be read from the disk.

As the btree search walks down the tree, access to the hot nodes is best
controlled by a spinlock.  Some operations (like a balance) will need to
read other blocks from the disk and keep the node/leaf locked.  So it
also needs to be able to sleep.

I try to drop the locks where it makes sense before sleeping operatinos,
but in some corner cases it isn't practical.

For leaves, once the code has found the item in the btree it was looking
for, it wants to go off and do something useful (insert an inode etc
etc). Those operations also tend to block, and the lock needs to be held
to keep the tree block from changing.

All of this is a long way of saying the btrfs locking scheme is far from
perfect.  I'll look harder at the loop and ways to get rid of it.

-chris

Re: Btrfs for mainline

[Matthew Wilcox]

On Tue, Jan 06, 2009 at 01:47:23AM +1100, Nick Piggin wrote:
> adaptive locks have traditionally (read: Linus says) indicated the locking
> is suboptimal from a performance perspective and should be reworked. This
> is definitely the case for the -rt patchset, because they deliberately
> trade performance by change even very short held spinlocks to sleeping locks. 
> 
> So I don't really know if -rt justifies adaptive locks in mainline/btrfs.
> Is there no way for the short critical sections to be decoupled from the
> long/sleeping ones?

I wondered about that option too.  Let's see if we have other users that
will benefit from adaptive locks -- my gut says that Linus is right, but
then there's a lot of lazy programmers out there using mutexes when they
should be using spinlocks.

I wonder about a new lockdep-style debugging option that adds a bit per
mutex class to determine whether the holder ever slept while holding it.
Then a periodic check could determine which mutexes were needlessly held
would find one style of bad lock management.

The comment in btrfs certainly indicates that locking redesign is a
potential solution:

 * locks the per buffer mutex in an extent buffer.  This uses adaptive locks
 * and the spin is not tuned very extensively.  The spinning does make a big
 * difference in almost every workload, but spinning for the right amount of
 * time needs some help.
 *
 * In general, we want to spin as long as the lock holder is doing btree searches,
 * and we should give up if they are in more expensive code.

btrfs almost wants its own hybrid locks (like lock_sock(), to choose
a new in-tree example).  One where it will spin, unless a flag is set
to not spin, in which case it sleeps.  Then the 'more expensive code'
can set the flag to not bother spinning.

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

Re: Btrfs for mainline

[Nick Piggin]

On Monday 05 January 2009 05:41:03 Matthew Wilcox wrote:
> On Sun, Jan 04, 2009 at 07:21:50PM +0100, Peter Zijlstra wrote:
> > The -rt tree has adaptive spin patches for the rtmutex code, its really
> > not all that hard to do -- the rtmutex code is way more tricky than the
> > regular mutexes due to all the PI fluff.
> >
> > For kernel only locking the simple rule: spin iff the lock holder is
> > running proved to be simple enough. Any added heuristics like max spin
> > count etc. only made things worse. The whole idea though did make sense
> > and certainly improved performance.
>
> That implies moving
>
>         struct thread_info      *owner;
>
> out from under the CONFIG_DEBUG_MUTEXES code.  One of the original
> justifications for mutexes was:
>
>  - 'struct mutex' is smaller on most architectures: .e.g on x86,
>    'struct semaphore' is 20 bytes, 'struct mutex' is 16 bytes.
>     A smaller structure size means less RAM footprint, and better
>     CPU-cache utilization.
>
> I'd be reluctant to reverse that decision just for btrfs.
>
> Benchmarking required!  Maybe I can put a patch together that implements
> the simple 'spin if it's running' heuristic and throw it at our
> testing guys on Monday ...

adaptive locks have traditionally (read: Linus says) indicated the locking
is suboptimal from a performance perspective and should be reworked. This
is definitely the case for the -rt patchset, because they deliberately
trade performance by change even very short held spinlocks to sleeping locks. 

So I don't really know if -rt justifies adaptive locks in mainline/btrfs.
Is there no way for the short critical sections to be decoupled from the
long/sleeping ones?

Re: Btrfs for mainline

[Chris Mason]

On Sun, 2009-01-04 at 19:21 +0100, Peter Zijlstra wrote:
> On Sat, 2009-01-03 at 12:17 -0700, Matthew Wilcox wrote:
> > > - 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.
> 
> I'm really opposed to having this in some filesystem. Please remove it
> before merging it.
> 

It is 5 lines in a single function that is local to btrfs.  I'll be
happy to take it out when a clear path to a replacement is in.

I know people have been doing work in this area for -rt, and do not want
to start a parallel effort to change things.

I'm not trying to jump into the design discussions because there are
people already working on it who know the issues much better than I do. 

But, if anyone working on adaptive mutexes is looking for a coder,
tester, use case, or benchmark for their locking scheme, my hand is up.

Until then, this is my for loop, there are many like it, but this one is
mine.

-chris

Re: Btrfs for mainline

[Chris Mason]

On Sat, 2009-01-03 at 18:44 +0900, Ryusuke Konishi wrote:
> On Fri, 02 Jan 2009 14:38:07 -0500, Chris Mason 
>
> Btrfs seems to have other helpful code including pages/bio compression
> which may become separable, too.  And, this may be the same for
> pages/bio encryption/decryption code which would come next.  (I don't
> mention about the volume management/raid feature here to avoid getting
> off the subject, but it's likewise).
> 

The compression code is somewhat tied to the btrfs internals, but it
could be pulled out without too much trouble.  The big question there is
if other filesystems are interested in transparent compression support.

But, at the end of the day, most of the work is still done by the zlib
code.  The btrfs bits just organize pages to send down to zlib.

-chris

Re: Btrfs for mainline

[Chris Mason]

On Sun, 2009-01-04 at 22:52 +0100, Arnd Bergmann wrote:
> On Saturday 03 January 2009, Chris Mason wrote:
> > 
> > > Actually a lot of the ioctl API don't just need documentation but
> > > a complete redo.  That's true at least for the physical device
> > > management and subvolume / snaphot ones.
> > > 
> > 
> > The ioctl interface is definitely not finalized.  Adding more vs
> > replacing the existing ones is an open question.
> 
> As long as that's an open question, the ioctl interface shouldn't get
> merged into the kernel, or should get in as btrfsdev, otherwise you
> get stuck with the current ABI forever.
> 

Maintaining the current ioctls isn't a problem.  There aren't very many
and they do very discrete things.  The big part that may change is the
device scanning, which may get more integrated into udev and mount (see
other threads about this).

But, that is one very simple ioctl, and most of the code it uses is
going to stay regardless of how the device scanning is done.

-chris

Re: Btrfs for mainline

[Chris Mason]

On Mon, 2009-01-05 at 21:32 +1100, Nick Piggin wrote:
> On Saturday 03 January 2009 06:38:07 Chris Mason wrote:

> > The extent_map and extent_buffer code was also intended for generic use.
> > It needs some love and care (making it work for blocksize != pagesize)
> > before I'd suggest moving it out of fs/btrfs.
> 
> I'm yet to be convinced it is a good idea to use extents for this. Been a
> long time since we visited the issue, but when you converted ext2 to use
> the extent mapping stuff, it actually went slower, and complexity went up
> a lot (IIRC possibly required allocations in the writeback path).
> 
>
> So I think it is a fine idea to live in btrfs until it is more proven and
> found useful elsewhere.

It has gotten faster since then, but it makes sense to wait on moving
extent_* code.

-chris

Re: Btrfs for mainline

[Chris Mason]

On Mon, 2009-01-05 at 21:07 +1100, Chris Samuel wrote:
> On Sat, 3 Jan 2009 8:01:04 am Andi Kleen wrote:
> 
> > When it's in mainline I suspect people will start using it for that.
> 
> Some people don't even wait for that. ;-)
> 
> Seriously though, if that is a concern can I suggest taking the btrfsdev route 
> and, if you want a real belt and braces approach, perhaps require it to have a 
> mandatory mount option specified to successfully mount, maybe "eat_my_data" ?

I think ext4dev made more sense for ext4 because people generally expect
ext* to be stable.  Btrfs doesn't quite have the reputation for
stability yet, so I don't feel we need a special -dev name for it.

But, if Andrew/Linus prefer that unstable filesystems are tagged with
-dev, I'm happy to do it.

-chris

Re: Btrfs for mainline

[Nick Piggin]

On Saturday 03 January 2009 06:38:07 Chris Mason wrote:
> On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote:
> > Hi,
> >
> > On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason <chris.mason@oracle.com> 
wrote:
> > > This has only btrfs as a module and would be the fastest way to see
> > > the .c files.  btrfs doesn't have any changes outside of fs/Makefile
> > > and fs/Kconfig
>
> [ ... ]
>
> > In addition, there seem to be well-separated reusable routines such as
> > async-thread (enhanced workqueue) and extent_map.  Do you intend to
> > move these into lib/ or so?
>
> Sorry, looks like I hit send too soon that time.  The async-thread code
> is very self contained, and was intended for generic use.  Pushing that
> into lib is probably a good idea.
>
> The extent_map and extent_buffer code was also intended for generic use.
> It needs some love and care (making it work for blocksize != pagesize)
> before I'd suggest moving it out of fs/btrfs.

I'm yet to be convinced it is a good idea to use extents for this. Been a
long time since we visited the issue, but when you converted ext2 to use
the extent mapping stuff, it actually went slower, and complexity went up
a lot (IIRC possibly required allocations in the writeback path).

So I think it is a fine idea to live in btrfs until it is more proven and
found useful elsewhere.

Re: Btrfs for mainline

[Chris Samuel]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On Sat, 3 Jan 2009 8:01:04 am Andi Kleen wrote:

> When it's in mainline I suspect people will start using it for that.

Some people don't even wait for that. ;-)

Seriously though, if that is a concern can I suggest taking the btrfsdev route 
and, if you want a real belt and braces approach, perhaps require it to have a 
mandatory mount option specified to successfully mount, maybe "eat_my_data" ?

cheers,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

This email may come with a PGP signature as a file. Do not panic.
For more info see: http://en.wikipedia.org/wiki/OpenPGP


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 481 bytes --]

Re: Btrfs for mainline

[Arnd Bergmann]

On Saturday 03 January 2009, Chris Mason wrote:
> 
> > Actually a lot of the ioctl API don't just need documentation but
> > a complete redo.  That's true at least for the physical device
> > management and subvolume / snaphot ones.
> > 
> 
> The ioctl interface is definitely not finalized.  Adding more vs
> replacing the existing ones is an open question.

As long as that's an open question, the ioctl interface shouldn't get
merged into the kernel, or should get in as btrfsdev, otherwise you
get stuck with the current ABI forever.

Is it possible to separate out the nonstandard ioctls into a patch
that can get merged when the interface is final, or will that make
btrfs unusable?

	Arnd <><

Re: Btrfs for mainline

[Matthew Wilcox]

On Sun, Jan 04, 2009 at 07:21:50PM +0100, Peter Zijlstra wrote:
> The -rt tree has adaptive spin patches for the rtmutex code, its really
> not all that hard to do -- the rtmutex code is way more tricky than the
> regular mutexes due to all the PI fluff.
> 
> For kernel only locking the simple rule: spin iff the lock holder is
> running proved to be simple enough. Any added heuristics like max spin
> count etc. only made things worse. The whole idea though did make sense
> and certainly improved performance.

That implies moving

        struct thread_info      *owner;

out from under the CONFIG_DEBUG_MUTEXES code.  One of the original
justifications for mutexes was:

 - 'struct mutex' is smaller on most architectures: .e.g on x86,
   'struct semaphore' is 20 bytes, 'struct mutex' is 16 bytes.
    A smaller structure size means less RAM footprint, and better
    CPU-cache utilization.

I'd be reluctant to reverse that decision just for btrfs.

Benchmarking required!  Maybe I can put a patch together that implements
the simple 'spin if it's running' heuristic and throw it at our
testing guys on Monday ...

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

Re: Btrfs for mainline

[Peter Zijlstra]

On Sat, 2009-01-03 at 12:17 -0700, Matthew Wilcox wrote:
> > - 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.

I'm really opposed to having this in some filesystem. Please remove it
before merging it.

The -rt tree has adaptive spin patches for the rtmutex code, its really
not all that hard to do -- the rtmutex code is way more tricky than the
regular mutexes due to all the PI fluff.

For kernel only locking the simple rule: spin iff the lock holder is
running proved to be simple enough. Any added heuristics like max spin
count etc. only made things worse. The whole idea though did make sense
and certainly improved performance.

We've also been looking at doing adaptive spins for futexes, although
that does get a little more complex, furthermore, we've never gotten
around to actually doing any code on that.

Re: Btrfs for mainline

[Ed Tomlinson]

On January 4, 2009, KOSAKI Motohiro wrote:
> Hi
> 
> > One possibility would be to mimic ext4 and register the fs as "btrfsdev"
> > until it's considered stable enough for production.  I agree with the
> > consensus that we want to use the upstream kernel as a nexus for
> > coordinating btrfs development, so I don't think it's worth waiting a
> > release or two to merge something.
> 
> I like this idea.
> I also want to test btrfs. but I'm not interested out of tree code.

I'll second this.  Please get btrfsdev into mainline asap.

TIA
Ed Tomlinson

Re: Btrfs for mainline

[KOSAKI Motohiro]

Hi

> One possibility would be to mimic ext4 and register the fs as "btrfsdev"
> until it's considered stable enough for production.  I agree with the
> consensus that we want to use the upstream kernel as a nexus for
> coordinating btrfs development, so I don't think it's worth waiting a
> release or two to merge something.

I like this idea.
I also want to test btrfs. but I'm not interested out of tree code.

Re: Btrfs for mainline

[Matthew Wilcox]

On Sat, Jan 03, 2009 at 02:50:34PM -0500, Christoph Hellwig wrote:
> On Sat, Jan 03, 2009 at 12:17:06PM -0700, Matthew Wilcox wrote:
> > It's no worse than XFS (which still has its own implementation of
> > 'synchronisation variables',
> 
> Which are a trivial wrapper around wait queues.  I have patches to kill
> them, but I'm not entirely sure it's worth it

I'm not sure it's worth it either.

> > a (very thin) wrapper around mutexes,
> 
> nope.

It's down to:

typedef struct mutex mutex_t;

but it's still there.

> > a
> > (thin) wrapper around rwsems,
> 
> Which are needed so we can have asserts about the lock state, which
> generic rwsems still don't have.  At some pointer Peter looked into
> it, and once we have that we can kill the wrapper.

Good to know.  Rather like btrfs's wrappers around mutexes then ...

> > > - compat.h needs to go
> > 
> > Later.  It's still there for XFS.
> 
> ?

XFS still has 'fs/xfs/linux-2.6'.  It's a little bigger than compat.h,
for sure, and doesn't contain code for supporting different Linux
versions, sure.  But it's still a compat layer.

> > > - there should be manpages for all the ioctls and other interfaces.
> > 
> > I wonder if Michael Kerrisk has time to help with that.  Cc'd.
> 
> Actually a lot of the ioctl API don't just need documentation but
> a complete redo.  That's true at least for the physical device
> management and subvolume / snaphot ones.

That's a more important critique than Andi's.  Let's take care of that.

> From painfull experience with a lot of things, including a filesystem
> you keep on mentioning it's clear that once stuff is upstream there
> is very little to no incentive to fix these things up.

I don't think that's as true of btrfs as it was of XFS -- for example,
Chris has no incentive to keep compatibility with IRIX, or continue to
support CXFS.  I don't think 'getting included in kernel' is Chris'
goal, so much as it is a step towards making btrfs better.

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

Re: Btrfs for mainline

[Chris Mason]

On Sat, 2009-01-03 at 14:50 -0500, Christoph Hellwig wrote:
> On Sat, Jan 03, 2009 at 12:17:06PM -0700, Matthew Wilcox wrote:
>  
> > > - compat.h needs to go
> > 
> > Later.  It's still there for XFS.
> 
> ?

> > > - there should be manpages for all the ioctls and other interfaces.
> > 
> > I wonder if Michael Kerrisk has time to help with that.  Cc'd.
> 
> Actually a lot of the ioctl API don't just need documentation but
> a complete redo.  That's true at least for the physical device
> management and subvolume / snaphot ones.
> 

The ioctl interface is definitely not finalized.  Adding more vs
replacing the existing ones is an open question.

> > > - 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.
> 
> >From painfull experience with a lot of things, including a filesystem
> you keep on mentioning it's clear that once stuff is upstream there
> is very little to no incentive to fix these things up.
> 

I'd disagree here.  Cleanup incentive is a mixture of the people
involved and the attention the project has.

-chris

Re: Btrfs for mainline

[Christoph Hellwig]

On Sat, Jan 03, 2009 at 12:17:06PM -0700, Matthew Wilcox wrote:
> It's no worse than XFS (which still has its own implementation of
> 'synchronisation variables',

Which are a trivial wrapper around wait queues.  I have patches to kill
them, but I'm not entirely sure it's worth it

> a (very thin) wrapper around mutexes,

nope.

> a
> (thin) wrapper around rwsems,

Which are needed so we can have asserts about the lock state, which
generic rwsems still don't have.  At some pointer Peter looked into
it, and once we have that we can kill the wrapper.

and wrappers around kmalloc and kmem_cache.
> 
> > - compat.h needs to go
> 
> Later.  It's still there for XFS.

?

> > - there should be manpages for all the ioctls and other interfaces.
> 
> I wonder if Michael Kerrisk has time to help with that.  Cc'd.

Actually a lot of the ioctl API don't just need documentation but
a complete redo.  That's true at least for the physical device
management and subvolume / snaphot ones.

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

>From painfull experience with a lot of things, including a filesystem
you keep on mentioning it's clear that once stuff is upstream there
is very little to no incentive to fix these things up.

Re: Btrfs for mainline

[Matthew Wilcox]

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

Re: Btrfs for mainline

[Ryusuke Konishi]

On Fri, 02 Jan 2009 14:38:07 -0500, Chris Mason <chris.mason@oracle.com> wrote:
> On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote:
> > In addition, there seem to be well-separated reusable routines such as
> > async-thread (enhanced workqueue) and extent_map.  Do you intend to
> > move these into lib/ or so?
> 
> Sorry, looks like I hit send too soon that time.  The async-thread code
> is very self contained, and was intended for generic use.  Pushing that
> into lib is probably a good idea.

As for async-thread, kernel/ seems to be better place (sorry, I also hit
send too soon ;)

Anyway, I think it should be reviewed deeply by scheduler people and
wider range of people.  So, it's a good idea to put it out in order to
arouse interest.
 
> The extent_map and extent_buffer code was also intended for generic use.
> It needs some love and care (making it work for blocksize != pagesize)
> before I'd suggest moving it out of fs/btrfs.
> 
> -chris

The extent_map itself seemed independent of the problem to me, but I
understand your plan.

Btrfs seems to have other helpful code including pages/bio compression
which may become separable, too.  And, this may be the same for
pages/bio encryption/decryption code which would come next.  (I don't
mention about the volume management/raid feature here to avoid getting
off the subject, but it's likewise).

I think it's wonderful if they can be well-integrated into sublayers
or libraries.

Regards,
Ryusuke

Re: Btrfs for mainline

[Roland Dreier]

 > > > I don't disagree, please do keep in mind that I'm not suggesting anyone
 > > > use this in production yet.

 > > When it's in mainline I suspect people will start using it for that.

 > I think the larger question here is where we want development to happen.
 > I'm definitely not pretending that btrfs is perfect, but I strongly
 > believe that it will be a better filesystem if the development moves to
 > mainline where it will attract more eyeballs and more testers.

One possibility would be to mimic ext4 and register the fs as "btrfsdev"
until it's considered stable enough for production.  I agree with the
consensus that we want to use the upstream kernel as a nexus for
coordinating btrfs development, so I don't think it's worth waiting a
release or two to merge something.

 - R.

Re: Btrfs for mainline

[Chris Mason]

On Fri, 2009-01-02 at 22:01 +0100, Andi Kleen wrote:
> On Fri, Jan 02, 2009 at 02:32:29PM -0500, Chris Mason wrote:
> > > 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.
> > 
> > Sure, I'm happy to use a generic framework there (or help create one).
> > They are definitely a win for btrfs, and show up in most benchmarks.
> 
> If they are such a big win then likely they will help other users
> too and should be generic in some form.
> 

I don't disagree.  It's about 6 lines of code though, and just hasn't
been at the top of my list.  I'm sure the generic version will be
faster, as it could add checks to see if the holder of the lock was
actually running.

> > 
> > > - compat.h needs to go
> > 
> > Projects that are out of mainline have a difficult task of making sure
> > development can continue until they are in mainline and being clean
> > enough to merge.  I'd rather get rid of the small amount of compat code
> > that I have left after btrfs is in (compat.h is 32 lines).  
> 
> It's fine for an out of tree variant, but the in tree version
> shouldn't have compat.h. For out of tree you just apply a patch
> that adds the includes. e.g.compat-wireless and lots of other
> projects do it this way. 
> 

It helps debugging that my standalone tree is generated from and exactly
the same as fs/btrfs in the full kernel tree.  I'll switch to a pull and
merge system for the standalone tree.

> > 
> > Yes, I tried to mark those as I did them (a very small number of
> > functions).  In general they were copied to avoid adding exports, and
> > that is easily fixed.
> > 
> > > - there should be manpages for all the ioctls and other interfaces.
> > > - ioctl.c was not explicitely root protected. security issues?
> > 
> > Christoph added a CAP_SYS_ADMIN check to the trans start ioctl, but I do
> > need to add one to the device add/remove/balance code as well.
> 
> Ok. Didn't see that.
> 
> It still needs to be carefully audited for security holes 
> even with root checks.

Yes, the most important one is the device scan ioctl (also missing the
root check, will fix).

> 
> Another thing is that once auto mounting is enabled each usb stick
> with btrfs on it could be a root hole if you have buffer overflows
> somewhere triggerable by disk data. I guess that would need some
> checking too.
> 
> > The subvol/snapshot creation is meant to be user callable (controlled by
> > something similar to quotas later on).
> 
> But right now that's not there so it should be root only.
> 

I'll switch to checking against directory permissions for now.
subvol/snapshot creation are basically mkdir anyway, so this fits well.

> > 
> > >  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.
> > > - various checkpath.pl level problems I think (e.g. printk levels) 
> > > - the printks should all include which file system they refer to
> > > 
> > > In general I think the whole thing needs more review.
> > 
> > I don't disagree, please do keep in mind that I'm not suggesting anyone
> > use this in production yet.
> 
> When it's in mainline I suspect people will start using it for that.

I think the larger question here is where we want development to happen.
I'm definitely not pretending that btrfs is perfect, but I strongly
believe that it will be a better filesystem if the development moves to
mainline where it will attract more eyeballs and more testers.

-chris

Re: Btrfs for mainline

[Andi Kleen]

On Fri, Jan 02, 2009 at 02:32:29PM -0500, Chris Mason wrote:
> > 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.
> 
> Sure, I'm happy to use a generic framework there (or help create one).
> They are definitely a win for btrfs, and show up in most benchmarks.

If they are such a big win then likely they will help other users
too and should be generic in some form.

> 
> > - compat.h needs to go
> 
> Projects that are out of mainline have a difficult task of making sure
> development can continue until they are in mainline and being clean
> enough to merge.  I'd rather get rid of the small amount of compat code
> that I have left after btrfs is in (compat.h is 32 lines).  

It's fine for an out of tree variant, but the in tree version
shouldn't have compat.h. For out of tree you just apply a patch
that adds the includes. e.g.compat-wireless and lots of other
projects do it this way. 

> 
> Yes, I tried to mark those as I did them (a very small number of
> functions).  In general they were copied to avoid adding exports, and
> that is easily fixed.
> 
> > - there should be manpages for all the ioctls and other interfaces.
> > - ioctl.c was not explicitely root protected. security issues?
> 
> Christoph added a CAP_SYS_ADMIN check to the trans start ioctl, but I do
> need to add one to the device add/remove/balance code as well.

Ok. Didn't see that.

It still needs to be carefully audited for security holes 
even with root checks.

Another thing is that once auto mounting is enabled each usb stick
with btrfs on it could be a root hole if you have buffer overflows
somewhere triggerable by disk data. I guess that would need some
checking too.

> The subvol/snapshot creation is meant to be user callable (controlled by
> something similar to quotas later on).

But right now that's not there so it should be root only.

> 
> >  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.
> > - various checkpath.pl level problems I think (e.g. printk levels) 
> > - the printks should all include which file system they refer to
> > 
> > In general I think the whole thing needs more review.
> 
> I don't disagree, please do keep in mind that I'm not suggesting anyone
> use this in production yet.

When it's in mainline I suspect people will start using it for that.

-Andi

-- 
ak@linux.intel.com

Re: Btrfs for mainline

[Chris Mason]

On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote:
> Hi,
> On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason <chris.mason@oracle.com> wrote:
> > 
> > This has only btrfs as a module and would be the fastest way to see
> > the .c files.  btrfs doesn't have any changes outside of fs/Makefile and
> > fs/Kconfig

[ ... ]

> In addition, there seem to be well-separated reusable routines such as
> async-thread (enhanced workqueue) and extent_map.  Do you intend to
> move these into lib/ or so?

Sorry, looks like I hit send too soon that time.  The async-thread code
is very self contained, and was intended for generic use.  Pushing that
into lib is probably a good idea.

The extent_map and extent_buffer code was also intended for generic use.
It needs some love and care (making it work for blocksize != pagesize)
before I'd suggest moving it out of fs/btrfs.

-chris

Re: Btrfs for mainline

[Chris Mason]

On Fri, 2009-01-02 at 20:05 +0100, Andi Kleen wrote:
> Chris Mason <chris.mason@oracle.com> writes:
> 
> > On Wed, 2008-12-31 at 10:45 -0800, Andrew Morton wrote:
> >> On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason <chris.mason@oracle.com> wrote:
> >> 
> >> > Hello everyone,
> >> 
> >> Hi!
> >> 
> >> > I've done some testing against Linus' git tree from last night and the
> >> > current btrfs trees still work well.
> >> 
> >> what's btrfs?  I think I've heard the name before, but I've never
> >> seen the patches :)
> >
> > The source is up to around 38k loc, I thought it better to use that http
> > thing for people who were interested in the code.
> >
> > There is also a standalone git repo:
> >
> > http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable-standalone.git;a=summary
> 
> 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):
> 

Hi Andi, thanks for looking at things.

> - locking.c needs a lot of cleanup.

Grin, lots of the code needs to be cleaned, but locking.c is really just
a few wrappers around the mutex calls.  I don't think I had it at the
top of my to-be-cleaned list ;)


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

Sure, I'm happy to use a generic framework there (or help create one).
They are definitely a win for btrfs, and show up in most benchmarks.

> - compat.h needs to go

Projects that are out of mainline have a difficult task of making sure
development can continue until they are in mainline and being clean
enough to merge.  I'd rather get rid of the small amount of compat code
that I have left after btrfs is in (compat.h is 32 lines).  

It isn't hurting anything, and taking it out makes it much more
difficult for our current users.

> - there's various copy'n'pasted code from the VFS (like may_create) 
> that needs to be cleaned up.

Yes, I tried to mark those as I did them (a very small number of
functions).  In general they were copied to avoid adding exports, and
that is easily fixed.

> - there should be manpages for all the ioctls and other interfaces.
> - ioctl.c was not explicitely root protected. security issues?

Christoph added a CAP_SYS_ADMIN check to the trans start ioctl, but I do
need to add one to the device add/remove/balance code as well.

The subvol/snapshot creation is meant to be user callable (controlled by
something similar to quotas later on).

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

Yes, there's quite some work to do in the error handling paths.

>  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.
> - various checkpath.pl level problems I think (e.g. printk levels) 
> - the printks should all include which file system they refer to
> 
> In general I think the whole thing needs more review.

I don't disagree, please do keep in mind that I'm not suggesting anyone
use this in production yet.

-chris

Re: Btrfs for mainline

[Andi Kleen]

Chris Mason <chris.mason@oracle.com> writes:

> On Wed, 2008-12-31 at 10:45 -0800, Andrew Morton wrote:
>> On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason <chris.mason@oracle.com> wrote:
>> 
>> > Hello everyone,
>> 
>> Hi!
>> 
>> > I've done some testing against Linus' git tree from last night and the
>> > current btrfs trees still work well.
>> 
>> what's btrfs?  I think I've heard the name before, but I've never
>> seen the patches :)
>
> The source is up to around 38k loc, I thought it better to use that http
> thing for people who were interested in the code.
>
> There is also a standalone git repo:
>
> http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable-standalone.git;a=summary

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.
- compat.h needs to go
- there's various copy'n'pasted code from the VFS (like may_create) 
that needs to be cleaned up.
- there should be manpages for all the ioctls and other interfaces.
- ioctl.c was not explicitely root protected. security issues?
- 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.
- 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.
- various checkpath.pl level problems I think (e.g. printk levels) 
- the printks should all include which file system they refer to

In general I think the whole thing needs more review.

-Andi

-- 
ak@linux.intel.com

Re: Btrfs for mainline

[Chris Mason]

On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote:
> Hi,
> On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason <chris.mason@oracle.com> wrote:
> > 
> > This has only btrfs as a module and would be the fastest way to see
> > the .c files.  btrfs doesn't have any changes outside of fs/Makefile and
> > fs/Kconfig
> 
> I found some overlapping (or cloned) functions in
> btrfs-unstable.git/fs/btrfs, for example:
> 
>  - Declarations to apply hardware crc32c in fs/btrfs/crc32c.h:
>    The same code is found in arch/x86/crypto/crc32c-intel.c
> 

Yes, I can just remove the btrfs version of this for now.

>  - btrfs_wait_on_page_writeback_range() and btrfs_fdatawrite_range():
>    These are clones of wait_on_page_writeback_range() and
>    __filemap_fdatawrite_range() respectively, and can be removed if they
>    are just exported.
> 
>  - Copies of add_to_page_cache_lru() found in compression.c and extent_io.c
>    (can be replaced if it's exported)
> 
> How about including patches to resolve these in the btrfs kernel tree
> (or patchset to be posted) ?
> 

My plan was to export those after btrfs was actually in.  But on Monday
I'll send along a patch to export them and make compat functions in
btrfs.

> In addition, there seem to be well-separated reusable routines such as
> async-thread (enhanced workqueue) and extent_map.  Do you intend to
> move these into lib/ or so?
> 
> I also tried scripts/checkpatch.pl against btrfs, and it has detected
> 45 ERRORs and 93 WARNINGs.  I think it's a good opportunity to clean
> up these violations.

Good point, thanks for looking at the code.

-chris

Re: Btrfs for mainline

[Ryusuke Konishi]

Hi,
On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason <chris.mason@oracle.com> wrote:
> 
> This has only btrfs as a module and would be the fastest way to see
> the .c files.  btrfs doesn't have any changes outside of fs/Makefile and
> fs/Kconfig

I found some overlapping (or cloned) functions in
btrfs-unstable.git/fs/btrfs, for example:

 - Declarations to apply hardware crc32c in fs/btrfs/crc32c.h:
   The same code is found in arch/x86/crypto/crc32c-intel.c

 - btrfs_wait_on_page_writeback_range() and btrfs_fdatawrite_range():
   These are clones of wait_on_page_writeback_range() and
   __filemap_fdatawrite_range() respectively, and can be removed if they
   are just exported.

 - Copies of add_to_page_cache_lru() found in compression.c and extent_io.c
   (can be replaced if it's exported)

How about including patches to resolve these in the btrfs kernel tree
(or patchset to be posted) ?

In addition, there seem to be well-separated reusable routines such as
async-thread (enhanced workqueue) and extent_map.  Do you intend to
move these into lib/ or so?

I also tried scripts/checkpatch.pl against btrfs, and it has detected
45 ERRORs and 93 WARNINGs.  I think it's a good opportunity to clean
up these violations.


With regards,
Ryusuke Konishi

Re: Btrfs for mainline

[Chris Mason]

On Wed, 2008-12-31 at 10:45 -0800, Andrew Morton wrote:
> On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason <chris.mason@oracle.com> wrote:
> 
> > Hello everyone,
> 
> Hi!
> 
> > I've done some testing against Linus' git tree from last night and the
> > current btrfs trees still work well.
> 
> what's btrfs?  I think I've heard the name before, but I've never
> seen the patches :)

The source is up to around 38k loc, I thought it better to use that http
thing for people who were interested in the code.

There is also a standalone git repo:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable-standalone.git;a=summary

This has only btrfs as a module and would be the fastest way to see
the .c files.  btrfs doesn't have any changes outside of fs/Makefile and
fs/Kconfig

(happy new year ;)

-chris

Re: Btrfs for mainline

[Andrew Morton]

On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason <chris.mason@oracle.com> wrote:

> Hello everyone,

Hi!

> I've done some testing against Linus' git tree from last night and the
> current btrfs trees still work well.

what's btrfs?  I think I've heard the name before, but I've never
seen the patches :)

Btrfs for mainline

[Chris Mason]

Hello everyone,

I've done some testing against Linus' git tree from last night and the
current btrfs trees still work well.

There are a few bug fixes that I need to include from while I was on
vacation but I haven't made any large changes since early in December:

Btrfs details and usage information can be found:

http://btrfs.wiki.kernel.org/

The btrfs kernel code is here:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=summary

And the utilities are here:

http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-progs-unstable.git;a=summary

-chris