diff mbox

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

Message ID 1437643090-13920-5-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive)
State Accepted
Headers show

Commit Message

Qu Wenruo July 23, 2015, 9:18 a.m. UTC 
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(-)

Comments

David Sterba July 24, 2015, 12:34 p.m. UTC | #1 
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.
--
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
Qu Wenruo July 25, 2015, 1:18 a.m. UTC | #2 
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.
>
--
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
David Sterba July 27, 2015, 1:40 p.m. UTC | #3 
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.
--
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
diff mbox

Patch

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;
 	}