Re: Btrfs for mainline
> > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
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 -- a...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote: > Hi, > On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
On Fri, 2009-01-02 at 20:05 +0100, Andi Kleen wrote: > Chris Mason writes: > > > On Wed, 2008-12-31 at 10:45 -0800, Andrew Morton wrote: > >> On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
Chris Mason writes: > On Wed, 2008-12-31 at 10:45 -0800, Andrew Morton wrote: >> On Wed, 31 Dec 2008 06:28:55 -0500 Chris Mason >> 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 -- a...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
On Sat, 2009-01-03 at 01:37 +0900, Ryusuke Konishi wrote: > Hi, > On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Btrfs for mainline
Hi, On Wed, 31 Dec 2008 18:19:09 -0500, Chris Mason 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
