Btrfs devel - Oct 2010 - [PATCH] Btrfs: add support for mixed data+metadata block groups V3

There are just a few things that need to be fixed in the kernel to support mixed
data+metadata block groups.  Mostly we just need to make sure that if we are
using mixed block groups that we continue to allocate mixed block groups as we
need them.  Also we need to make sure __find_space_info will find our space info
if we search for DATA or METADATA only.  Tested this with xfstests and it works
nicely.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
V2->V3: Add btrfs_mixed_space_info helper

V1->V2: In do_chunk_alloc I was changing flags to == space_info->flags,
which
isn''t right since space_info doesn''t carry the RAID profiles
anymore, so instead
check to see if the space info has DATA and METADATA set and if so set that in
the flags as well.
 fs/btrfs/ctree.h       |    6 ++++++
 fs/btrfs/extent-tree.c |   22 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 1ecd8f6..86a4f4b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2043,6 +2043,12 @@ static inline struct dentry *fdentry(struct file *file)
 	return file->f_path.dentry;
 }
 
+static inline bool btrfs_mixed_space_info(struct btrfs_space_info *space_info)
+{
+	return ((space_info->flags & BTRFS_BLOCK_GROUP_METADATA) &&
+		(space_info->flags & BTRFS_BLOCK_GROUP_DATA));
+}
+
 /* extent-tree.c */
 void btrfs_put_block_group(struct btrfs_block_group_cache *cache);
 int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 91a0a41..ed84271 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -547,7 +547,7 @@ static struct btrfs_space_info *__find_space_info(struct
btrfs_fs_info *info,
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(found, head, list) {
-		if (found->flags == flags) {
+		if (found->flags & flags) {
 			rcu_read_unlock();
 			return found;
 		}
@@ -3267,6 +3267,13 @@ static int do_chunk_alloc(struct btrfs_trans_handle
*trans,
 	spin_unlock(&space_info->lock);
 
 	/*
+	 * If we have mixed data/metadata chunks we want to make sure we keep
+	 * allocating mixed chunks instead of individual chunks.
+	 */
+	if (btrfs_mixed_space_info(space_info))
+		flags |= (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA);
+
+	/*
 	 * if we''re doing a data chunk, go ahead and make sure that
 	 * we keep a reasonable number of metadata chunks allocated in the
 	 * FS as well.
@@ -4793,6 +4800,7 @@ static noinline int find_free_extent(struct
btrfs_trans_handle *trans,
 	bool found_uncached_bg = false;
 	bool failed_cluster_refill = false;
 	bool failed_alloc = false;
+	bool use_cluster = true;
 	u64 ideal_cache_percent = 0;
 	u64 ideal_cache_offset = 0;
 
@@ -4807,16 +4815,24 @@ static noinline int find_free_extent(struct
btrfs_trans_handle *trans,
 		return -ENOSPC;
 	}
 
+	/*
+	 * If the space info is for both data and metadata it means we have a
+	 * small filesystem and we can''t use the clustering stuff.
+	 */
+	if (btrfs_mixed_space_info(space_info))
+		use_cluster = false;
+
 	if (orig_root->ref_cows || empty_size)
 		allowed_chunk_alloc = 1;
 
-	if (data & BTRFS_BLOCK_GROUP_METADATA) {
+	if (data & BTRFS_BLOCK_GROUP_METADATA && use_cluster) {
 		last_ptr = &root->fs_info->meta_alloc_cluster;
 		if (!btrfs_test_opt(root, SSD))
 			empty_cluster = 64 * 1024;
 	}
 
-	if ((data & BTRFS_BLOCK_GROUP_DATA) && btrfs_test_opt(root, SSD))
{
+	if ((data & BTRFS_BLOCK_GROUP_DATA) && use_cluster &&
+	    btrfs_test_opt(root, SSD)) {
 		last_ptr = &root->fs_info->data_alloc_cluster;
 	}
 
-- 
1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I''ve been testing this patch (as well as the accompanying patch to
btrfs-progs).

It seems to save a decent amount of space (maybe 10-20% according to
df in my testing, YMMV), but I was also noticing a performance penalty
of maybe 5-15%, depending on the application (in my case, I was timing
the untar-ing of data to a btrfs partition).

I also developed the perception that I was encountering some minor
latency issues when heavily using a drive formated for mixed data and
metadata (more keyboard and mouse hesitations than normal, difficult
stuff to quantify).

The main problem I encountered was a btrfs crash when booting with an
un-patched kernel, and letting the boot process attempt to mount the
drive with mixed data+metadata.  I wasn''t entirely surprised by this
result, but it was unclear from the patch description whether a volume
formatted with mixed data+metadata would be incompatible with an
un-patched, older kernel.

Other than that, it seemed to perform well.  I did not encounter any
stability issues as long as I was using a patched kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Does this mixing constitute a forbidden change of on-disk format, and
if not how not?


On Wed, Oct 20, 2010 at 11:21 PM, Mitch Harder
<mitch.harder@sabayonlinux.org> wrote:
> Other than that, it seemed to perform well.  I did not encounter any
> stability issues as long as I was using a patched kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol
escribió:
> Does this mixing constitute a forbidden change of on-disk format, and
> if not how not?
It doesn''t need a format change. The difference between a data and
a metadata block group is just an allocation hint AFAIK.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 21, 2010 at 5:09 PM, Diego Calleja <diegocg@gmail.com>
wrote:
> On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol escribió:
>> Does this mixing constitute a forbidden change of on-disk format, and
>> if not how not?
>
> It doesn''t need a format change. The difference between a data and
> a metadata block group is just an allocation hint AFAIK.
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Let me know if the problems with an un-patched kernel were un-expected.

I can provide more information on the crash when booting an older kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 21, 2010 at 05:21:06PM -0500, Mitch Harder
wrote:
> On Thu, Oct 21, 2010 at 5:09 PM, Diego Calleja <diegocg@gmail.com>
wrote:
> > On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol escribió:
> >> Does this mixing constitute a forbidden change of on-disk format,
and
> >> if not how not?
> >
> > It doesn''t need a format change. The difference between a
data and
> > a metadata block group is just an allocation hint AFAIK.
> > --
> > To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Let me know if the problems with an un-patched kernel were un-expected.
> 
> I can provide more information on the crash when booting an older kernel.
Nope they are expected, it''s not a disk format change, but older
kernels won''t
deal with mixed block groups.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 21, 2010 at 8:05 PM, Josef Bacik <josef@redhat.com>
wrote:
> On Thu, Oct 21, 2010 at 05:21:06PM -0500, Mitch Harder wrote:
>> On Thu, Oct 21, 2010 at 5:09 PM, Diego Calleja
<diegocg@gmail.com> wrote:
>> > On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol escribió:
>> >> Does this mixing constitute a forbidden change of on-disk
format, and
>> >> if not how not?
>> >
>> > It doesn''t need a format change. The difference between a
data and
>> > a metadata block group is just an allocation hint AFAIK.
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>>
>> Let me know if the problems with an un-patched kernel were un-expected.
>>
>> I can provide more information on the crash when booting an older
kernel.
>
> Nope they are expected, it''s not a disk format change, but older
kernels won''t
> deal with mixed block groups.
When something like this goes mainline, is it used by default/automatically?

I ask because I maintain a btrfs-based rollback initramfs hook [1],
and am currently updating it for extlinux, enabling kernel-level
system rollbacks via `btrfs set-default` + reboot (or maybe
`kexec`)...

rolling back to an old kernel will then blow up my machine
(figuratively of course :-)?

C Anthony

[1] http://aur.archlinux.org/packages.php?ID=33376
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 21, 2010 at 08:32:12PM -0500, C Anthony Risinger
wrote:
> On Thu, Oct 21, 2010 at 8:05 PM, Josef Bacik <josef@redhat.com>
wrote:
> > On Thu, Oct 21, 2010 at 05:21:06PM -0500, Mitch Harder wrote:
> >> On Thu, Oct 21, 2010 at 5:09 PM, Diego Calleja
<diegocg@gmail.com> wrote:
> >> > On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol
escribió:
> >> >> Does this mixing constitute a forbidden change of on-disk
format, and
> >> >> if not how not?
> >> >
> >> > It doesn''t need a format change. The difference
between a data and
> >> > a metadata block group is just an allocation hint AFAIK.
> >> > --
> >> > To unsubscribe from this list: send the line
"unsubscribe linux-btrfs" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at
 http://vger.kernel.org/majordomo-info.html
> >> >
> >>
> >> Let me know if the problems with an un-patched kernel were
un-expected.
> >>
> >> I can provide more information on the crash when booting an older
kernel.
> >
> > Nope they are expected, it''s not a disk format change, but
older kernels won''t
> > deal with mixed block groups.
> 
> When something like this goes mainline, is it used by
default/automatically?
> 
> I ask because I maintain a btrfs-based rollback initramfs hook [1],
> and am currently updating it for extlinux, enabling kernel-level
> system rollbacks via `btrfs set-default` + reboot (or maybe
> `kexec`)...
> 
> rolling back to an old kernel will then blow up my machine
> (figuratively of course :-)?
>
The only way you get this feature is if you mkfs with the feature enabled, and
is only meant for small filesystems (1 gig or smaller).  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 21, 2010 at 8:37 PM, Josef Bacik <josef@redhat.com>
wrote:
> On Thu, Oct 21, 2010 at 08:32:12PM -0500, C Anthony Risinger wrote:
>> On Thu, Oct 21, 2010 at 8:05 PM, Josef Bacik <josef@redhat.com>
wrote:
>> > On Thu, Oct 21, 2010 at 05:21:06PM -0500, Mitch Harder wrote:
>> >> On Thu, Oct 21, 2010 at 5:09 PM, Diego Calleja
<diegocg@gmail.com> wrote:
>> >> > On Jueves, 21 de Octubre de 2010 17:46:58 David Nicol
escribió:
>> >> >> Does this mixing constitute a forbidden change of
on-disk format, and
>> >> >> if not how not?
>> >> >
>> >> > It doesn''t need a format change. The difference
between a data and
>> >> > a metadata block group is just an allocation hint AFAIK.
>> >> > --
>> >> > To unsubscribe from this list: send the line
"unsubscribe linux-btrfs" in
>> >> > the body of a message to majordomo@vger.kernel.org
>> >> > More majordomo info at
 http://vger.kernel.org/majordomo-info.html
>> >> >
>> >>
>> >> Let me know if the problems with an un-patched kernel were
un-expected.
>> >>
>> >> I can provide more information on the crash when booting an
older kernel.
>> >
>> > Nope they are expected, it''s not a disk format change,
but older kernels won''t
>> > deal with mixed block groups.
>>
>> When something like this goes mainline, is it used by
default/automatically?
>>
>> I ask because I maintain a btrfs-based rollback initramfs hook [1],
>> and am currently updating it for extlinux, enabling kernel-level
>> system rollbacks via `btrfs set-default` + reboot (or maybe
>> `kexec`)...
>>
>> rolling back to an old kernel will then blow up my machine
>> (figuratively of course :-)?
>>
>
> The only way you get this feature is if you mkfs with the feature enabled,
and
> is only meant for small filesystems (1 gig or smaller).
Ah right :-), thanks

C Anthony
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html