Chunk allocator DUP fix and cleanups

PATCH In review Hans van Kranenburg 4 participants 26 messages Oct 4, 2018 – Nov 13, 2018 Btrfs file system

Share & tools

Hans van Kranenburg

author

2018-10-04 21:31 UTC #0 lore raw

cover Chunk allocator DUP fix and cleanups

This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:

https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374

The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.

Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.

When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.

I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.

Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.

The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.

Moo!,
Knorrie

Hans van Kranenburg (6):
btrfs: alloc_chunk: do not refurbish num_bytes
btrfs: alloc_chunk: improve chunk size variable name
btrfs: alloc_chunk: fix more DUP stripe size handling
btrfs: fix ncopies raid_attr for RAID56
btrfs: introduce nparity raid_attr
btrfs: alloc_chunk: rework chunk/stripe calculations

 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  4 ++-
 2 files changed, 45 insertions(+), 43 deletions(-)

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Hans van Kranenburg

author

2018-10-04 21:31 UTC #1 lore raw

PATCH 1/6 btrfs: alloc_chunk: do not refurbish num_bytes

num_bytes is used to store the chunk length of the chunk that we're allocating. Do not reuse it for something really different in the same function.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +3 -2

index f4405e430da6..9c9bb127eeee 100644
@@ -4837,8 +4837,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		goto error_del_extent;
 
 	for (i = 0; i < map->num_stripes; i++) {
-		num_bytes = map->stripes[i].dev->bytes_used + stripe_size;
-		btrfs_device_set_bytes_used(map->stripes[i].dev, num_bytes);
+		btrfs_device_set_bytes_used(map->stripes[i].dev,
+					    map->stripes[i].dev->bytes_used +
+					    stripe_size);
 	}
 
 	atomic64_sub(stripe_size * map->num_stripes, &info->free_chunk_space);

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Nikolay Borisov

Reviewed-by on v1 1/6

2018-10-05 08:59 UTC #2 lore raw

Re: [PATCH 1/6] btrfs: alloc_chunk: do not refurbish num_bytes

Quoted 5 lines · Hans van Kranenburg

num_bytes is used to store the chunk length of the chunk that we're allocating. Do not reuse it for something really different in the same function.
Signed-off-by Hans van Kranenburg <[email protected]>

nit: That's a minor cleanup and brings no functional changes. I think it even allows to give a more descriptive name of num_bytes such as chunk_size, especially since we have a max_chunk_size. I think they would pair nicely.

Anyway,

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 21 lines

---
 fs/btrfs/volumes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +3 -2

index f4405e430da6..9c9bb127eeee 100644
@@ -4837,8 +4837,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		goto error_del_extent;
 
 	for (i = 0; i < map->num_stripes; i++) {
-		num_bytes = map->stripes[i].dev->bytes_used + stripe_size;
-		btrfs_device_set_bytes_used(map->stripes[i].dev, num_bytes);
+		btrfs_device_set_bytes_used(map->stripes[i].dev,
+					    map->stripes[i].dev->bytes_used +
+					    stripe_size);
 	}
 
 	atomic64_sub(stripe_size * map->num_stripes, &info->free_chunk_space);

Nikolay Borisov

2018-10-05 09:00 UTC #3 lore raw

Re: [PATCH 1/6] btrfs: alloc_chunk: do not refurbish num_bytes

Quoted 7 lines · Nikolay Borisov

Quoted 4 lines · Hans van Kranenburg
num_bytes is used to store the chunk length of the chunk that we're allocating. Do not reuse it for something really different in the same function.

Quoted 6 lines

Quoted 1 line
Signed-off-by Hans van Kranenburg <[email protected]>
nit: That's a minor cleanup and brings no functional changes. I think it even allows to give a more descriptive name of num_bytes such as chunk_size, especially since we have a max_chunk_size. I think they would pair nicely.

I saw that your patch 2 actually does that, so ignore this comment :)

Quoted 26 lines

Anyway,

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 21 lines

---
 fs/btrfs/volumes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +3 -2

index f4405e430da6..9c9bb127eeee 100644
@@ -4837,8 +4837,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		goto error_del_extent;
 
 	for (i = 0; i < map->num_stripes; i++) {
-		num_bytes = map->stripes[i].dev->bytes_used + stripe_size;
-		btrfs_device_set_bytes_used(map->stripes[i].dev, num_bytes);
+		btrfs_device_set_bytes_used(map->stripes[i].dev,
+					    map->stripes[i].dev->bytes_used +
+					    stripe_size);
 	}
 
 	atomic64_sub(stripe_size * map->num_stripes, &info->free_chunk_space);

Hans van Kranenburg

author

2018-10-04 21:31 UTC #4 lore raw

PATCH 2/6 btrfs: alloc_chunk: improve chunk size variable name

In reply to Hans van Kranenburg

num_bytes is really a way too generic name for a variable in this function. There are a dozen other variables that hold a number of bytes as value.

Give it a name that actually describes what it does, which is holding the size of the chunk that we're allocating.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

fs/btrfs/volumes.c +7 -5

index 9c9bb127eeee..40fa85e68b1f 100644
@@ -123,6 +123,8 @@ const char *get_raid_name(enum btrfs_raid_types type)
 	return btrfs_raid_array[type].raid_name;
 }
 
+
+
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_fs_info *fs_info);
 static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info);

@@ -4599,7 +4601,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	u64 max_stripe_size;
 	u64 max_chunk_size;
 	u64 stripe_size;
-	u64 num_bytes;
+	u64 chunk_size;
 	int ndevs;
 	int i;
 	int j;

@@ -4801,9 +4803,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	map->type = type;
 	map->sub_stripes = sub_stripes;
 
-	num_bytes = stripe_size * data_stripes;
+	chunk_size = stripe_size * data_stripes;
 
-	trace_btrfs_chunk_alloc(info, map, start, num_bytes);
+	trace_btrfs_chunk_alloc(info, map, start, chunk_size);
 
 	em = alloc_extent_map();
 	if (!em) {

@@ -4814,7 +4816,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags);
 	em->map_lookup = map;
 	em->start = start;
-	em->len = num_bytes;
+	em->len = chunk_size;
 	em->block_start = 0;
 	em->block_len = em->len;
 	em->orig_block_len = stripe_size;

@@ -4832,7 +4834,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	refcount_inc(&em->refs);
 	write_unlock(&em_tree->lock);
 
-	ret = btrfs_make_block_group(trans, 0, type, start, num_bytes);
+	ret = btrfs_make_block_group(trans, 0, type, start, chunk_size);
 	if (ret)
 		goto error_del_extent;

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Hans van Kranenburg

author

2018-10-04 21:36 UTC #5 lore raw

Re: [PATCH 2/6] btrfs: alloc_chunk: improve chunk size variable name

Quoted 17 lines · Hans van Kranenburg

num_bytes is really a way too generic name for a variable in this function. There are a dozen other variables that hold a number of bytes as value.
Give it a name that actually describes what it does, which is holding the size of the chunk that we're allocating.
Signed-off-by Hans van Kranenburg <[email protected]>
---
 fs/btrfs/volumes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
fs/btrfs/volumes.c +0 -0
index 9c9bb127eeee..40fa85e68b1f 100644

fs/btrfs/volumes.c const char *get_raid_name(enum btrfs_raid_types type) · line 127

Quoted 5 lines

fs/btrfs/volumes.c +2 -0

@@ -123,6 +123,8 @@ const char *get_raid_name(enum btrfs_raid_types type)
 	return btrfs_raid_array[type].raid_name;
 }
 
+
+

Meh, missed that one, will remove.

Quoted 43 lines

fs/btrfs/volumes.c +5 -5

 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_fs_info *fs_info);
 static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info);

@@ -4599,7 +4601,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	u64 max_stripe_size;
 	u64 max_chunk_size;
 	u64 stripe_size;
-	u64 num_bytes;
+	u64 chunk_size;
 	int ndevs;
 	int i;
 	int j;

@@ -4801,9 +4803,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	map->type = type;
 	map->sub_stripes = sub_stripes;
 
-	num_bytes = stripe_size * data_stripes;
+	chunk_size = stripe_size * data_stripes;
 
-	trace_btrfs_chunk_alloc(info, map, start, num_bytes);
+	trace_btrfs_chunk_alloc(info, map, start, chunk_size);
 
 	em = alloc_extent_map();
 	if (!em) {

@@ -4814,7 +4816,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags);
 	em->map_lookup = map;
 	em->start = start;
-	em->len = num_bytes;
+	em->len = chunk_size;
 	em->block_start = 0;
 	em->block_len = em->len;
 	em->orig_block_len = stripe_size;

