[PATCH 0/4] Metadata crossing stripe boundary fixes

[PATCH 0/4] Metadata crossing stripe boundary fixes

[Qu Wenruo]

The problem is originally reported by Chris Murphy<lists@colorremedies.com>,
and Zhao Lei digs out the root cause:
Metadata extent in mixed block group may cross stripe boundary, causing
scrub can't handle them.

For normal data/metadata separated case, as BTRFS_STRIPE_LEN(64K) can
always be divided by nodesize (4~64K, power of 2), so metadata will
never cross the stripe boundary.
But for mixed block group, data is page size(4K) aligned, breaking the
original metadata alignment, make metadata extent possible to cross
stripe boundary.

In the patchset, btrfsck will report such error and normal extent
allocate routine along with btrfs-convert will be patched to fix such
problem.

I'd like to add a test case, but it is already covered by the existing
convert test, and further more, btrfsck can only report the error with
only the 2nd patch applied.
So no good test case yet.

Kernel also needs such check, I'll check kernel codes later.

Qu Wenruo (4):
  btrfs: print-tree: print stripe len of a chunk
  btrfs: fsck: Check if a metadata tree block crossing stripe boundary
  btrfs: extent-tree: Avoid allocating tree block that cross stripe    
    boundary
  btrfs: convert: Avoid allocating metadata extent crossing stripe    
    boundary

 btrfs-convert.c | 15 ++++++++++++---
 cmds-check.c    | 28 +++++++++++++++++++++++++++-
 ctree.h         |  2 +-
 extent-tree.c   |  7 ++++++-
 print-tree.c    |  4 +++-
 volumes.h       | 10 ++++++++++
 6 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.4.6

[PATCH 1/4] btrfs: print-tree: print stripe len of a chunk

[Qu Wenruo]

Although it is fixed to BTRFS_STRIPE_LEN(64K) now, it's still used in a
lot of codes, just output it for user who want to trace the source of
stripe_len in btrfs_map_bio() codes.

Reported-by: Chris Murphy <lists@colorremedies.com>
Reported-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 print-tree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/print-tree.c b/print-tree.c
index a72a979..dc1d276 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -224,9 +224,11 @@ void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk)
 	char chunk_flags_str[32] = {0};
 
 	bg_flags_to_str(btrfs_chunk_type(eb, chunk), chunk_flags_str);