@@ -4832,7 +4834,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	refcount_inc(&em->refs);
 	write_unlock(&em_tree->lock);
 
-	ret = btrfs_make_block_group(trans, 0, type, start, num_bytes);
+	ret = btrfs_make_block_group(trans, 0, type, start, chunk_size);
 	if (ret)
 		goto error_del_extent;

Signature · 1 line

Hans van Kranenburg

Nikolay Borisov

Reviewed-by on v1 2/6

2018-10-05 09:01 UTC #6 lore raw

Re: [PATCH 2/6] btrfs: alloc_chunk: improve chunk size variable name

In reply to Hans van Kranenburg

Quoted 8 lines · Hans van Kranenburg

num_bytes is really a way too generic name for a variable in this function. There are a dozen other variables that hold a number of bytes as value.
Give it a name that actually describes what it does, which is holding the size of the chunk that we're allocating.
Signed-off-by Hans van Kranenburg <[email protected]>

Apart from the extra 2 spaces you can add:

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 57 lines

---
 fs/btrfs/volumes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

fs/btrfs/volumes.c +7 -5

index 9c9bb127eeee..40fa85e68b1f 100644
@@ -123,6 +123,8 @@ const char *get_raid_name(enum btrfs_raid_types type)
 	return btrfs_raid_array[type].raid_name;
 }
 
+
+
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_fs_info *fs_info);
 static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info);

@@ -4599,7 +4601,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	u64 max_stripe_size;
 	u64 max_chunk_size;
 	u64 stripe_size;
-	u64 num_bytes;
+	u64 chunk_size;
 	int ndevs;
 	int i;
 	int j;

@@ -4801,9 +4803,9 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	map->type = type;
 	map->sub_stripes = sub_stripes;
 
-	num_bytes = stripe_size * data_stripes;
+	chunk_size = stripe_size * data_stripes;
 
-	trace_btrfs_chunk_alloc(info, map, start, num_bytes);
+	trace_btrfs_chunk_alloc(info, map, start, chunk_size);
 
 	em = alloc_extent_map();
 	if (!em) {

@@ -4814,7 +4816,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags);
 	em->map_lookup = map;
 	em->start = start;
-	em->len = num_bytes;
+	em->len = chunk_size;
 	em->block_start = 0;
 	em->block_len = em->len;
 	em->orig_block_len = stripe_size;

@@ -4832,7 +4834,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	refcount_inc(&em->refs);
 	write_unlock(&em_tree->lock);
 
-	ret = btrfs_make_block_group(trans, 0, type, start, num_bytes);
+	ret = btrfs_make_block_group(trans, 0, type, start, chunk_size);
 	if (ret)
 		goto error_del_extent;

Hans van Kranenburg

author

2018-10-04 21:30 UTC #7 lore raw

PATCH 3/6 btrfs: alloc_chunk: fix more DUP stripe size handling

In reply to Hans van Kranenburg

Commit 92e222df7b "btrfs: alloc_chunk: fix DUP stripe size handling" fixed calculating the stripe_size for a new DUP chunk.

However, the same calculation reappears a bit later, and that one was not changed yet. The resulting bug that is exposed is that the newly allocated device extents ('stripes') can have a few MiB overlap with the next thing stored after them, which is another device extent or the end of the disk.

The scenario in which this can happen is:
* The block device for the filesystem is less than 10GiB in size.
* The amount of contiguous free unallocated disk space chosen to use for chunk allocation is 20% of the total device size, or a few MiB more or less.

An example:
- The filesystem device is 7880MiB (max_chunk_size gets set to 788MiB)
- There's 1578MiB unallocated raw disk space left in one contiguous piece.

In this case stripe_size is first calculated as 789MiB, (half of 1578MiB).

Since 789MiB (stripe_size * data_stripes) > 788MiB (max_chunk_size), we enter the if block. Now stripe_size value is immediately overwritten while calculating an adjusted value based on max_chunk_size, which ends up as 788MiB.

Next, the value is rounded up to a 16MiB boundary, 800MiB, which is actually more than the value we had before. However, the last comparison fails to detect this, because it's comparing the value with the total amount of free space, which is about twice the size of stripe_size.

In the example above, this means that the resulting raw disk space being allocated is 1600MiB, while only a gap of 1578MiB has been found. The second device extent object for this DUP chunk will overlap for 22MiB with whatever comes next.

The underlying problem here is that the stripe_size is reused all the time for different things. So, when entering the code in the if block, stripe_size is immediately overwritten with something else. If later we decide we want to have the previous value back, then the logic to compute it was copy pasted in again.

With this change, the value in stripe_size is not unnecessarily destroyed, so the duplicated calculation is not needed any more.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

fs/btrfs/volumes.c +7 ~~-10~~

index 40fa85e68b1f..7045814fc98d 100644
@@ -4763,19 +4763,16 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	/*
 	 * Use the number of data stripes to figure out how big this chunk
 	 * is really going to be in terms of logical address space,
-	 * and compare that answer with the max chunk size
+	 * and compare that answer with the max chunk size. If it's higher,
+	 * we try to reduce stripe_size.
 	 */
 	if (stripe_size * data_stripes > max_chunk_size) {
-		stripe_size = div_u64(max_chunk_size, data_stripes);
-
-		/* bump the answer up to a 16MB boundary */
-		stripe_size = round_up(stripe_size, SZ_16M);
-
-		/*
-		 * But don't go higher than the limits we found while searching
-		 * for free extents
+		/* Reduce stripe_size, round it up to a 16MB boundary
+		 * again and then use it, unless it ends up being even
+		 * bigger than the previous value we had already.
 		 */
-		stripe_size = min(devices_info[ndevs - 1].max_avail,
+		stripe_size = min(round_up(div_u64(max_chunk_size,
+						   data_stripes), SZ_16M),
 				  stripe_size);
 	}

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Hans van Kranenburg

author

2018-10-04 21:30 UTC #8 lore raw

PATCH 4/6 btrfs: fix ncopies raid_attr for RAID56

In reply to Hans van Kranenburg

RAID5 and RAID6 profile store one copy of the data, not 2 or 3. These values are not used anywhere by the way.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +2 -2

index 7045814fc98d..d82b3d735ebe 100644
@@ -96,7 +96,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.devs_min	= 2,
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
-		.ncopies	= 2,
+		.ncopies	= 1,
 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -108,7 +108,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.devs_min	= 3,
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
-		.ncopies	= 3,
+		.ncopies	= 1,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Nikolay Borisov

Reviewed-by on v1 4/6

2018-10-05 09:10 UTC #9 lore raw

Re: [PATCH 4/6] btrfs: fix ncopies raid_attr for RAID56

Quoted 4 lines · Hans van Kranenburg

RAID5 and RAID6 profile store one copy of the data, not 2 or 3. These values are not used anywhere by the way.
Signed-off-by Hans van Kranenburg <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 27 lines

---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +2 -2

index 7045814fc98d..d82b3d735ebe 100644
@@ -96,7 +96,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.devs_min	= 2,
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
-		.ncopies	= 2,
+		.ncopies	= 1,
 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -108,7 +108,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.devs_min	= 3,
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
-		.ncopies	= 3,
+		.ncopies	= 1,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

Hans van Kranenburg

author

2018-10-04 21:31 UTC #10 lore raw

PATCH 5/6 btrfs: introduce nparity raid_attr

In reply to Hans van Kranenburg

Instead of hardcoding exceptions for RAID5 and RAID6 in the code, use an nparity field in raid_attr.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 18 +++++++++++-------
 fs/btrfs/volumes.h |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

fs/btrfs/volumes.c +11 -7

index d82b3d735ebe..453046497ac8 100644
@@ -37,6 +37,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid10",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID10,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,

@@ -49,6 +50,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid1",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,

@@ -61,6 +63,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "dup",
 		.bg_flag	= BTRFS_BLOCK_GROUP_DUP,
 		.mindev_error	= 0,

@@ -73,6 +76,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "raid0",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID0,
 		.mindev_error	= 0,

@@ -85,6 +89,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "single",
 		.bg_flag	= 0,
 		.mindev_error	= 0,

@@ -97,6 +102,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -109,6 +115,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

@@ -4597,6 +4604,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	int devs_min;		/* min devs needed */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int ret;
 	u64 max_stripe_size;
 	u64 max_chunk_size;

@@ -4623,6 +4632,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	devs_min = btrfs_raid_array[index].devs_min;
 	devs_increment = btrfs_raid_array[index].devs_increment;
 	ncopies = btrfs_raid_array[index].ncopies;
+	nparity = btrfs_raid_array[index].nparity;
 
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
 		max_stripe_size = SZ_1G;

@@ -4752,13 +4762,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * this will have to be fixed for RAID1 and RAID10 over
 	 * more drives
 	 */
-	data_stripes = num_stripes / ncopies;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID5)
-		data_stripes = num_stripes - 1;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID6)
-		data_stripes = num_stripes - 2;
+	data_stripes = (num_stripes - nparity) / ncopies;
 
 	/*
 	 * Use the number of data stripes to figure out how big this chunk

fs/btrfs/volumes.h +2 -0

index 23e9285d88de..0fe005b4295a 100644
@@ -331,6 +331,8 @@ struct btrfs_raid_attr {
 	int tolerated_failures; /* max tolerated fail devs */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int mindev_error;	/* error code if min devs requisite is unmet */
 	const char raid_name[8]; /* name of the raid */
 	u64 bg_flag;		/* block group flag of the raid */

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Hans van Kranenburg

author

2018-10-04 22:21 UTC #11 lore raw

Re: [PATCH 5/6] btrfs: introduce nparity raid_attr

Quoted 54 lines · Hans van Kranenburg

Instead of hardcoding exceptions for RAID5 and RAID6 in the code, use an nparity field in raid_attr.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 18 +++++++++++-------
 fs/btrfs/volumes.h |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

fs/btrfs/volumes.c +5 -0

index d82b3d735ebe..453046497ac8 100644
@@ -37,6 +37,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid10",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID10,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,

@@ -49,6 +50,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid1",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,

@@ -61,6 +63,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "dup",
 		.bg_flag	= BTRFS_BLOCK_GROUP_DUP,
 		.mindev_error	= 0,

@@ -73,6 +76,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "raid0",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID0,
 		.mindev_error	= 0,

@@ -85,6 +89,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "single",
 		.bg_flag	= 0,
 		.mindev_error	= 0,

fs/btrfs/volumes.c const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = { · line 105

Quoted 4 lines

fs/btrfs/volumes.c +1 -0

@@ -97,6 +102,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,

Ahem, this obviously needs to be 1. Another reorganize/rebase-fail.

Quoted 57 lines

fs/btrfs/volumes.c +5 -7

 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -109,6 +115,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

@@ -4597,6 +4604,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	int devs_min;		/* min devs needed */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int ret;
 	u64 max_stripe_size;
 	u64 max_chunk_size;

@@ -4623,6 +4632,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	devs_min = btrfs_raid_array[index].devs_min;
 	devs_increment = btrfs_raid_array[index].devs_increment;
 	ncopies = btrfs_raid_array[index].ncopies;
+	nparity = btrfs_raid_array[index].nparity;
 
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
 		max_stripe_size = SZ_1G;

@@ -4752,13 +4762,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * this will have to be fixed for RAID1 and RAID10 over
 	 * more drives
 	 */
-	data_stripes = num_stripes / ncopies;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID5)
-		data_stripes = num_stripes - 1;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID6)
-		data_stripes = num_stripes - 2;
+	data_stripes = (num_stripes - nparity) / ncopies;
 
 	/*
 	 * Use the number of data stripes to figure out how big this chunk

fs/btrfs/volumes.h +2 -0

index 23e9285d88de..0fe005b4295a 100644
@@ -331,6 +331,8 @@ struct btrfs_raid_attr {
 	int tolerated_failures; /* max tolerated fail devs */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int mindev_error;	/* error code if min devs requisite is unmet */
 	const char raid_name[8]; /* name of the raid */
 	u64 bg_flag;		/* block group flag of the raid */

Signature · 1 line

Hans van Kranenburg

Nikolay Borisov

2018-10-05 14:42 UTC #12 lore raw

Re: [PATCH 5/6] btrfs: introduce nparity raid_attr

In reply to Hans van Kranenburg

Quoted 106 lines · Hans van Kranenburg

Instead of hardcoding exceptions for RAID5 and RAID6 in the code, use an nparity field in raid_attr.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 18 +++++++++++-------
 fs/btrfs/volumes.h |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

fs/btrfs/volumes.c +11 -7

index d82b3d735ebe..453046497ac8 100644
@@ -37,6 +37,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid10",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID10,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,

@@ -49,6 +50,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid1",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,

@@ -61,6 +63,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "dup",
 		.bg_flag	= BTRFS_BLOCK_GROUP_DUP,
 		.mindev_error	= 0,

@@ -73,6 +76,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "raid0",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID0,
 		.mindev_error	= 0,

@@ -85,6 +89,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "single",
 		.bg_flag	= 0,
 		.mindev_error	= 0,

@@ -97,6 +102,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -109,6 +115,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

@@ -4597,6 +4604,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	int devs_min;		/* min devs needed */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int ret;
 	u64 max_stripe_size;
 	u64 max_chunk_size;

@@ -4623,6 +4632,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	devs_min = btrfs_raid_array[index].devs_min;
 	devs_increment = btrfs_raid_array[index].devs_increment;
 	ncopies = btrfs_raid_array[index].ncopies;
+	nparity = btrfs_raid_array[index].nparity;
 
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
 		max_stripe_size = SZ_1G;

@@ -4752,13 +4762,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * this will have to be fixed for RAID1 and RAID10 over
 	 * more drives
 	 */
-	data_stripes = num_stripes / ncopies;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID5)
-		data_stripes = num_stripes - 1;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID6)
-		data_stripes = num_stripes - 2;
+	data_stripes = (num_stripes - nparity) / ncopies;
 
 	/*
 	 * Use the number of data stripes to figure out how big this chunk

fs/btrfs/volumes.h +0 -0

index 23e9285d88de..0fe005b4295a 100644

fs/btrfs/volumes.h struct btrfs_raid_attr { · line 335

Quoted 5 lines

fs/btrfs/volumes.h +2 -0

@@ -331,6 +331,8 @@ struct btrfs_raid_attr {
 	int tolerated_failures; /* max tolerated fail devs */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */

The description can be simplified to: number of parity stripes

Quoted 4 lines

fs/btrfs/volumes.h +0 -0

 	int mindev_error;	/* error code if min devs requisite is unmet */
 	const char raid_name[8]; /* name of the raid */
 	u64 bg_flag;		/* block group flag of the raid */

Hans van Kranenburg

author

2018-10-05 22:45 UTC #13 lore raw

Re: [PATCH 5/6] btrfs: introduce nparity raid_attr

Quoted 102 lines · Nikolay Borisov

Quoted 99 lines · Hans van Kranenburg

Instead of hardcoding exceptions for RAID5 and RAID6 in the code, use an nparity field in raid_attr.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 18 +++++++++++-------
 fs/btrfs/volumes.h |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

fs/btrfs/volumes.c +11 -7

index d82b3d735ebe..453046497ac8 100644
@@ -37,6 +37,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid10",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID10,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,

@@ -49,6 +50,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 2,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "raid1",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,

@@ -61,6 +63,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 2,
+		.nparity        = 0,
 		.raid_name	= "dup",
 		.bg_flag	= BTRFS_BLOCK_GROUP_DUP,
 		.mindev_error	= 0,

@@ -73,6 +76,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "raid0",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID0,
 		.mindev_error	= 0,

@@ -85,6 +89,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 0,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 0,
 		.raid_name	= "single",
 		.bg_flag	= 0,
 		.mindev_error	= 0,

@@ -97,6 +102,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 1,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid5",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID5,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,

@@ -109,6 +115,7 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 2,
 		.devs_increment	= 1,
 		.ncopies	= 1,
+		.nparity        = 2,
 		.raid_name	= "raid6",
 		.bg_flag	= BTRFS_BLOCK_GROUP_RAID6,
 		.mindev_error	= BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,

@@ -4597,6 +4604,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	int devs_min;		/* min devs needed */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */
 	int ret;
 	u64 max_stripe_size;
 	u64 max_chunk_size;

@@ -4623,6 +4632,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	devs_min = btrfs_raid_array[index].devs_min;
 	devs_increment = btrfs_raid_array[index].devs_increment;
 	ncopies = btrfs_raid_array[index].ncopies;
+	nparity = btrfs_raid_array[index].nparity;
 
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
 		max_stripe_size = SZ_1G;

@@ -4752,13 +4762,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * this will have to be fixed for RAID1 and RAID10 over
 	 * more drives
 	 */
-	data_stripes = num_stripes / ncopies;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID5)
-		data_stripes = num_stripes - 1;
-
-	if (type & BTRFS_BLOCK_GROUP_RAID6)
-		data_stripes = num_stripes - 2;
+	data_stripes = (num_stripes - nparity) / ncopies;

Quoted 14 lines