-	printf("\t\tchunk length %llu owner %llu type %s num_stripes %d\n",
+	printf("\t\tchunk length %llu owner %llu stripe_len %llu\n",
 	       (unsigned long long)btrfs_chunk_length(eb, chunk),
 	       (unsigned long long)btrfs_chunk_owner(eb, chunk),
+	       (unsigned long long)btrfs_chunk_stripe_len(eb, chunk));
+	printf("\t\ttype %s num_stripes %d\n",
 	       chunk_flags_str, num_stripes);
 	for (i = 0 ; i < num_stripes ; i++) {
 		printf("\t\t\tstripe %d devid %llu offset %llu\n", i,
-- 
2.4.6

[PATCH 2/4] btrfs: fsck: Check if a metadata tree block crossing stripe boundary

[Qu Wenruo]

Kernel btrfs_map_block() function has a limitation that it can only
map BTRFS_STRIPE_LEN size.
That will cause scrub fails to scrub tree block which crosses strip
boundary, causing BUG_ON().

Normally, it's OK as metadata is always in metadata chunk and
BTRFS_STRIPE_LEN can always be divided by node/leaf size.
So without mixed block group, tree block won't cross stripe boundary.

But for mixed block group, especially for converted btrfs from ext4,
it's almost sure one or more tree blocks are not aligned with node size
and cross stripe boundary.
Causing bug with kernel scrub.

This patch will report the problem, although we don't have a good idea
to fix it in user space until we add the ability to relocate tree block
in user space.

Also, kernel codes should also be checked for such tree block alloc
problem.

Reported-by: Chris Murphy <lists@colorremedies.com>
Reported-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 cmds-check.c | 28 +++++++++++++++++++++++++++-
 volumes.h    | 10 ++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index dd2fce3..49c1f4a 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -126,6 +126,7 @@ struct extent_record {
 	unsigned int is_root:1;
 	unsigned int metadata:1;
 	unsigned int bad_full_backref:1;
+	unsigned int crossing_stripes:1;
 };
 
 struct inode_backref {
@@ -3734,7 +3735,7 @@ static int maybe_free_extent_rec(struct cache_tree *extent_cache,
 	if (rec->content_checked && rec->owner_ref_checked &&
 	    rec->extent_item_refs == rec->refs && rec->refs > 0 &&
 	    rec->num_duplicates == 0 && !all_backpointers_checked(rec, 0) &&
-	    !rec->bad_full_backref) {
+	    !rec->bad_full_backref && !rec->crossing_stripes) {
 		remove_cache_extent(extent_cache, &rec->cache);
 		free_all_extent_backrefs(rec);
 		list_del_init(&rec->list);
@@ -4381,6 +4382,15 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 		if (rec->max_size < max_size)
 			rec->max_size = max_size;
 
+		/*
+		 * for metadata extent, it can't cross stripe_len boundary, or
+		 * kernel scrub can't handle it
+		 * And now stripe_len is fixed to BTRFS_STRIPE_LEN yet,
+		 * just check it.
+		 */
+		if (metadata && check_crossing_stripes(rec->start,
+						       rec->max_size))
+				rec->crossing_stripes = 1;
 		maybe_free_extent_rec(extent_cache, rec);
 		return ret;
 	}
@@ -4433,6 +4443,10 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 		rec->content_checked = 1;
 		rec->owner_ref_checked = 1;
 	}
+
+	if (metadata)
+		if (check_crossing_stripes(rec->start, rec->max_size))
+			rec->crossing_stripes = 1;
 	return ret;
 }
 
@@ -7478,6 +7492,18 @@ static int check_extent_refs(struct btrfs_root *root,
 			err = 1;
 			cur_err = 1;
 		}
+		/*
+		 * Although it's not a extent ref problem, still reuse this
+		 * routine for error reporting
+		 * No repair function yet.
+		 */
+		if (rec->crossing_stripes) {
+			fprintf(stderr,
+				"bad metadata [%llu, %llu) crossing stripe boundary\n",
+				rec->start, rec->start + rec->max_size);
+			err = 1;
+			cur_err = 1;
+		}
 
 		remove_cache_extent(extent_cache, cache);
 		free_all_extent_backrefs(rec);
diff --git a/volumes.h b/volumes.h
index 99a3fa1..71d5d66 100644
--- a/volumes.h
+++ b/volumes.h
@@ -148,6 +148,16 @@ struct map_lookup {
 #define BTRFS_RAID5_P_STRIPE ((u64)-2)
 #define BTRFS_RAID6_Q_STRIPE ((u64)-1)
 
+/*
+ * Check if the given range cross stripes.
+ * To ensure kernel scrub won't causing bug on with METADATA in mixed
+ * block group
+ */
+static inline int check_crossing_stripes(u64 start, u64 len)
+{
+	return (start / BTRFS_STRIPE_LEN) !=
+	       ((start + len) / BTRFS_STRIPE_LEN);
+}
 
 int __btrfs_map_block(struct btrfs_mapping_tree *map_tree, int rw,
 		      u64 logical, u64 *length, u64 *type,
-- 
2.4.6

[PATCH 3/4] btrfs: extent-tree: Avoid allocating tree block that cross stripe boundary

[Qu Wenruo]

Now find_free_extent() function won't return a metadata extent that
cross stripe boundary.

Reported-by: Chris Murphy <lists@colorremedies.com>
Reported-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 extent-tree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/extent-tree.c b/extent-tree.c
index ac582e0..6f07e4b 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2605,6 +2605,11 @@ check_failed:
 	}
 
 	if (!(data & BTRFS_BLOCK_GROUP_DATA)) {
+		if (check_crossing_stripes(ins->objectid, num_bytes)) {
+			search_start = round_down(ins->objectid + num_bytes,
+						  BTRFS_STRIPE_LEN);
+			goto new_group;
+		}
 		block_group = btrfs_lookup_block_group(info, ins->objectid);
 		if (block_group)
 			trans->block_group = block_group;
-- 
2.4.6

[PATCH 4/4] btrfs: convert: Avoid allocating metadata extent crossing stripe boundary

[Qu Wenruo]

As convert implement its own alloc extent, avoid such metadata problem
too.

Reported-by: Chris Murphy <lists@colorremedies.com>
Reported-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-convert.c | 15 ++++++++++++---
 ctree.h         |  2 +-
 extent-tree.c   |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index b89c685..c97068e 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -207,7 +207,8 @@ static int cache_free_extents(struct btrfs_root *root, ext2_filsys ext2_fs)
 }
 
 static int custom_alloc_extent(struct btrfs_root *root, u64 num_bytes,
-			       u64 hint_byte, struct btrfs_key *ins)
+			       u64 hint_byte, struct btrfs_key *ins,
+			       int metadata)
 {
 	u64 start;
 	u64 end;
@@ -246,6 +247,14 @@ static int custom_alloc_extent(struct btrfs_root *root, u64 num_bytes,
 			continue;
 		}
 
+		if (metadata) {
+			BUG_ON(num_bytes != root->nodesize);
+			if (check_crossing_stripes(start, num_bytes)) {
+				last = round_down(start + num_bytes,
+						  BTRFS_STRIPE_LEN);
+				continue;
+			}
+		}
 		clear_extent_dirty(&root->fs_info->free_space_cache,
 				   start, start + num_bytes - 1, 0);
 
@@ -1280,7 +1289,7 @@ static int create_ext2_image(struct btrfs_root *root, ext2_filsys ext2_fs,
 	 * special, we can't rely on relocate_extents_range to relocate it.
 	 */
 	for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
-		ret = custom_alloc_extent(root, sectorsize, 0, &key);
+		ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
 		if (ret)
 			goto fail;
 		ret = copy_disk_extent(root, key.objectid, last_byte,
@@ -1938,7 +1947,7 @@ static int relocate_one_reference(struct btrfs_trans_handle *trans,
 			ret = get_state_private(reloc_tree, bytenr, &new_pos);
 			BUG_ON(ret);
 		} else {
-			ret = custom_alloc_extent(root, sectorsize, 0, &key);
+			ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
 			if (ret)
 				goto fail;
 			new_pos = key.objectid;
diff --git a/ctree.h b/ctree.h
index 227a00b..bcad2b9 100644
--- a/ctree.h
+++ b/ctree.h
@@ -946,7 +946,7 @@ struct btrfs_block_group_cache {
 
 struct btrfs_extent_ops {
        int (*alloc_extent)(struct btrfs_root *root, u64 num_bytes,
-		           u64 hint_byte, struct btrfs_key *ins);
+			   u64 hint_byte, struct btrfs_key *ins, int metadata);
        int (*free_extent)(struct btrfs_root *root, u64 bytenr,
 		          u64 num_bytes);
 };
diff --git a/extent-tree.c b/extent-tree.c
index 6f07e4b..0c8152a 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2654,7 +2654,7 @@ int btrfs_reserve_extent(struct btrfs_trans_handle *trans,
 
 	if (info->extent_ops) {
 		struct btrfs_extent_ops *ops = info->extent_ops;
-		ret = ops->alloc_extent(root, num_bytes, hint_byte, ins);
+		ret = ops->alloc_extent(root, num_bytes, hint_byte, ins, !data);
 		BUG_ON(ret);
 		goto found;
 	}
-- 
2.4.6

Re: [PATCH 4/4] btrfs: convert: Avoid allocating metadata extent crossing stripe boundary

[David Sterba]

On Thu, Jul 23, 2015 at 05:18:10PM +0800, Qu Wenruo wrote:
> @@ -246,6 +247,14 @@ static int custom_alloc_extent(struct btrfs_root *root, u64 num_bytes,
>  			continue;
>  		}
>  
> +		if (metadata) {
> +			BUG_ON(num_bytes != root->nodesize);

This caught my attention and looking at possible values of num_bytes,
this can crash:

1291         for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
1292                 ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);

where sectorsize == num_bytes.

> +			if (check_crossing_stripes(start, num_bytes)) {
> +				last = round_down(start + num_bytes,
> +						  BTRFS_STRIPE_LEN);
> +				continue;
> +			}
> +		}
>  		clear_extent_dirty(&root->fs_info->free_space_cache,
>  				   start, start + num_bytes - 1, 0);
>  
> @@ -1280,7 +1289,7 @@ static int create_ext2_image(struct btrfs_root *root, ext2_filsys ext2_fs,
>  	 * special, we can't rely on relocate_extents_range to relocate it.
>  	 */
>  	for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
> -		ret = custom_alloc_extent(root, sectorsize, 0, &key);
> +		ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);

Same here.

> -			ret = custom_alloc_extent(root, sectorsize, 0, &key);
> +			ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);

And here.

I hope there's a way how to avoid the BUG_ON at all.

Re: [PATCH 0/4] Metadata crossing stripe boundary fixes

[David Sterba]

On Thu, Jul 23, 2015 at 05:18:06PM +0800, Qu Wenruo wrote:
> Qu Wenruo (4):
>   btrfs: print-tree: print stripe len of a chunk
>   btrfs: fsck: Check if a metadata tree block crossing stripe boundary
>   btrfs: extent-tree: Avoid allocating tree block that cross stripe    
>     boundary

Applied the above, thanks.

>   btrfs: convert: Avoid allocating metadata extent crossing stripe    
>     boundary

The BUG_ON there does not look right, patch not applied.

Re: [PATCH 4/4] btrfs: convert: Avoid allocating metadata extent crossing stripe boundary

[Qu Wenruo]

David Sterba wrote on 2015/07/24 14:34 +0200:
> On Thu, Jul 23, 2015 at 05:18:10PM +0800, Qu Wenruo wrote:
>> @@ -246,6 +247,14 @@ static int custom_alloc_extent(struct btrfs_root *root, u64 num_bytes,
>>   			continue;
>>   		}
>>
>> +		if (metadata) {
>> +			BUG_ON(num_bytes != root->nodesize);
>
> This caught my attention and looking at possible values of num_bytes,
> this can crash:
>
> 1291         for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
> 1292                 ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
>
> where sectorsize == num_bytes.
For that case, the last 0 means that's a data block,
and won't comes to the BUG_ON, as it is only designed for metadata.
And for metadata allocation, the size will only be nodes/leafsize

Thanks,
Qu
>
>> +			if (check_crossing_stripes(start, num_bytes)) {
>> +				last = round_down(start + num_bytes,
>> +						  BTRFS_STRIPE_LEN);
>> +				continue;
>> +			}
>> +		}
>>   		clear_extent_dirty(&root->fs_info->free_space_cache,
>>   				   start, start + num_bytes - 1, 0);
>>
>> @@ -1280,7 +1289,7 @@ static int create_ext2_image(struct btrfs_root *root, ext2_filsys ext2_fs,
>>   	 * special, we can't rely on relocate_extents_range to relocate it.
>>   	 */
>>   	for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
>> -		ret = custom_alloc_extent(root, sectorsize, 0, &key);
>> +		ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
>
> Same here.
>
>> -			ret = custom_alloc_extent(root, sectorsize, 0, &key);
>> +			ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
>
> And here.
>
> I hope there's a way how to avoid the BUG_ON at all.
>

Re: [PATCH 4/4] btrfs: convert: Avoid allocating metadata extent crossing stripe boundary

[David Sterba]

On Sat, Jul 25, 2015 at 09:18:54AM +0800, Qu Wenruo wrote:
> > This caught my attention and looking at possible values of num_bytes,
> > this can crash:
> >
> > 1291         for (last_byte = 0; last_byte < first_free; last_byte += sectorsize) {
> > 1292                 ret = custom_alloc_extent(root, sectorsize, 0, &key, 0);
> >
> > where sectorsize == num_bytes.
> For that case, the last 0 means that's a data block,
> and won't comes to the BUG_ON, as it is only designed for metadata.
> And for metadata allocation, the size will only be nodes/leafsize

Right, so it's an asseert in disguise. Applied, thanks.