fs/btrfs/volumes.h struct btrfs_raid_attr { · line 335

Quoted 12 lines

fs/btrfs/volumes.c +0 -0

 	/*
 	 * Use the number of data stripes to figure out how big this chunk

fs/btrfs/volumes.h +2 -0

index 23e9285d88de..0fe005b4295a 100644
@@ -331,6 +331,8 @@ struct btrfs_raid_attr {
 	int tolerated_failures; /* max tolerated fail devs */
 	int devs_increment;	/* ndevs has to be a multiple of this */
 	int ncopies;		/* how many copies to data has */
+	int nparity;		/* number of stripes worth of bytes to
+				   store parity information */

The description can be simplified to: number of parity stripes

Well, if 'stripe' is a synonym for 'device extent' here, then technically that's incorrect, since all profiles using parity do distribute the parity data over all stripes. IOW, it's not RAID4.

For the calculations in here we treat it like that, because on this level we don't care about what goes where later, just that the total amount of allocated bytes is right.

That's why I thought putting a little hint in there, to make the reader think about it and then realize that it's actually not that simple. And apparently that works. :)

Quoted 5 lines

fs/btrfs/volumes.h +0 -0

 	int mindev_error;	/* error code if min devs requisite is unmet */
 	const char raid_name[8]; /* name of the raid */
 	u64 bg_flag;		/* block group flag of the raid */

Signature · 1 line

Hans van Kranenburg

Hans van Kranenburg

author

2018-10-04 21:30 UTC #14 lore raw

PATCH 6/6 btrfs: alloc_chunk: rework chunk/stripe calculations

In reply to Hans van Kranenburg

Previously, the stripe_size variable was modified too many times in the __btrfs_alloc_chunk function. The most problematic place was the if block dealing with a chunk bigger than max_chunk_size, which would throw away (overwrite) the value of stripe_size, maybe realizing a few lines later that the previous value was actually better and executing a copy of former logic to try get it back in the previous state.

Instead of on-the-fly calculating the target chunk size, adjust the max_stripe_size variable based on the max_chunk_size that was set before, and use that to simply compare it to stripe_size at some point. This removes the whole problematic if block.

Signed-off-by Hans van Kranenburg <[email protected]>

---
 fs/btrfs/volumes.c | 46 +++++++++++++++++++++-------------------------
 fs/btrfs/volumes.h |  2 +-
 2 files changed, 22 insertions(+), 26 deletions(-)

fs/btrfs/volumes.c +21 ~~-25~~

index 453046497ac8..862ee17ee0e5 100644
@@ -4596,14 +4596,15 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	struct btrfs_device_info *devices_info = NULL;
 	u64 total_avail;
 	int num_stripes;	/* total number of stripes to allocate */
-	int data_stripes;	/* number of stripes that count for
-				   block group size */
+	int num_data_stripes;	/* number of stripes worth of bytes to
+				   store data including copies */
 	int sub_stripes;	/* sub_stripes info for map */
 	int dev_stripes;	/* stripes per dev */
 	int devs_max;		/* max devs to use */
 	int devs_min;		/* min devs needed */
 	int devs_increment;	/* ndevs has to be a multiple of this */
-	int ncopies;		/* how many copies to data has */
+	int ncopies;		/* how many times actual data is duplicated
+				   inside num_data_stripes */
 	int nparity;		/* number of stripes worth of bytes to
 				   store parity information */
 	int ret;

@@ -4747,6 +4748,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	}
 
 	ndevs = min(ndevs, devs_max);
+	num_stripes = ndevs * dev_stripes;
+	num_data_stripes = num_stripes - nparity;
 
 	/*
 	 * The primary goal is to maximize the number of stripes, so use as

@@ -4756,31 +4759,24 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * max_avail is the total size so we have to adjust.
 	 */
 	stripe_size = div_u64(devices_info[ndevs - 1].max_avail, dev_stripes);
-	num_stripes = ndevs * dev_stripes;
-
-	/*
-	 * this will have to be fixed for RAID1 and RAID10 over
-	 * more drives
-	 */
-	data_stripes = (num_stripes - nparity) / ncopies;
 
 	/*
-	 * Use the number of data stripes to figure out how big this chunk
-	 * is really going to be in terms of logical address space,
-	 * and compare that answer with the max chunk size. If it's higher,
-	 * we try to reduce stripe_size.
+	 * Now that we know how many stripes we're going to use, we can adjust
+	 * down max_stripe_size if needed, paying attention to max_chunk_size.
+	 *
+	 * By multiplying chunk size with ncopies, we get the total amount of
+	 * bytes that need to fit into all the non-parity stripes.
+	 *
+	 * A chunk is allowed to end up being a bit bigger than max_chunk_size
+	 * when rounding up the stripe_size to a 16MiB boundary makes it so.
+	 * Unless... it ends up being bigger than the amount of physical free
+	 * space we can use for it.
 	 */
-	if (stripe_size * data_stripes > max_chunk_size) {
-		/* Reduce stripe_size, round it up to a 16MB boundary
-		 * again and then use it, unless it ends up being even
-		 * bigger than the previous value we had already.
-		 */
-		stripe_size = min(round_up(div_u64(max_chunk_size,
-						   data_stripes), SZ_16M),
-				  stripe_size);
-	}
+	max_stripe_size = min(round_up((max_chunk_size * ncopies) /
+				       num_data_stripes, SZ_16M),
+			      max_stripe_size);
 
-	/* align to BTRFS_STRIPE_LEN */
+	stripe_size = min(max_stripe_size, stripe_size);
 	stripe_size = round_down(stripe_size, BTRFS_STRIPE_LEN);
 
 	map = kmalloc(map_lookup_size(num_stripes), GFP_NOFS);

@@ -4804,7 +4800,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	map->type = type;
 	map->sub_stripes = sub_stripes;
 
-	chunk_size = stripe_size * data_stripes;
+	chunk_size = div_u64(stripe_size * num_data_stripes, ncopies);
 
 	trace_btrfs_chunk_alloc(info, map, start, chunk_size);

fs/btrfs/volumes.h +1 -1

index 0fe005b4295a..ee2ec77b1291 100644
@@ -330,7 +330,7 @@ struct btrfs_raid_attr {
 	int devs_min;		/* min devs needed */
 	int tolerated_failures; /* max tolerated fail devs */
 	int devs_increment;	/* ndevs has to be a multiple of this */
-	int ncopies;		/* how many copies to data has */
+	int ncopies;		/* how many copies the data has */
 	int nparity;		/* number of stripes worth of bytes to
 				   store parity information */
 	int mindev_error;	/* error code if min devs requisite is unmet */

Signature · 1 line

2.19.0.329.g76f2f5c1e3

Qu Wenruo

2018-10-05 07:51 UTC #15 lore raw

In reply to Hans van Kranenburg

Quoted 4 lines · Hans van Kranenburg

This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374

For that bug, did you succeeded in reproducing the bug?

I'm adding dev extent overlap checking in btrfs_verify_dev_extents() and btrfs-progs.
I think it could help to detect such problem.

Thanks,
Qu

Quoted 42 lines

The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Moo!,
Knorrie
Hans van Kranenburg (6):
btrfs: alloc_chunk: do not refurbish num_bytes
btrfs: alloc_chunk: improve chunk size variable name
btrfs: alloc_chunk: fix more DUP stripe size handling
btrfs: fix ncopies raid_attr for RAID56
btrfs: introduce nparity raid_attr
btrfs: alloc_chunk: rework chunk/stripe calculations
 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  4 ++-
 2 files changed, 45 insertions(+), 43 deletions(-)

Hans van Kranenburg

author

2018-10-05 10:58 UTC #16 lore raw

Quoted 9 lines · Qu Wenruo

Quoted 4 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
For that bug, did you succeeded in reproducing the bug?

Yes, open the above link and scroll to "Steps to reproduce".

Quoted 50 lines

I'm adding dev extent overlap checking in btrfs_verify_dev_extents() and btrfs-progs.
I think it could help to detect such problem.
Thanks,
Qu
Quoted 42 lines
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Moo!,
Knorrie
Hans van Kranenburg (6):
btrfs: alloc_chunk: do not refurbish num_bytes
btrfs: alloc_chunk: improve chunk size variable name
btrfs: alloc_chunk: fix more DUP stripe size handling
btrfs: fix ncopies raid_attr for RAID56
btrfs: introduce nparity raid_attr
btrfs: alloc_chunk: rework chunk/stripe calculations
 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  4 ++-
 2 files changed, 45 insertions(+), 43 deletions(-)

Signature · 1 line

Hans van Kranenburg

Qu Wenruo

2018-10-08 06:43 UTC #17 lore raw

Quoted 4 lines · Hans van Kranenburg

Quoted 3 lines · Qu Wenruo
On 2018/10/5 上午5:24, Hans van Kranenburg wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
For that bug, did you succeeded in reproducing the bug?
Yes, open the above link and scroll to "Steps to reproduce".

That's beyond device boundary one. Also reproduced here. And hand-crafted a super small image as test case for btrfs-progs.

But I'm a little curious about the dev extent overlapping case. Have you got one?

Thanks,
Qu

Quoted 55 lines

o/
Quoted 50 lines
I'm adding dev extent overlap checking in btrfs_verify_dev_extents() and btrfs-progs.
I think it could help to detect such problem.
Thanks,
Qu
Quoted 42 lines
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Moo!,
Knorrie
Hans van Kranenburg (6):
btrfs: alloc_chunk: do not refurbish num_bytes
btrfs: alloc_chunk: improve chunk size variable name
btrfs: alloc_chunk: fix more DUP stripe size handling
btrfs: fix ncopies raid_attr for RAID56
btrfs: introduce nparity raid_attr
btrfs: alloc_chunk: rework chunk/stripe calculations
 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  4 ++-
 2 files changed, 45 insertions(+), 43 deletions(-)

Hans van Kranenburg

author

2018-10-08 13:19 UTC #18 lore raw

Quoted 14 lines · Qu Wenruo

Quoted 11 lines · Hans van Kranenburg
Quoted 9 lines · Qu Wenruo
Quoted 4 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
For that bug, did you succeeded in reproducing the bug?

Quoted 7 lines

Quoted 1 line
Yes, open the above link and scroll to "Steps to reproduce".
That's beyond device boundary one. Also reproduced here. And hand-crafted a super small image as test case for btrfs-progs.
But I'm a little curious about the dev extent overlapping case. Have you got one?

Ah ok, I see. No, I didn't do that yet.

By using unmodified tooling, I think this can be done by a combination of a few resizings and using very specific balance to cause a fs of exactly 7880MiB again with a single 1578MiB gap inside...

I'll try later today to see if I can come up with a recipe for this.

Signature · 1 line

Hans van Kranenburg

Hans van Kranenburg

author

2018-10-08 23:51 UTC #19 lore raw

Quoted 23 lines · Hans van Kranenburg

Quoted 14 lines · Qu Wenruo
Quoted 11 lines · Hans van Kranenburg
Quoted 9 lines · Qu Wenruo
Quoted 4 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
For that bug, did you succeeded in reproducing the bug?
Quoted 7 lines
Quoted 1 line
Yes, open the above link and scroll to "Steps to reproduce".
That's beyond device boundary one. Also reproduced here. And hand-crafted a super small image as test case for btrfs-progs.
But I'm a little curious about the dev extent overlapping case. Have you got one?

Quoted 7 lines

Ah ok, I see. No, I didn't do that yet.
By using unmodified tooling, I think this can be done by a combination of a few resizings and using very specific balance to cause a fs of exactly 7880MiB again with a single 1578MiB gap inside...
I'll try later today to see if I can come up with a recipe for this.

Ok, this is actually pretty simple to do:

---- >8 ----

-# mkdir bork
-# cd bork
-# dd if=/dev/zero of=image bs=1 count=0 seek=1024M 0+0 records in
0+0 records out
0 bytes copied, 0.000183343 s, 0.0 kB/s
-# mkfs.btrfs -d dup -m dup image
-# losetup -f image
-# mount -o space_cache=v2 /dev/loop0 mountpoint
-# dd if=/dev/zero of=mountpoint/flapsie bs=1M
dd: error writing 'mountpoint/flapsie': No space left on device 453+0 records in
452+0 records out
474185728 bytes (474 MB, 452 MiB) copied, 4.07663 s, 116 MB/s

---- >8 ----

-# ./show_usage.py /bork/mountpoint/
Target profile for SYSTEM (chunk tree): DUP
Target profile for METADATA: DUP
Target profile for DATA: DUP
Mixed groups: False

Virtual space usage by block group type:

|
| type                    total         used
| ----                    -----         ----
| Data                452.31MiB    452.22MiB
| System                8.00MiB     16.00KiB
| Metadata             51.19MiB    656.00KiB

Total raw filesystem size: 1.00GiB
Total raw allocated bytes: 1023.00MiB
Allocatable bytes remaining: 1.00MiB
Unallocatable raw bytes: 0.00B
Unallocatable bytes that can be reclaimed by balancing: 0.00B

Estimated virtual space left to use for metadata: 51.05MiB Estimated virtual space left to use for data: 96.00KiB

Allocated raw disk bytes by chunk type. Parity is a reserved part of the allocated bytes, limiting the amount that can be used for data or metadata:

|
| flags               allocated         used       parity
| -----               ---------         ----       ------
| DATA|DUP            904.62MiB    904.44MiB        0.00B
| SYSTEM|DUP           16.00MiB     32.00KiB        0.00B
| METADATA|DUP        102.38MiB      1.28MiB        0.00B

Allocated bytes per device:

|
| devid      total size    allocated path
| -----      ----------    --------- ----
| 1             1.00GiB   1023.00MiB /dev/loop0

Allocated bytes per device, split up per chunk type. Parity bytes are again part of the total amount of allocated bytes.

|
| Device ID: 1
| | flags               allocated       parity
| | -----               ---------       ------
| | DATA|DUP            904.62MiB        0.00B
| | SYSTEM|DUP           16.00MiB        0.00B
| | METADATA|DUP        102.38MiB        0.00B

Unallocatable raw bytes per device:

|
| devid    unallocatable
| -----    -------------
| 1               0.00B

---- >8 ----

Now we're gonna cause some neat 1578MiB to happen that we're going to free up later to reproduce the failure:

-# dd if=/dev/zero of=image bs=1 count=0 seek=2602M 0+0 records in
0+0 records out
0 bytes copied, 0.000188621 s, 0.0 kB/s
-# losetup -c /dev/loop0
-# btrfs fi resize max mountpoint/
Resize 'mountpoint/' of 'max'
-# dd if=/dev/zero of=mountpoint/1578MiB bs=1M
dd: error writing 'mountpoint/1578MiB': No space left on device 790+0 records in
789+0 records out
827326464 bytes (827 MB, 789 MiB) copied, 12.2452 s, 67.6 MB/s

---- >8 ----

-# python3
import btrfs
fs = btrfs.FileSystem('/bork/mountpoint/')
for d in fs.dev_extents():
print("start {} end {} vaddr {}".format(d.paddr, d.paddr + d.length,
d.vaddr))

start 1048576 end 11534336 vaddr 547880960
start 11534336 end 22020096 vaddr 547880960
start 22020096 end 30408704 vaddr 22020096
start 30408704 end 38797312 vaddr 22020096
start 38797312 end 92471296 vaddr 30408704
start 92471296 end 146145280 vaddr 30408704
start 146145280 end 213254144 vaddr 84082688
start 213254144 end 280363008 vaddr 84082688
start 280363008 end 397803520 vaddr 151191552
start 397803520 end 515244032 vaddr 151191552
start 515244032 end 632684544 vaddr 268632064
start 632684544 end 750125056 vaddr 268632064
start 750125056 end 867565568 vaddr 386072576
start 867565568 end 985006080 vaddr 386072576
start 985006080 end 1029373952 vaddr 503513088
start 1029373952 end 1073741824 vaddr 503513088
start 1073741824 end 1358954496 vaddr 558366720
start 1358954496 end 1644167168 vaddr 558366720
start 1644167168 end 1929379840 vaddr 843579392
start 1929379840 end 2214592512 vaddr 843579392
start 2214592512 end 2471493632 vaddr 1128792064
start 2471493632 end 2728394752 vaddr 1128792064

The last three block groups were just added, using up the new space:

for vaddr in (558366720, 843579392, 1128792064):
print(fs.block_group(vaddr))

block group vaddr 558366720 length 285212672 flags DATA|DUP used 285212672 used_pct 100
block group vaddr 843579392 length 285212672 flags DATA|DUP used 285212672 used_pct 100
block group vaddr 1128792064 length 256901120 flags DATA|DUP used 256901120 used_pct 100

By the way.. for the first and second time (to do the writeup) I did this, extent allocation looks like paste linked below O_o... I have no idea where the pattern with decreasing sizes the first time is coming from, and no idea why the second time all the data is being stored in 144KiB extents...

http://paste.debian.net/plainh/537bdd95

---- >8 ----

Ok, now let's extend the disk to our famous number of 7880M

-# dd if=/dev/zero of=image bs=1 count=0 seek=7880M 0+0 records in
0+0 records out
0 bytes copied, 0.000185768 s, 0.0 kB/s
-# losetup -c /dev/loop0
-# btrfs fi resize max mountpoint/
Resize 'mountpoint/' of 'max'

-# ./show_usage.py /bork/mountpoint/
[...]
Total raw filesystem size: 7.70GiB
Total raw allocated bytes: 2.54GiB
Allocatable bytes remaining: 5.16GiB
[...]

-# dd if=/dev/zero of=mountpoint/bakkiepleur bs=1M
dd: error writing 'mountpoint/bakkiepleur': No space left on device 2384+0 records in
2383+0 records out
2498756608 bytes (2.5 GB, 2.3 GiB) copied, 34.1946 s, 73.1 MB/s

-# ./show_usage.py /bork/mountpoint/
Target profile for SYSTEM (chunk tree): DUP
Target profile for METADATA: DUP
Target profile for DATA: DUP
Mixed groups: False

Virtual space usage by block group type:

|
| type                    total         used
| ----                    -----         ----
| Data                  3.54GiB      3.54GiB
| System                8.00MiB     16.00KiB
| Metadata            307.19MiB    102.05MiB

Total raw filesystem size: 7.70GiB
Total raw allocated bytes: 7.69GiB
Allocatable bytes remaining: 1.00MiB
Unallocatable raw bytes: 0.00B
Unallocatable bytes that can be reclaimed by balancing: 0.00B

Estimated virtual space left to use for metadata: 205.64MiB Estimated virtual space left to use for data: 0.00B

Allocated raw disk bytes by chunk type. Parity is a reserved part of the allocated bytes, limiting the amount that can be used for data or metadata:

|
| flags               allocated         used       parity
| -----               ---------         ----       ------
| DATA|DUP              7.08GiB      7.08GiB        0.00B
| SYSTEM|DUP           16.00MiB     32.00KiB        0.00B
| METADATA|DUP        614.38MiB    204.09MiB        0.00B

Allocated bytes per device:

|
| devid      total size    allocated path
| -----      ----------    --------- ----
| 1             7.70GiB      7.69GiB /dev/loop0

Allocated bytes per device, split up per chunk type. Parity bytes are again part of the total amount of allocated bytes.

|
| Device ID: 1
| | flags               allocated       parity
| | -----               ---------       ------
| | DATA|DUP              7.08GiB        0.00B
| | SYSTEM|DUP           16.00MiB        0.00B
| | METADATA|DUP        614.38MiB        0.00B

Unallocatable raw bytes per device:

|
| devid    unallocatable
| -----    -------------
| 1               0.00B

---- >8 ----

Now, generating the gap in the physical is as simple as dropping the 1578MiB file and triggering a transaction commit to clean up empty block groups:

-# rm mountpoint/1578MiB
-# btrfs fi sync mountpoint/

for d in fs.dev_extents():
print("start {} end {} vaddr {}".format(d.paddr, d.paddr + d.length,
d.vaddr))

[558366720, 843579392 and 1128792064 are gone]

start 2728394752 end 3567255552 vaddr 1385693184
start 3567255552 end 4406116352 vaddr 1385693184
start 4406116352 end 5244977152 vaddr 2224553984
start 5244977152 end 6083837952 vaddr 2224553984
start 6083837952 end 6352273408 vaddr 3063414784
start 6352273408 end 6620708864 vaddr 3063414784
start 6620708864 end 7441743872 vaddr 3331850240
start 7441743872 end 8262778880 vaddr 3331850240

-# ./show_usage.py /bork/mountpoint/
[...]
Allocatable bytes remaining: 1.54GiB
[...]

---- >8 ----

And then...

-# dd if=/dev/zero of=mountpoint/omgwtfbbq bs=1M
dd: error writing 'mountpoint/omgwtfbbq': No space left on device 801+0 records in
800+0 records out
838860800 bytes (839 MB, 800 MiB) copied, 12.6689 s, 66.2 MB/s

We happily write 800MiB(!), destroying the first 22MiB of the device extent at 2728394752...

for d in fs.dev_extents():
print("start {} end {} vaddr {}".format(d.paddr, d.paddr + d.length,
d.vaddr))

[...]
start 1912602624 end 2751463424 vaddr 4152885248
start 2728394752 end 3567255552 vaddr 1385693184
[...]

import btrfs
fs = btrfs.FileSystem('/bork/mountpoint/')
paddr = 1912602624
tree = btrfs.ctree.DEV_TREE_OBJECTID
devid = 1
for header, data in btrfs.ioctl.search_v2(fs.fd, tree, key, key):
btrfs.utils.pretty_print(btrfs.ctree.DevExtent(header, data))

<btrfs.ctree.DevExtent (1 DEV_EXTENT 1912602624)>
devid: 1 (key objectid)
paddr: 1912602624 (key offset)
length: 838860800
chunk_offset: 4152885248
uuid: 00000000-0000-0000-0000-000000000000
chunk_objectid: 256
chunk_tree: 3

Ouch!

-# ./show_usage.py /bork/mountpoint/

[...]
Total raw filesystem size: 7.70GiB
Total raw allocated bytes: 7.72GiB
Allocatable bytes remaining: 0.00B
Unallocatable raw bytes: -22020096.00B
[...]

---- >8 ----

And now more fun:

-# btrfs scrub status mountpoint/
scrub status for 1f7a998a-4ea3-4117-8b42-9d0e6f1d3b4c

	scrub started at Tue Oct  9 01:35:30 2018 and finished after 00:00:13
	total bytes scrubbed: 6.52GiB with 0 errors

But that's probably because it's all data from /dev/zero...

-# btrfs check /dev/loop0
Checking filesystem on /dev/loop0
UUID: 1f7a998a-4ea3-4117-8b42-9d0e6f1d3b4c
checking extents
Device extent[1, 2728394752, 838860800] existed.
Chunk[256, 228, 1385693184] stripe[1, 2728394752] dismatch dev extent[1, 1912602624, 838860800]
Dev extent's total-byte(7445938176) is not equal to byte-used(8284798976) in dev[1, 216, 1]
ERROR: errors found in extent allocation tree or chunk allocation checking free space tree
checking fs roots
checking only csum items (without verifying data)
checking root refs
found 3917824000 bytes used, error(s) found
total csum bytes: 3722464
total tree bytes: 106020864
total fs tree bytes: 48955392
total extent tree bytes: 48644096
btree space waste bytes: 1428937
file data blocks allocated: 3811803136
referenced 3811803136

This one actually already detects that something is wrong.

Signature · 1 line

Hans van Kranenburg

Qu Wenruo

2018-10-05 07:51 UTC #20 lore raw

In reply to Hans van Kranenburg

Quoted 4 lines · Hans van Kranenburg

This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374

For that bug, did you succeed in reproducing the bug?

I'm adding dev extent overlap checking in btrfs_verify_dev_extents() and btrfs-progs.
I think it could help to detect such problem.

Thanks,
Qu

Quoted 42 lines

The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Moo!,
Knorrie
Hans van Kranenburg (6):
btrfs: alloc_chunk: do not refurbish num_bytes
btrfs: alloc_chunk: improve chunk size variable name
btrfs: alloc_chunk: fix more DUP stripe size handling
btrfs: fix ncopies raid_attr for RAID56
btrfs: introduce nparity raid_attr
btrfs: alloc_chunk: rework chunk/stripe calculations
 fs/btrfs/volumes.c | 84 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  4 ++-
 2 files changed, 45 insertions(+), 43 deletions(-)

David Sterba

2018-10-11 15:13 UTC #21 lore raw

In reply to Hans van Kranenburg

Quoted 18 lines · Hans van Kranenburg

This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.

Quoted 8 lines

I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.

The patches look good, thanks. Problem is explained, preparatory work is separated from the fix itself.

Quoted 3 lines

The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.

Almost nobody does it right, no worries. If you can identify a single patch that introduces a bug then it's for Fixes:, otherwise a CC: stable with version where it makes sense & applies is enough. I do that check myself regardless of what's in the patch.

I ran the patches in a VM and hit a division-by-zero in test fstests/btrfs/011, stacktrace below. First guess is that it's caused by patch 3/6.

[ 3116.065595] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 1 transid 5 /dev/vdb
[ 3116.071274] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 2 transid 5 /dev/vdc
[ 3116.087086] BTRFS info (device vdb): disk space caching is enabled
[ 3116.088644] BTRFS info (device vdb): has skinny extents
[ 3116.089796] BTRFS info (device vdb): flagging fs with big metadata feature
[ 3116.093971] BTRFS info (device vdb): checking UUID tree
[ 3125.853755] BTRFS info (device vdb): dev_replace from /dev/vdb (devid 1) to /dev/vdd started
[ 3125.860269] divide error: 0000 [#1] PREEMPT SMP
[ 3125.861264] CPU: 1 PID: 6477 Comm: btrfs Not tainted 4.19.0-rc7-default+ #288
[ 3125.862841] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
[ 3125.865385] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.870541] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.871862] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.873587] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.874677] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.875816] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.876742] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.877657] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.878862] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.880080] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0
[ 3125.881485] Call Trace:
[ 3125.882105]  do_chunk_alloc+0x266/0x3e0 [btrfs]
[ 3125.882841]  btrfs_inc_block_group_ro+0x10e/0x160 [btrfs]
[ 3125.883875]  scrub_enumerate_chunks+0x18b/0x5d0 [btrfs]
[ 3125.884658]  ? is_module_address+0x11/0x30
[ 3125.885271]  ? wait_for_completion+0x160/0x190
[ 3125.885928]  btrfs_scrub_dev+0x1b8/0x5a0 [btrfs]
[ 3125.887767]  ? start_transaction+0xa1/0x470 [btrfs]
[ 3125.888648]  btrfs_dev_replace_start.cold.19+0x155/0x17e [btrfs]
[ 3125.889459]  btrfs_dev_replace_by_ioctl+0x35/0x60 [btrfs]
[ 3125.890251]  btrfs_ioctl+0x2a94/0x31d0 [btrfs]
[ 3125.890885]  ? do_sigaction+0x7c/0x210
[ 3125.891731]  ? do_vfs_ioctl+0xa2/0x6b0
[ 3125.892652]  do_vfs_ioctl+0xa2/0x6b0
[ 3125.893642]  ? do_sigaction+0x1a7/0x210
[ 3125.894665]  ksys_ioctl+0x3a/0x70
[ 3125.895523]  __x64_sys_ioctl+0x16/0x20
[ 3125.896339]  do_syscall_64+0x5a/0x1a0
[ 3125.896949]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 3125.897638] RIP: 0033:0x7f6de28ecaa7
[ 3125.901313] RSP: 002b:00007ffe963da9c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 3125.902486] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f6de28ecaa7
[ 3125.903538] RDX: 00007ffe963dae00 RSI: 00000000ca289435 RDI: 0000000000000003
[ 3125.904878] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 3125.905788] R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffe963de26f
[ 3125.906700] R13: 0000000000000001 R14: 0000000000000004 R15: 000055fceeceb2a0
[ 3125.907954] Modules linked in: btrfs libcrc32c xor zstd_decompress zstd_compress xxhash raid6_pq loop
[ 3125.909342] ---[ end trace 5492bb467d3be2da ]---
[ 3125.910031] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.913600] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.914595] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.916209] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.917701] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.919209] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.920782] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.922413] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.924264] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.925627] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0

Hans van Kranenburg

author

2018-10-11 19:40 UTC #22 lore raw

Quoted 25 lines · David Sterba

Quoted 24 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.

Quoted 5 lines

Quoted 2 lines
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The patches look good, thanks. Problem is explained, preparatory work is separated from the fix itself.

\o/

Quoted 8 lines

Quoted 3 lines
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Almost nobody does it right, no worries. If you can identify a single patch that introduces a bug then it's for Fixes:, otherwise a CC: stable with version where it makes sense & applies is enough. I do that check myself regardless of what's in the patch.

It's 92e222df7b and the thing I'm not sure about is if it also will catch the same patch which was already backported to LTS kernels since 92e222df7b also has Fixes in it... So by now the new bug is in 4.19, 4.14, 4.9, 4.4, 3.16...

Quoted 3 lines

I ran the patches in a VM and hit a division-by-zero in test fstests/btrfs/011, stacktrace below. First guess is that it's caused by patch 3/6.

Ah interesting, dev replace.

I'll play around with replace and find out how to run the tests properly and then reproduce this.

The code introduced in patch 3 is removed again in patch 6, so I don't suspect that one. :)

But, I'll find out.

Thanks for testing.

Hans

Quoted 59 lines

[ 3116.065595] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 1 transid 5 /dev/vdb
[ 3116.071274] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 2 transid 5 /dev/vdc
[ 3116.087086] BTRFS info (device vdb): disk space caching is enabled
[ 3116.088644] BTRFS info (device vdb): has skinny extents
[ 3116.089796] BTRFS info (device vdb): flagging fs with big metadata feature
[ 3116.093971] BTRFS info (device vdb): checking UUID tree
[ 3125.853755] BTRFS info (device vdb): dev_replace from /dev/vdb (devid 1) to /dev/vdd started
[ 3125.860269] divide error: 0000 [#1] PREEMPT SMP
[ 3125.861264] CPU: 1 PID: 6477 Comm: btrfs Not tainted 4.19.0-rc7-default+ #288
[ 3125.862841] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
[ 3125.865385] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.870541] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.871862] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.873587] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.874677] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.875816] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.876742] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.877657] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.878862] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.880080] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0
[ 3125.881485] Call Trace:
[ 3125.882105]  do_chunk_alloc+0x266/0x3e0 [btrfs]
[ 3125.882841]  btrfs_inc_block_group_ro+0x10e/0x160 [btrfs]
[ 3125.883875]  scrub_enumerate_chunks+0x18b/0x5d0 [btrfs]
[ 3125.884658]  ? is_module_address+0x11/0x30
[ 3125.885271]  ? wait_for_completion+0x160/0x190
[ 3125.885928]  btrfs_scrub_dev+0x1b8/0x5a0 [btrfs]
[ 3125.887767]  ? start_transaction+0xa1/0x470 [btrfs]
[ 3125.888648]  btrfs_dev_replace_start.cold.19+0x155/0x17e [btrfs]
[ 3125.889459]  btrfs_dev_replace_by_ioctl+0x35/0x60 [btrfs]
[ 3125.890251]  btrfs_ioctl+0x2a94/0x31d0 [btrfs]
[ 3125.890885]  ? do_sigaction+0x7c/0x210
[ 3125.891731]  ? do_vfs_ioctl+0xa2/0x6b0
[ 3125.892652]  do_vfs_ioctl+0xa2/0x6b0
[ 3125.893642]  ? do_sigaction+0x1a7/0x210
[ 3125.894665]  ksys_ioctl+0x3a/0x70
[ 3125.895523]  __x64_sys_ioctl+0x16/0x20
[ 3125.896339]  do_syscall_64+0x5a/0x1a0
[ 3125.896949]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 3125.897638] RIP: 0033:0x7f6de28ecaa7
[ 3125.901313] RSP: 002b:00007ffe963da9c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 3125.902486] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f6de28ecaa7
[ 3125.903538] RDX: 00007ffe963dae00 RSI: 00000000ca289435 RDI: 0000000000000003
[ 3125.904878] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 3125.905788] R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffe963de26f
[ 3125.906700] R13: 0000000000000001 R14: 0000000000000004 R15: 000055fceeceb2a0
[ 3125.907954] Modules linked in: btrfs libcrc32c xor zstd_decompress zstd_compress xxhash raid6_pq loop
[ 3125.909342] ---[ end trace 5492bb467d3be2da ]---
[ 3125.910031] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.913600] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.914595] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.916209] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.917701] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.919209] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.920782] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.922413] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.924264] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.925627] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0

Signature · 1 line

Hans van Kranenburg

Hans van Kranenburg

author

2018-10-11 20:34 UTC #23 lore raw

Quoted 52 lines · Hans van Kranenburg

Quoted 25 lines · David Sterba
Quoted 24 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Quoted 5 lines
Quoted 2 lines
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The patches look good, thanks. Problem is explained, preparatory work is separated from the fix itself.
\o/
Quoted 8 lines
Quoted 3 lines
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Almost nobody does it right, no worries. If you can identify a single patch that introduces a bug then it's for Fixes:, otherwise a CC: stable with version where it makes sense & applies is enough. I do that check myself regardless of what's in the patch.
It's 92e222df7b and the thing I'm not sure about is if it also will catch the same patch which was already backported to LTS kernels since 92e222df7b also has Fixes in it... So by now the new bug is in 4.19, 4.14, 4.9, 4.4, 3.16...
Quoted 3 lines
I ran the patches in a VM and hit a division-by-zero in test fstests/btrfs/011, stacktrace below. First guess is that it's caused by patch 3/6.

Quoted 7 lines

Ah interesting, dev replace.
I'll play around with replace and find out how to run the tests properly and then reproduce this.
The code introduced in patch 3 is removed again in patch 6, so I don't suspect that one. :)

Actually, while writing this I realize that this means it should be tested separately (like, older kernel with only 3), because, well, obvious I guess.

Quoted 67 lines

But, I'll find out.

Thanks for testing.

Hans

Quoted 59 lines

[ 3116.065595] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 1 transid 5 /dev/vdb
[ 3116.071274] BTRFS: device fsid e3bd8db5-304f-4b1a-8488-7791ea94088f devid 2 transid 5 /dev/vdc
[ 3116.087086] BTRFS info (device vdb): disk space caching is enabled
[ 3116.088644] BTRFS info (device vdb): has skinny extents
[ 3116.089796] BTRFS info (device vdb): flagging fs with big metadata feature
[ 3116.093971] BTRFS info (device vdb): checking UUID tree
[ 3125.853755] BTRFS info (device vdb): dev_replace from /dev/vdb (devid 1) to /dev/vdd started
[ 3125.860269] divide error: 0000 [#1] PREEMPT SMP
[ 3125.861264] CPU: 1 PID: 6477 Comm: btrfs Not tainted 4.19.0-rc7-default+ #288
[ 3125.862841] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
[ 3125.865385] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.870541] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.871862] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.873587] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.874677] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.875816] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.876742] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.877657] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.878862] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.880080] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0
[ 3125.881485] Call Trace:
[ 3125.882105]  do_chunk_alloc+0x266/0x3e0 [btrfs]
[ 3125.882841]  btrfs_inc_block_group_ro+0x10e/0x160 [btrfs]
[ 3125.883875]  scrub_enumerate_chunks+0x18b/0x5d0 [btrfs]
[ 3125.884658]  ? is_module_address+0x11/0x30
[ 3125.885271]  ? wait_for_completion+0x160/0x190
[ 3125.885928]  btrfs_scrub_dev+0x1b8/0x5a0 [btrfs]
[ 3125.887767]  ? start_transaction+0xa1/0x470 [btrfs]
[ 3125.888648]  btrfs_dev_replace_start.cold.19+0x155/0x17e [btrfs]
[ 3125.889459]  btrfs_dev_replace_by_ioctl+0x35/0x60 [btrfs]
[ 3125.890251]  btrfs_ioctl+0x2a94/0x31d0 [btrfs]
[ 3125.890885]  ? do_sigaction+0x7c/0x210
[ 3125.891731]  ? do_vfs_ioctl+0xa2/0x6b0
[ 3125.892652]  do_vfs_ioctl+0xa2/0x6b0
[ 3125.893642]  ? do_sigaction+0x1a7/0x210
[ 3125.894665]  ksys_ioctl+0x3a/0x70
[ 3125.895523]  __x64_sys_ioctl+0x16/0x20
[ 3125.896339]  do_syscall_64+0x5a/0x1a0
[ 3125.896949]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 3125.897638] RIP: 0033:0x7f6de28ecaa7
[ 3125.901313] RSP: 002b:00007ffe963da9c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 3125.902486] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f6de28ecaa7
[ 3125.903538] RDX: 00007ffe963dae00 RSI: 00000000ca289435 RDI: 0000000000000003
[ 3125.904878] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 3125.905788] R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffe963de26f
[ 3125.906700] R13: 0000000000000001 R14: 0000000000000004 R15: 000055fceeceb2a0
[ 3125.907954] Modules linked in: btrfs libcrc32c xor zstd_decompress zstd_compress xxhash raid6_pq loop
[ 3125.909342] ---[ end trace 5492bb467d3be2da ]---
[ 3125.910031] RIP: 0010:__btrfs_alloc_chunk+0x368/0xa70 [btrfs]
[ 3125.913600] RSP: 0018:ffffa4ea0409fa48 EFLAGS: 00010206
[ 3125.914595] RAX: 0000000004000000 RBX: ffff94e074374508 RCX: 0000000000000002
[ 3125.916209] RDX: 0000000000000000 RSI: ffff94e017818c80 RDI: 0000000002000000
[ 3125.917701] RBP: 0000000080800000 R08: 0000000000000000 R09: 0000000000000002
[ 3125.919209] R10: 0000000300000000 R11: 0000000080900000 R12: 0000000000000000
[ 3125.920782] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000002
[ 3125.922413] FS:  00007f6de34208c0(0000) GS:ffff94e07d600000(0000) knlGS:0000000000000000
[ 3125.924264] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3125.925627] CR2: 00007ffe963d5ce8 CR3: 000000007659b000 CR4: 00000000000006e0

Signature · 1 line

Hans van Kranenburg

David Sterba

2018-11-13 15:04 UTC #24 lore raw

In reply to Hans van Kranenburg

Quoted 57 lines · Hans van Kranenburg

Quoted 25 lines · David Sterba
Quoted 24 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Quoted 5 lines
Quoted 2 lines
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The patches look good, thanks. Problem is explained, preparatory work is separated from the fix itself.
\o/
Quoted 8 lines
Quoted 3 lines
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Almost nobody does it right, no worries. If you can identify a single patch that introduces a bug then it's for Fixes:, otherwise a CC: stable with version where it makes sense & applies is enough. I do that check myself regardless of what's in the patch.
It's 92e222df7b and the thing I'm not sure about is if it also will catch the same patch which was already backported to LTS kernels since 92e222df7b also has Fixes in it... So by now the new bug is in 4.19, 4.14, 4.9, 4.4, 3.16...
Quoted 3 lines
I ran the patches in a VM and hit a division-by-zero in test fstests/btrfs/011, stacktrace below. First guess is that it's caused by patch 3/6.
Ah interesting, dev replace.
I'll play around with replace and find out how to run the tests properly and then reproduce this.

Quoted 6 lines

The code introduced in patch 3 is removed again in patch 6, so I don't suspect that one. :)
But, I'll find out.
Thanks for testing.

I've merged patches 1-5 to misc-next as they seem to be safe and pass fstests, please let me know when you have updates to the last one. Thanks.

Hans van Kranenburg

author

2018-11-13 16:46 UTC #25 lore raw

Quoted 61 lines · David Sterba

Quoted 60 lines · Hans van Kranenburg
Quoted 25 lines · David Sterba
Quoted 24 lines · Hans van Kranenburg
This patch set contains an additional fix for a newly exposed bug after the previous attempt to fix a chunk allocator bug for new DUP chunks:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
The DUP fix is "fix more DUP stripe size handling". I did that one before starting to change more things so it can be applied to earlier LTS kernels.
Besides that patch, which is fixing the bug in a way that is least intrusive, I added a bunch of other patches to help getting the chunk allocator code in a state that is a bit less error-prone and bug-attracting.
When running this and trying the reproduction scenario, I can now see that the created DUP device extent is 827326464 bytes long, which is good.
I wrote and tested this on top of linus 4.19-rc5. I still need to create a list of related use cases and test more things to at least walk through a bunch of obvious use cases to see if there's nothing exploding too quickly with these changes. However, I'm quite confident about it, so I'm sharing all of it already.
Quoted 5 lines
Quoted 2 lines
Any feedback and review is appreciated. Be gentle and keep in mind that I'm still very much in a learning stage regarding kernel development.
The patches look good, thanks. Problem is explained, preparatory work is separated from the fix itself.
\o/
Quoted 8 lines
Quoted 3 lines
The stable patches handling workflow is not 100% clear to me yet. I guess I have to add a Fixes: in the DUP patch which points to the previous commit 92e222df7b.
Almost nobody does it right, no worries. If you can identify a single patch that introduces a bug then it's for Fixes:, otherwise a CC: stable with version where it makes sense & applies is enough. I do that check myself regardless of what's in the patch.
It's 92e222df7b and the thing I'm not sure about is if it also will catch the same patch which was already backported to LTS kernels since 92e222df7b also has Fixes in it... So by now the new bug is in 4.19, 4.14, 4.9, 4.4, 3.16...
Quoted 3 lines
I ran the patches in a VM and hit a division-by-zero in test fstests/btrfs/011, stacktrace below. First guess is that it's caused by patch 3/6.
Ah interesting, dev replace.
I'll play around with replace and find out how to run the tests properly and then reproduce this.
The code introduced in patch 3 is removed again in patch 6, so I don't suspect that one. :)

Quoted 7 lines

Quoted 3 lines
But, I'll find out.
Thanks for testing.
I've merged patches 1-5 to misc-next as they seem to be safe and pass fstests, please let me know when you have updates to the last one. Thanks.

I'll definitely follow up on that. It has not happened because something about todo lists and ordering work and not doing too many things at the same time.

Thanks for already confirming it was not patch 3 but 6 that has the problem, I already suspected that.

For patch 3, the actual fix, how do we get that on top of old stable kernels where the previous fix (92e222df7b) is in? Because of the Fixes:
line that one was picked again in 4.14, 4.9, 4.4 and even 3.16. How does this work? Does it need all the other commit ids from those branches in Fixes lines? Or is there magic somewhere that does this?

From your "development cycle open" mails, I understand that for testing myself, I would test based on misc-next, for-next or for-x.y in that order depending on where the first 5 are yet at that moment?

Hans